add pin-source feature for pkcs11 uri by ortibazar · Pull Request #236 · OpenSC/libp11

ortibazar · 2018-08-13T18:10:51Z

Implement "pin-source" attribute using below specs

https://tools.ietf.org/html/rfc7512#page-9

ortibazar · 2018-08-14T15:06:47Z

Hi, Could you review this PR?

mtrojnar · 2018-08-14T15:16:43Z

I promise I will. Just give me a few days.

ortibazar · 2018-08-14T15:18:20Z

Thank you.

mtrojnar

I like the idea, the implementation, and the coding style consistency.

There are however two RFC 7512 requirements that remain unimplemented:

Refusing of using both pin-source and pin-value (Section 2.4).

If a URI contains both "pin-source" and "pin-value" query attributes, the URI SHOULD be refused as invalid.

This requirement should be fairly easy to implement by refusing to parse pin-set and pin-source if pin_set
is already set to 1.

Path normalization (Section 2.6).

The value of "pin-source", if containing a "file:" URI or "|", MUST be compared using the simple string comparison after the full syntax-based normalization, as specified in Section 6.2.2 of [RFC3986], is applied. If the value of the "pin-source" attribute is believed to be overloaded, the case and percent-encoding normalization SHOULD be applied before the values are compared, but the exact mechanism of comparison is left to the application.

frankmorgner · 2018-08-20T00:19:45Z

I second that and would like to add, that you shouldn't use popen() to do the job, see man 3 popen.

mtrojnar · 2018-08-20T10:47:59Z

@frankmorgner Yes, popen(3) indeed introduces additional risks in applications where attackers may control environment variables, but not the name of the executable. I don't think this is likely to be the case for our engine. I think exposing the pin-source functionality may be dangerous in some applications regardless of the implementation. This, however, is a vulnerability of the RFC, not specific to our implementation.
An obvious advantage of using popen(3) is portability. Alternative solutions would require at least a separate Windows implementation.

frankmorgner · 2018-08-21T08:22:11Z

AFAIK, there is no clear oversight on how libp11 (engine and library) is actually used. So likely or not, we should prevent problems when seeing them. Programmer's laziness or lack of specification are no good excuses.

mtrojnar · 2018-08-21T09:05:10Z

@frankmorgner What I meant was not "I am too lazy to implement a proper solution", but rather "I think that a complex (and thus error-prone) solution to fix a problem that unlikely to occur would do more harm than good here". This is just my opinion and I understand and appreciate your point of view.

ortibazar · 2018-08-21T21:20:42Z

I agree with @mtrojnar that

exposing the pin-source functionality may be dangerous in some applications regardless of the implementation

@frankmorgner Could you suggest an alternative solution for popen?

…rmalization by comparing case insentive

frankmorgner · 2018-08-28T12:17:27Z

src/eng_parse.c

 	return 1;
 }

+static int strnicmp(const char *cs, const char *ct, size_t count)


have you tried strncasecmp()?

frankmorgner · 2018-08-28T12:33:28Z

An alternative to popen would be to use fork and the exec family, as suggested by the man page. That, however, would require a seperate implementation for Windows (MSDN has example code). Indeed, that's more complex than just using popen(), but given the examples of both variants, I don't think that there's too much that can go wrong.

Anyway, if you still want to use popen, maybe it's possible to deactivate this feature by default. A note in the documentation for activating the feature can then make the preconditions for safe use more apparent.

frankmorgner · 2019-04-19T19:40:06Z

This pull request introduces 1 alert when merging a659aba into 97700cb - view on LGTM.com

new alerts:

1 for Wrong type of arguments to formatting function

Comment posted by LGTM.com

frankmorgner · 2019-04-20T00:28:00Z

src/eng_parse.c

+{
+	FILE *fp;
+
+	fp = fopen(path, "r");


could use OpenSSL's bio methods

frankmorgner · 2019-04-20T00:31:07Z

@Ortigali an alternative to popen is suggested in the man page (linked above with http://manpages.ubuntu.com/manpages/trusty/en/man3/popen.3posix.html#rationale)

This is a modified version of original PR: OpenSC#236

According to https://tools.ietf.org/html/rfc7512#page-9: """ 2.4. PKCS OpenSC#11 URI Scheme Query Attribute Semantics An application can always ask for a PIN by any means it decides to. What is more, in order not to limit PKCS OpenSC#11 URI portability, the "pin-source" attribute value format and interpretation is left to be implementation specific. However, the following rules SHOULD be followed in descending order for the value of the "pin-source" attribute: o If the value represents a URI, it SHOULD be treated as an object containing the PIN. Such a URI may be "file:", "https:", another PKCS OpenSC#11 URI, or something else. o If the value contains "|<absolute-command-path>", the implementation SHOULD read the PIN from the output of an application specified with absolute path "<absolute-command- path>". Note that character "|" representing a pipe does not have to be percent-encoded in the query component of a PKCS OpenSC#11 URI. o Interpret the value as needed in an implementation-dependent way. """ This patch is based on: OpenSC#236, but implements only the first clause of RFC, since the second one is considered as dangerous. For example, such functionality is required by FreeIPA (Bind + OpenDNSSEC). Fixes: OpenSC#273 Co-authored-by: Ortigali Bazarov <ortigali.bazarov@gmail.com> Signed-off-by: Stanislav Levin <slev@altlinux.org>

According to https://tools.ietf.org/html/rfc7512#page-9: """ 2.4. PKCS #11 URI Scheme Query Attribute Semantics An application can always ask for a PIN by any means it decides to. What is more, in order not to limit PKCS #11 URI portability, the "pin-source" attribute value format and interpretation is left to be implementation specific. However, the following rules SHOULD be followed in descending order for the value of the "pin-source" attribute: o If the value represents a URI, it SHOULD be treated as an object containing the PIN. Such a URI may be "file:", "https:", another PKCS #11 URI, or something else. o If the value contains "|<absolute-command-path>", the implementation SHOULD read the PIN from the output of an application specified with absolute path "<absolute-command- path>". Note that character "|" representing a pipe does not have to be percent-encoded in the query component of a PKCS #11 URI. o Interpret the value as needed in an implementation-dependent way. """ This patch is based on: #236, but implements only the first clause of RFC, since the second one is considered as dangerous. For example, such functionality is required by FreeIPA (Bind + OpenDNSSEC). Fixes: #273 Co-authored-by: Ortigali Bazarov <ortigali.bazarov@gmail.com> Signed-off-by: Stanislav Levin <slev@altlinux.org>

mtrojnar · 2019-10-04T16:49:23Z

I merged #309 instead of this PR.

Ortigali Bazarov added 2 commits August 13, 2018 14:06

add pin-source feature for pkcs11 uri

c3f9ebc

fix pin.txt directory in rsa-evp-sign.softhsm

9daf120

mtrojnar self-requested a review August 19, 2018 11:41

mtrojnar requested changes Aug 19, 2018

View reviewed changes

implement Refusing of using both pin-source and pin-value and Path no…

6ce8469

…rmalization by comparing case insentive

frankmorgner reviewed Aug 28, 2018

View reviewed changes

ansasaki mentioned this pull request Feb 13, 2019

pin-source is not handled in PKCS#11 URI #273

Closed

Ortigali Bazarov added 3 commits April 19, 2019 13:43

remove popen call

8248250

change shebang to /bin/sh

abefef0

just to get debug log in travis-ci

a659aba

frankmorgner reviewed Apr 20, 2019

View reviewed changes

src/eng_parse.c Outdated

{

FILE *fp;

fp = fopen(path, "r");

Copy link

Member

frankmorgner Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use OpenSSL's bio methods

change to execv and use BIO

7b1d0b8

stanislavlevin pushed a commit to stanislavlevin/libp11 that referenced this pull request Sep 20, 2019

Add pin-source

be97d75

This is a modified version of original PR: OpenSC#236

stanislavlevin mentioned this pull request Oct 2, 2019

Add support for pin-source within PKCS11 URI #309

Merged

mtrojnar closed this Oct 4, 2019

keldonin mentioned this pull request Apr 6, 2020

support pin-source with piping from a subprocess #333

Closed

Conversation

ortibazar commented Aug 13, 2018

Uh oh!

ortibazar commented Aug 14, 2018

Uh oh!

mtrojnar commented Aug 14, 2018

Uh oh!

ortibazar commented Aug 14, 2018

Uh oh!

mtrojnar left a comment

Choose a reason for hiding this comment

Uh oh!

frankmorgner commented Aug 20, 2018

Uh oh!

mtrojnar commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankmorgner commented Aug 21, 2018

Uh oh!

mtrojnar commented Aug 21, 2018

Uh oh!

ortibazar commented Aug 21, 2018

Uh oh!

frankmorgner Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

frankmorgner commented Aug 28, 2018

Uh oh!

frankmorgner commented Apr 19, 2019

Uh oh!

frankmorgner Apr 20, 2019

Choose a reason for hiding this comment

Uh oh!

frankmorgner commented Apr 20, 2019

Uh oh!

mtrojnar commented Oct 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtrojnar commented Aug 20, 2018 •

edited

Loading