David Woodhouse Assignee
	Description • 9 years ago

RFC7512 defines a URI scheme for PKCS#11 objects, allowing the token and the object therein to be specified.

We should extend PK11_FindCertByNickname() and potentially other functions to accept an identifier string in this standard form.

By making PK11_FindCertByNickname() do it instead of adding a new PK11_FindCertByURI() function, we make it simple for existing applications to start accepting URIs without minimal or no modifications.

	Ryan Sleevi
	Comment 1 • 9 years ago

I'm very much opposed to changing the API of PK11_FindCertByNickname. While I can understand the appeal, I think that's a dangerous and breaking API change.

A new function sounds like the right way. However, I'm not enthused by the notion of having a URI-parser as part of NSS, which is fundamentally what it would require. URI specs - including the PKCS#11 URI spec - are notoriously complex and full of edge cases (as I try to deal w/ interoperability issues between Chrome, Firefox, IE and Safari regarding URI parsing, I'm especially not keen to introduce per-application URI parsing)

If PKCS#11 URI parsing is to be the norm in the future, I think it's entirely reasonable to separate that out into a separate library, so that independent applications (NSS, GnuTLS, etc) can parse the URIs consistently. Having separate URI implementations in each is begging for long-term incompatibility.

	David Woodhouse Assignee
	Comment 2 • 9 years ago

In that case we *could* just use libp11-kit, which is BSD-licensed and fairly ubiquitous across *NIX systems. I had assumed that we'd not want to introduce an external dependency though.

If we *do* use p11-kit, that opens other possibilities which I hadn't yet mentioned — it also handles 'wrapping' PKCS#11 providers so we can cope with them being loaded multiple times within a process (for example when multiple Pidgin protocol plugins load *different* crypto libraries, each of which loads the PKCS#11 provider). I wasn't going to suggest that just yet though... :)

	David Woodhouse Assignee
	Comment 3 • 8 years ago

On changing PK11_FindCertByNickname()... I understand the reticence about changing the API.

But is it really that dangerous? The behaviour will only change in the case where the 'nickname' you provide is actually a valid PKCS#11 URI. Which is extremely unlikely to happen by *accident*. And I think it's reasonable to assume that such a 'nickname' would *never* have actually matched anything before, isn't it? (And hey, if you *do* have a token named 'pkcs11' and an object therein with a carefully crafted nickname to make it look like an RFC7512 URI, if that's even possible, then we can always give precedence to that one).

So the 'dangerous' API change would be that when you pass a PKCS#11 URI to the function, instead of getting no result back, you get the object specified by the URI. I'm not sure how dangerous that really is :)

There are tools which already take a 'nickname' either on the command line or in a config file, and just pass it through to PK11_FindCertFromNickname(). IF we make that function just Do The Right Thing when it sees a valid URI, everything is good. If we introduce a new API, then *all* those tools need to be patched to make them check if the string they have is a valid PKCS#11 URI, then call either PK11_FindCertFromNickname() or PK11_FindCertFromUri() as appropriate. Surely it's better to have that logic live in the library?

At the very least, perhaps we could introduce a PK11_FindCertFromNicknameOrUri(), and then at least the bombing run on applications is just s/PK11_FindCertFromNickname/\1OrUri/ ?

	David Woodhouse Assignee
	Comment 4 • 8 years ago

Let's start with some cleanup to the PK11_FindCert{s,}FromNickname() functions, before we use them as the basis for our new PK11_FindCert{s,}FromURI() functions.

We have a whole bunch of code which is basically identical, modulo the
occasional bug fix that's been applied to one side but not the other,
and gratuitous cosmetic differences.

Factor it out into a helper function and use it from both.

	David Woodhouse Assignee
	Comment 5 • 8 years ago

Make NSSTrustDomain_FindTokenByName() take a ref on the token it returns

AFAICT without this, there's nothing that guarantees that 'tok' still
actually exists, from the moment we drop the locks and return it.

This function isn't exported, and is used precisely once in the entire
code base. So make it take a reference on the token, and make its only
caller release that reference.

	David Woodhouse Assignee
	Comment 6 • 8 years ago

Here comes the fun part. The URI handling code from libp11-kit is suitably licensed, and has been stable for years — it hasn't changed since 2014, and even then it was just an update to reflect some of the final tweaks in the I-D before it was published as RFC7512.

The simplest option is just to import its URI functionality and make it look like NSS code.

	David Woodhouse Assignee
	Comment 7 • 8 years ago

In later patches we ended up having to do locking gymnastics, dropping locks so that we could safely call into PK11_GetTokenInfo() for comparisons. Let's just keep a copy.

Further cleanups are warranted after this patch — like removing 'slot->serial' and a few other variables which are now redundant. Those should, and will, follow as separate patches (not in my current line-up, but soon).

	David Woodhouse Assignee
	Comment 8 • 8 years ago

$ certutil -d sql:/etc/pki/nssdb -U

    slot: NSS Application Slot 00000004
   token: NSS system database
     uri: pkcs11:model=NSS%203;manufacturer=Mozilla%20Foundation;serial=0000000000000000;token=NSS%20system%20database

    slot: NSS User Private Key and Certificate Services
   token: NSS Certificate DB
     uri: pkcs11:model=NSS%203;manufacturer=Mozilla%20Foundation;serial=0000000000000000;token=NSS%20Certificate%20DB

    slot: NSS Internal Cryptographic Services
   token: NSS Generic Crypto Services
     uri: pkcs11:model=NSS%203;manufacturer=Mozilla%20Foundation;serial=0000000000000000;token=NSS%20Generic%20Crypto%20Services

    slot: Yubico Yubikey NEO OTP+CCID 00 00
   token: PIV_II (PIV Card Holder pin)
     uri: pkcs11:model=PKCS%2315%20emulated;manufacturer=piv_II;serial=108421384210c3f5;token=PIV_II%20%28PIV%20Card%20Holder%20pin%29

	David Woodhouse Assignee
	Comment 9 • 8 years ago

Add PK11_GetModuleURI() function

	David Woodhouse Assignee
	Comment 10 • 8 years ago

/modutil -dbdir sql:/etc/pki/nssdb -list

Listing of PKCS #11 Modules
-----------------------------------------------------------
  1. NSS Internal Crypto Services
	   uri: pkcs11:library-description=NSS%20Internal%20Crypto%20Services;library-manufacturer=Mozilla%20Foundation
	 slots: 3 slots attached
	status: loaded

	 slot: NSS Internal Cryptographic Services
	token: NSS Generic Crypto Services
	  uri: pkcs11:model=NSS%203;manufacturer=Mozilla%20Foundation;serial=0000000000000000;token=NSS%20Generic%20Crypto%20Services

	 slot: NSS User Private Key and Certificate Services
	token: NSS Certificate DB
	  uri: pkcs11:model=NSS%203;manufacturer=Mozilla%20Foundation;serial=0000000000000000;token=NSS%20Certificate%20DB
...

	David Woodhouse Assignee
	Comment 11 • 8 years ago

Rename and export find_objects_by_template()

We're going to want this for PK11_FindCertFromURI() and friends.

	David Woodhouse Assignee
	Comment 12 • 8 years ago

Add NSSTrustDomain_FindTokensByURI()

	David Woodhouse Assignee
	Comment 13 • 8 years ago

Add PK11_FindCertsFromURI() and PK11_FindCertFromURI()

Just like their FindCert{s,}FromNickname() counterparts, these functions
take a URI string and return either a list of matching certificates, or
the "best" one.

	David Woodhouse Assignee
	Comment 14 • 8 years ago

Detect PKCS#11 URI in PK11_FindCert{s,}FromNickname and search accordingly

If the "nickname" starts with "pkcs11:" then try treating it as a PKCS#11 URI.
If that *doesn't* yield any results, then keep going and treat it as "token:nickname" on the vague chance that there is actually a token which is bizarrely named just "pkcs11".

Finally, tools like 'curl' will work with PKCS#11 URIs on the command line (as required by things like Fedora packaging guidelines) when built against NSS! :)

	David Woodhouse Assignee
	Comment 15 • 8 years ago

For reference and ease of testing, all the above commits are in
https://github.com/varunnaganathan/nss/commits/20160819

	Robert Relyea
	Comment 16 • 8 years ago

Comment on attachment 8782907 [details] [diff] [review]
0002-Make-NSSTrustDomain_FindTokenByName-take-a-ref-on-th.patch

Review of attachment 8782907 [details] [diff] [review]:
-----------------------------------------------------------------

This patch required more looking than the simple calls indicate. I've verified that after patch 001, findcertbynickname is the only caller of NSSTrustDomain_FindTokenByName(), It's a private function, so there are no applications using it. The new semantic is more consistant with NSS usage.

good patch. Just one minor nit

r+

::: lib/pk11wrap/pk11cert.c
@@ -536,4 @@
>  	*delimit = ':';
>      } else {
>  	slot = PK11_GetInternalKeySlot();
> -	token = PK11Slot_GetNSSToken(slot);

For paranoia sake, please initialize token to NULL in the declaration. That way if someone adds a test and goto loser before we set it in this if statement, then we won't crash randomly in the loser portion.

	Robert Relyea
	Comment 17 • 8 years ago

Comment on attachment 8782916 [details] [diff] [review]
0004-Add-CK_TOKEN_INFO-to-the-PK11SlotInfo-struct-for-cac.patch

Review of attachment 8782916 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pk11wrap/pk11slot.c
@@ -1102,4 @@
>  SECStatus
>  PK11_InitToken(PK11SlotInfo *slot, PRBool loadCerts)
>  {
> -    CK_TOKEN_INFO tokenInfo;

style issue. It might be worth changing this to CK_TOKEN_INFO *tokenInfoPtr and setting it to &slot->tokenInfo.

That would allow you to dromp the slot-> from the changes below (instead tokenInfo. becomes tokenInfoPtr->).

Just a comment, not a required change.

@@ -1285,4 @@
>  SECStatus
>  PK11_TokenRefresh(PK11SlotInfo *slot)
>  {
> -    CK_TOKEN_INFO tokenInfo;

Same comment as

	Robert Relyea
	Comment 18 • 8 years ago

Comment on attachment 8782921 [details] [diff] [review]
0005-Add-PK11_GetTokenURI-and-use-it-from-certutil.patch

Review of attachment 8782921 [details] [diff] [review]:
-----------------------------------------------------------------

This patch requires 0003, which I haven't completed my review, but I have reviewed enough to be OK with the API and basic structure.

The one thing I noticed from this patch is I would have expected the P11URI_Get functions to return const pointers (and take const P11URI pointers). I see that's not the case by our usage here. I guess that makes sense since we don't have ways of setting these structures in the uri except to get them and set the values.

bob

	Robert Relyea
	Comment 19 • 8 years ago

Comment on attachment 8782923 [details] [diff] [review]
0007-Include-PKCS-11-URI-in-modutil-output.patch

Review of attachment 8782923 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/modutil/pk11.c
@@ +430,4 @@
>      /* Print slot and token names */
>      for (i = 0; i < slotCount; i++) {
>          PK11SlotInfo *slot = module->slots[i];
> +        char *tokenUri = PK11_GetTokenURI(slot);

Good, I can cross off having this added to modutil from my list;)

	Robert Relyea
	Comment 20 • 8 years ago

Comment on attachment 8782926 [details] [diff] [review]
0009-Add-NSSTrustDomain_FindTokensByURI.patch

Review of attachment 8782926 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pki/trustdomain.c
@@ +266,5 @@
> +	return NULL;
> +    }
> +    for (tok = (NSSToken *)nssListIterator_Start(td->tokens);
> +	 tok != (NSSToken *)NULL;
> +	 tok = (NSSToken *)nssListIterator_Next(td->tokens)) {

probably should have an assert that i < count. It should be since we counted the list and we're holding the lock, but and assert is cheap peace of mind.

@@ +277,5 @@
> +    tokens[i] = NULL;
> +    nssListIterator_Finish(td->tokens);
> +    NSSRWLock_UnlockRead(td->tokensLock);
> +    return tokens;
> +}

OK we already have NSSTokenArray_Destroy which will properly destroy tokens returned from this function.

	David Woodhouse Assignee
	Comment 21 • 8 years ago

Comment on attachment 8782904 [details] [diff] [review]
0001-Remove-duplication-in-PK11_FindCert-s-FromNickname.patch

Review of attachment 8782904 [details] [diff] [review]:
-----------------------------------------------------------------

I should explicitly point out the "bug fix" that was different.... the FindCert version called pk11_AuthenticateUnfriendly() while the FindCerts version did not. Bob, ISTR you added that... it wasn't *intentionally* omitted from the latter, was it?

	David Woodhouse Assignee
	Comment 22 • 8 years ago

(In reply to Robert Relyea from comment #18)
> The one thing I noticed from this patch is I would have expected the
> P11URI_Get functions to return const pointers (and take const P11URI
> pointers). I see that's not the case by our usage here. I guess that makes
> sense since we don't have ways of setting these structures in the uri except
> to get them and set the values.

Right. I actually got half way through adding 'const' to a bunch of them, before coming to the same realisation and backing it out... :)

	David Woodhouse Assignee
	Comment 23 • 8 years ago

(In reply to Robert Relyea from comment #17)
> > -    CK_TOKEN_INFO tokenInfo;
> 
> style issue. It might be worth changing this to CK_TOKEN_INFO *tokenInfoPtr
> and setting it to &slot->tokenInfo.
> 
> That would allow you to dromp the slot-> from the changes below (instead
> tokenInfo. becomes tokenInfoPtr->).

Hm, I don't much like that because it makes it *less* obvious, in the later code, that this is actually going into slot->

	Robert Relyea
	Comment 24 • 8 years ago

Comment on attachment 8782913 [details] [diff] [review]
0003-Add-PKCS-11-URI-handling-support-in-lib-p11uri.patch

Review of attachment 8782913 [details] [diff] [review]:
-----------------------------------------------------------------

The only real issue here is the pin value handling. We need to zero it before freeing any pin value (or clear it from any static stack data before returning). Fortunately we only have it in allocated buffers, and we control the free routines for those buffers. I've indicated where that should happen.

The rest are mostly 'yes this code is correct and here's why' comments. Basically if it wasn't immediately obvious that the code was correct, I added the comment. Ideally those comments should translate to code comments helping any future user.

I'm going to r+, but I'd like a separate patch the fixed the pin clearing issues. I'd also be happy with an additional patch that adds the comments, but I won't require it.

::: lib/p11uri/p11uri.c
@@ +74,5 @@
> +    char *pin_source;
> +    char *pin_value;
> +};
> +
> +#define P11_URL_WHITESPACE " \n\r\v"

not tab?

@@ +373,5 @@
> +P11URI_SetPinValue(P11URI *uri, const char *pin)
> +{
> +    PORT_Assert(uri != NULL);
> +
> +    PORT_Free(uri->pin_value);

We should zero the pin before we free it. PORT_ZFree(uri->pin_value, PL_strlen(uri->pin_value)); (You'll need to check for NULL in this case. I'm not sure strlen is NULL safe on all platforms).

@@ +441,5 @@
> +        return SECSuccess;
> +
> +    *buf = PR_sprintf_append(*buf, "%s%s=%s",
> +                             *buf ? ";" : P11URI_SCHEME,
> +                             name, value);

yes, PR_sprintf_append frees *buf on error, so we aren't leaking here in the error case.

@@ +529,5 @@
> +    const char *value;
> +
> +    /* Not set */;
> +    if (attr == NULL)
> +        return SECSuccess;

Should we be checking that the type is class and the length is sizeof(CK_OBJECT_CLASS)?

@@ +558,5 @@
> +
> +static SECStatus
> +format_struct_version(char **buf, const char *name, CK_VERSION_PTR version)
> +{
> +    char verbuf[64];

31 bytes to hold a decimal 64bit (8 byte) integer?  Yup that works until we get to 256 bit words.OK it's moot because verison->major and version->minor are bytes.

@@ +585,5 @@
> + * cannot simply pass an allocated string around and expect the caller
> + * to free it. In this case we want to be using PR_smprintf_free() but
> + * that is an implementation detail which we'd do better to hide from
> + * the callers. Just let them call P11URI_FreeString().
> + */

A reasonable adaptation.

@@ +609,5 @@
> +            goto out;
> +    }
> +
> +    if ((uri_type & P11URI_FOR_MODULE_WITH_VERSION) ==
> +        P11URI_FOR_MODULE_WITH_VERSION) {

OK, we only use with_version with Module. If we tried it alone, it wouldn't match here.

@@ +645,5 @@
> +
> +    if ((uri_type & P11URI_FOR_OBJECT) == P11URI_FOR_OBJECT) {
> +        rv = format_attribute_string(&buffer, "id",
> +                                     P11URI_GetAttribute(uri, CKA_ID),
> +                                     PR_TRUE);

Good CKA_ID is always encoded as a number, even if the value happens to match a normal ascii character.

@@ +672,5 @@
> +
> +    if (uri->pin_value) {
> +        rv = format_encode_string(&buffer, "pin-value",
> +                                  (const unsigned char *)uri->pin_value,
> +                                  strlen(uri->pin_value), PR_FALSE);

We probably should clear the URI string on free because we could potentially encode the pin here too. Fortunately there's an interface for that.

@@ +688,5 @@
> +    return buffer;
> +}
> +
> +static char *
> +key_decode(const char *value, const char *end)

OK, This function takes the key (as in key value pair, not a cryptographic key) and removes any imbeded white space. It doesn't select a key out of a string. That description would be a useful comment.

@@ +700,5 @@
> +        PORT_SetError(SEC_ERROR_NO_MEMORY);
> +        return NULL;
> +    }
> +
> +    memcpy(key, value, length);

I've verified below (and included comments at the relevant locations) that there won't be any pin data in the string value to vale + length) so even though we copy the whole thing, we don't need to agressively zero any extra bytes to clear out any potential pin values we may have included.

@@ +717,5 @@
> +}
> +
> +static unsigned char *
> +url_decode(const char *value, const char *end, const char *skip,
> +           size_t *length)

This code contracts the escaped characters and removed any of the (non-escaped) 'skip' characters from the string

@@ +730,5 @@
> +    result = PORT_Alloc((end - value) + 1);
> +    if (result == NULL) {
> +        PORT_SetError(SEC_ERROR_NO_MEMORY);
> +        return NULL;
> +    }

Unlike key_decode, we don't copy the entire url before manipulating it, so there is no chance that a copy of the pin would be in our extra buffer behind the NULL. That means we don't need to zero that part of the buffer before returning. If we fail before returning however we will need to clear the buffer before freeing it.

@@ +743,5 @@
> +        if (*value == '%') {
> +            value++;
> +            if (value + 2 > end) {
> +                PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +                PORT_Free(result);

result could have the pin at this point so we should zero it, at least up to the current pointer p. Same with the free below.

@@ +820,5 @@
> +    if (strcmp("type", name) != 0 && strcmp("objecttype", name) != 0 &&
> +        strcmp("object-type", name) != 0)
> +        return 0;
> +
> +    value = key_decode(start, end);

Hmm, this is using key_decode on the *value*, not the key. Probably because class is encoded as a string value?
This is the only place where we use key_decode on the value, so I think we're fine.

@@ +910,5 @@
> +    return parse_struct_info(where, length, start, end, uri);
> +}
> +
> +static int
> +atoin(const char *start, const char *end)

Since this can come in from the user, we probably should check to make sure it can't overflow the int.

never mind, we check it below, well sort of if we overflow enough we could get back to the range of 0-255. Maybe blow out if we find a we have looped more than 3 times.

@@ +1024,5 @@
> +        PORT_Free(uri->pin_source);
> +        uri->pin_source = (char *)pin_source;
> +        return 1;
> +    } else if (strcmp(name, "pin-value") == 0) {
> +        pin_source = url_decode(start, end, P11_URL_WHITESPACE, NULL);

OK, good. pin-value value is decoded with url_decode, so we don't need to worry about clearing the data copied in key_decode.

@@ +1028,5 @@
> +        pin_source = url_decode(start, end, P11_URL_WHITESPACE, NULL);
> +        if (pin_source == NULL)
> +            return -1;
> +        PORT_Free(uri->pin_value);
> +        uri->pin_value = (char *)pin_source;

uri adopts the space we've alocated. That means we only need to worry about clearing the pin data when we free the uri structure.

@@ +1076,5 @@
> +    uri->module.libraryVersion.minor = (CK_BYTE)-1;
> +    uri->unrecognized = 0;
> +    PORT_Free(uri->pin_source);
> +    uri->pin_source = NULL;
> +    PORT_Free(uri->pin_value);

again, need to zero the pin before freeing.

@@ +1094,5 @@
> +            PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +            return SECFailure;
> +        }
> +
> +        key = key_decode(string, epos);

here's the second place we call key_decode, it doesn't touch the pin, so our copy in key_decode won't have any pin characters in it and we don't need to zero it.

@@ +1130,5 @@
> +}
> +
> +void
> +P11URI_FreeString(char *string)
> +{

OK, here we should zero the URI string because it may contain a pin .

@@ +1142,5 @@
> +        return;
> +
> +    P11URI_ClearAttributes(uri);
> +    PORT_Free(uri->pin_source);
> +    PORT_Free(uri->pin_value);

again, zero the pin before freeing it.

::: lib/p11uri/p11uri.h
@@ +66,5 @@
> +    P11URI_FOR_TOKEN = (1 << 2),
> +    P11URI_FOR_MODULE = (1 << 3),
> +
> +    P11URI_FOR_MODULE_WITH_VERSION =
> +        (1 << 4) | P11URI_FOR_MODULE,

Do we not want to define version? I guess just P11URI_FOR_VERSION doesn't make sense alone.

	Robert Relyea
	Comment 25 • 8 years ago

Comment on attachment 8782929 [details] [diff] [review]
0011-Detect-PKCS-11-URI-in-PK11_FindCert-s-FromNickname-a.patch

Review of attachment 8782929 [details] [diff] [review]:
-----------------------------------------------------------------

This patch requires patch 0010, but since it's short, and the subject of some of the comment discussion, I'm going to way in here and approve the patch. Yes it technically would cause some issue if a token actually had a name of 'pkcs11', all lower case, or a cert in the database was called pkcs11: something. Both are highly unlikely and more over would have to have enough following it to look like a valid uri (including at least one '='), Making the chance that this change would strand a certificate in a database, or in a token highly unlikely.

r+ rrelyea

::: lib/pk11wrap/pk11cert.c
@@ +718,4 @@
>      char *delimit = NULL;
>      char *tokenName;
>  
> +    if (!strncmp(nickname, "pkcs11:", 7)) {

random style issue. I personally trust the compiler count better than my own, so I tend to do things like

if (!strncmp(nickanme, PK11URI_SCHEME, sizeof(PK11URI_SCHEME)-1)

and include the pkuri.h. You'd have to move the PKIURI_SCHEME define to that header file as well.

Not a requirement, just a suggestion.

	Robert Relyea
	Comment 26 • 8 years ago

> I should explicitly point out the "bug fix" that was different.... the FindCert version called
> pk11_AuthenticateUnfriendly() while the FindCerts version did not. Bob, ISTR you added that... it
> wasn't *intentionally* omitted from the latter, was it?

I honestly don't remember. It has the semantics of providing a function to get certs that won't generate password prompts (at the cost of not seeing all the certs).

	Robert Relyea
	Comment 27 • 8 years ago

> Right. I actually got half way through adding 'const' to a bunch of them, before coming to the same 
> realisation and backing it out... :)

I might have been tempted to explicity add 'set' functions so the get functions could be 'const'

	Robert Relyea
	Comment 28 • 8 years ago

> Hm, I don't much like that because it makes it *less* obvious, in the later code, 
> that this is actually going into slot->

I think this makes sense for the C_GetTokenInfo call. For all the rest of the reverences, though it just adds more unnecessary text to me. It's fine to keep (in the old days I would have required the change so the compiler didn't keep walking down the pointer chain every time, but compilers are smart these days so the issue is one of style).

bob

	David Woodhouse Assignee
	Comment 29 • 8 years ago

(In reply to Robert Relyea from comment #27)
> I might have been tempted to explicity add 'set' functions so the get
> functions could be 'const'

Yeah, although 'set' functions would be a memcpy while as it is, we can C_GetTokenInfo() directly into the right place. So that's probably why Stef did p11-kit this way in the first place. I'm not sure it it's worth changing it now.

(In reply to Robert Relyea from comment #26)
> > I should explicitly point out the "bug fix" that was different.... 
> 
> I honestly don't remember. It has the semantics of providing a function to
> get certs that won't generate password prompts (at the cost of not seeing
> all the certs).

Yeah. And my "cleanup" patch changes those semantics. I should probably add the pk11_AuthenticateUnfriendly() call explicitly in a first patch to make the semantics match the FindCert variant, and *then* apply the cleanup which will then have no functional effect.

	Robert Relyea
	Comment 30 • 8 years ago

> I'm not sure it it's worth changing it now.

Agreed. It's also best to maintain close to the same API as we get on openssl unless there's good reason to vary that.

> I should probably add the pk11_AuthenticateUnfriendly() call explicitly in a first patch to make 
> the semantics match the FindCert variant, and *then* apply the cleanup which will then have no
> functional effect.

I think this is safest.

bob

	Robert Relyea
	Comment 31 • 8 years ago

Comment on attachment 8782927 [details] [diff] [review]
0010-Add-PK11_FindCertsFromURI-and-PK11_FindCertFromURI.patch

Review of attachment 8782927 [details] [diff] [review]:
-----------------------------------------------------------------

There isn't a bug here, but there is one thing that could lead to future bugs if we aren't careful. 

We are freeing the cert references in a function other than the one that acquired them. It looks like we are freeing references given to us by nssList_getArray(). The nssList functions do not know about references to anything returned from them or any of there free functions will not modify the references at all. The trust domain cache functions do know about references, so they return lists with full references. The cert references need to be freed from the certlist before we free the certlist. In this code The referenc freeing happens in transfer_uri_certs_to_collection(), so we don't actually leak, but we are returning with a certlist full a certs we no longer hold a reference to.

bob

::: lib/pk11wrap/pk11cert.c
@@ +512,5 @@
> +    NSSCertificate **certs;
> +    PRUint32 i, count;
> +    NSSToken **tokens, **tp;
> +    PK11SlotInfo *slot;
> +    CK_ATTRIBUTE_PTR id_attr;

even though the API doesn't return const, we probably should declare this variable const so we know we shouldn't tweak with the result returned to us from the P11URI interface.

@@ +523,5 @@
> +    certs = nss_ZNEWARRAY(NULL, NSSCertificate *, count);
> +    if (!certs) {
> +	return;
> +    }
> +    nssList_GetArray(certList, (void **)certs, count);

certs do not carry references

@@ +538,5 @@
> +	    for (tp = tokens; *tp; tp++) {
> +		slot = (*tp)->pk11slot;
> +		if (P11URI_MatchTokenInfo(URI, &(slot->tokenInfo)) == 1) {
> +		    nssPKIObjectCollection_AddObject(collection,
> +						     (nssPKIObject *)certs[i]);

This get's it's own reference.

@@ +544,5 @@
> +		}
> +	    }
> +	    nssTokenArray_Destroy(tokens);
> +	}
> +	CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(certs[i]));

This destroys the reference in the certlist, since The array didn't get any references.
Note that nssList_Destroy() does not free these references, so there isn't an overall leak, but the reason may be confusing. It may be better not free the certs here, but walk down the certList in find_cert_from_uri() when we want to free the certlist.

@@ +555,5 @@
> +{
> +    P11URI *p11Uri;
> +    CK_ATTRIBUTE_PTR attributes;
> +    CK_ATTRIBUTE_PTR finalattributes;
> +    CK_ATTRIBUTE_PTR label;

as per the comment above, attributes and label should be const (finalattributes are our local copy, so obviously it shouldn't be const).

@@ +602,5 @@
> +	(void)nssTrustDomain_GetCertsForNicknameFromCache(defaultTD,
> +							  (const char *)label->pValue,
> +							  certList);
> +    } else {
> +	(void)nssTrustDomain_GetCertsFromCache(defaultTD, certList);

This returns a certlist with references to each cert.

@@ +605,5 @@
> +    } else {
> +	(void)nssTrustDomain_GetCertsFromCache(defaultTD, certList);
> +    }
> +
> +    transfer_uri_certs_to_collection(certList, p11Uri, collection);

We could free the certList here. The collection has it's own references to the certs. Note: transer_uri_certs_to_collection currently frees the certlist references, so there isn't a leak by freeing the certList below using nssList_Destroy().

If we fix the free I suggest above, then we need to free the cert objects in the certlist before we free it.

@@ +652,5 @@
> +
> +CERTCertificate *
> +PK11_FindCertFromURI(const char *uri, void *wincx)
> +{
> +    static const NSSUsage usage = {PR_TRUE /* ... */ };

We should probably pass the usage in. See common_FindCertByNicknameOrEmailAddrForUsage().
We would pass in a SECCertUsage().

	David Woodhouse Assignee
	Comment 32 • 8 years ago

OK, updated series at https://github.com/varunnaganathan/nss/tree/20160902
This attachment is an incremental patch which applies on top of the above, for ease of continued review. The git tree has the individual patches fixed according to the review comments.

I think I've caught everything. The main thing I *haven't* done is comment 31. The refcounting here is strange, I agree, but it is entirely modelled on the existing code in PK11_FindCert{,s}FromNickname() which behaves in precisely the same way.

You'll note that the first two (now) patches in this series are *already* fixing up the existing PK11_FindCert{,s}FromNickname() code before we copy it, because I neither wanted to copy it as-was, nor be inconsistent with it. I have no *particular* objection to embarking on a further cleanup and making it three. But on the other than I'd quite like to merge it and then if you really care about fixing up a code flow that's been present for decades we can fix them both at once :)

You also mention passing in a SECCertUsage()... again, this was a strict parallel for PK11_FindCertFromNickname(), and we even call it directly *from* PK11_FindCertFromNickname() when the "nickname" looks like a PKCS#11 URI. So I'm not keen to break the parallel. However, we should continue with the process of adding URI functionality (as Varun is doing in his own branch pending my review). And 'FindCertByURIForUsage()' would probably make sense to add. It's just that PK11_FindCertFromURI() was the most important one to do first.

	Kai Engert (:KaiE:)
	Comment 33 • 8 years ago

The most recent patch is waiting for Bob's review.

Patch 1 to 11, which have already been reviewed, have been checked in:
(See changeset https://hg.mozilla.org/projects/nss/rev/47868a8399fd and the 10 changesets before it.)

	Kai Engert (:KaiE:)
	Comment 34 • 8 years ago

(In reply to Kai Engert (:kaie) from comment #33)
> The most recent patch is waiting for Bob's review.
> 
> Patch 1 to 11, which have already been reviewed, have been checked in:
> (See changeset https://hg.mozilla.org/projects/nss/rev/47868a8399fd and the
> 10 changesets before it.)

The patches have been backed, because:
- the nss.def file has been changed incorrectly
  (must identify the NSS version number in which a function is exported for the first time)

In addition, it's strongly encouraged to add tests, prior to landing again.

(See https://hg.mozilla.org/projects/nss/rev/c32a5490a47f and the previous 10 commits.)

	Kai Engert (:KaiE:)
	Comment 35 • 8 years ago

Assigning to David, because he is working on it.

	David Woodhouse Assignee
	Comment 36 • 8 years ago

Fix clang-format bustage in lib/pki

This got formatted since we touched it, so now triggers a complaint. Other directories still look like they haven't been done yet (lib/pk11wrap, lib/pki).

Is there a local test I should be running which would show this problem?

	David Woodhouse Assignee
	Comment 37 • 8 years ago

Oops, I hadn't noticed that this was a disguised linker script. This should fix it.

	David Woodhouse Assignee
	Comment 38 • 8 years ago

(In reply to Kai Engert (:kaie) from comment #34)
> (In reply to Kai Engert (:kaie) from comment #33)
> > The most recent patch is waiting for Bob's review.
> > 
> > Patch 1 to 11, which have already been reviewed, have been checked in:
> > (See changeset https://hg.mozilla.org/projects/nss/rev/47868a8399fd and the
> > 10 changesets before it.)
> 
> The patches have been backed, because:
> - the nss.def file has been changed incorrectly
>   (must identify the NSS version number in which a function is exported for
> the first time)

Apologies for not catching that. I've made it 3.27; does that seem sane?

> In addition, it's strongly encouraged to add tests, prior to landing again.

Indeed; I'll work on those but it'll take a day or so. Thanks for looking at this.

	Kai Engert (:KaiE:)
	Comment 39 • 8 years ago

Comment on attachment 8794867 [details] [diff] [review]
0013-Fix-clang-format-bustage.patch

r=kaie on this whitespace-only change

	Kai Engert (:KaiE:)
	Comment 40 • 8 years ago

Comment on attachment 8794868 [details] [diff] [review]
0014-Fix-new-exports-in-nss.def.patch

NSS 3.27 is already completed, please change the version to 3.28

	David Woodhouse Assignee
	Comment 41 • 8 years ago

> NSS 3.27 is already completed, please change the version to 3.28

Fixed; thanks.

	Robert Relyea
	Comment 42 • 8 years ago

Comment on attachment 8787785 [details] [diff] [review]
bug1162897-20160819-20160902.patch

Review of attachment 8787785 [details] [diff] [review]:
-----------------------------------------------------------------

r+

	Martin Thomson [:mt:]
	Comment 43 • 8 years ago

Just dropping a note in here as a record of the discussion in IRC: this was backed out.  As a condition of landing, it needs reasonable test coverage.  As a new feature, there are probably two other things to consider:

 1. I tend to think that it probably needs some documentation as well - the new functions in lib/pk11wrap/pk11pub.h don't even have comments.
 2. The command line utilities could be updated with new options that use these new functions.  I don't know which is most appropriate, but it seems like certutil is probably the one to look at.  Adding code here also serves as a demonstration of use, which is nice in support of documentation.  It could help with testing.

David, if you need access to the try servers, I'm sure that any one of the team would be happy to coordinate with you.

	David Woodhouse Assignee
	Comment 44 • 8 years ago

Turns out that assert I added at Bob's request in NSSTrustDomain_FindTokensByURI() had an off-by-one error, which triggers when you do a lookup by with a URI that matches *all* tokens (by not having any token-specific information in it at all). Fix that.

Should I really be doing this as incremental commits, and not going back to retroactively fix the originals?

	David Woodhouse Assignee
	Comment 45 • 8 years ago

This exercises the raison d'être of all this code: check that 'tstclnt' will happily accept PKCS#11 URIs in place of nicknames. Two forms of the test — one specifying the token, and one just the label.

	David Woodhouse Assignee
	Comment 46 • 8 years ago

(In reply to Martin Thomson [:mt:] from comment #43)
>  1. I tend to think that it probably needs some documentation as well - the
> new functions in lib/pk11wrap/pk11pub.h don't even have comments.

In our defence, the *old* functions in pk11pub.h don't have comments either.
If one already knows what PK11_FindCertsFromNickname() and PK11_FindCertsFromEmailAddress() are going to do (there's a clue in the name), then it might be reasonably expected that one can also work out what PK11_FindCertsFromURI() is going to do... :)

Less flippantly... what manner of comment would you like to see there? We have a bunch of functions which are precise parallels of existing functions that take/give names, but which take/give URIs instead. Would it be sufficient to add a one-line comment to each which says that it takes/gives a PKCS#11 URI according to RFC7512?

I suppose for PK11_Get{Token,Module}URI() we do at least want a comment that the returned string needs to be freed with P11URI_FreeString(). 

>  2. The command line utilities could be updated with new options that use
> these new functions.  I don't know which is most appropriate, but it seems
> like certutil is probably the one to look at.  Adding code here also serves
> as a demonstration of use, which is nice in support of documentation.  It
> could help with testing.

Yes. Actually it's already working in many cases; we deliberately made PK11_FindCert{s,}FromNickname() fall back to doing the lookup by URI if the "nickname" string actually is a URI. This means that *various* tools will automatically Just Work. I'd been doing my manual testing with curl, but the patch in comment #45 exercises the same in tstclnt, in the SSL tests.

There's a little more we can do in certutil to make it fully RFC7512-literate, especially in the way it handles the list of tokens it wants to operate on (including the support for "all"). Varun was working on that, but seems to have disappeared at the end of GSoC. I *believe* he'll be back after a bit of a rest, but I'll also take a look at it myself.

For now though, the main purpose of all this work is already being tested through the tstclnt tests. Fedora packaging guidelines (as well as common sense and decency) require that when I plug in my Yubikey, I should be able to use the certificate therein by handing 'pkcs11:manufacturer=piv_II;id=%01' to *any* application which would accept a filename for a cert, and it should Just Work™. And with these patches — and the fixes in bug 1296263 so the correct tokens actually get loaded — that's now working.

> David, if you need access to the try servers, I'm sure that any one of the
> team would be happy to coordinate with you.

Thanks.

	Eric Rescorla (:ekr)
	Comment 47 • 8 years ago

Comment on attachment 8795226 [details] [diff] [review]
0016-Tweak-SSL-tests-to-exercise-PKCS-11-URI-matching.patch

Review of attachment 8795226 [details] [diff] [review]:
-----------------------------------------------------------------

Thsi seems like pretty incomplete test coverage. What happens if you provide bogus URIs of various stripes, for instance?
What if you have two keys with similar URIs, does the right one get selected, etc.?

	Eric Rescorla (:ekr)
	Comment 48 • 8 years ago

(In reply to David Woodhouse from comment #46)
> (In reply to Martin Thomson [:mt:] from comment #43)
> >  1. I tend to think that it probably needs some documentation as well - the
> > new functions in lib/pk11wrap/pk11pub.h don't even have comments.
> 
> In our defence, the *old* functions in pk11pub.h don't have comments either.
> If one already knows what PK11_FindCertsFromNickname() and
> PK11_FindCertsFromEmailAddress() are going to do (there's a clue in the
> name), then it might be reasonably expected that one can also work out what
> PK11_FindCertsFromURI() is going to do... :)

It's true that that's not great. Perhaps you could supply comments for those
functions as well.


> Less flippantly... what manner of comment would you like to see there? We
> have a bunch of functions which are precise parallels of existing functions
> that take/give names, but which take/give URIs instead. Would it be
> sufficient to add a one-line comment to each which says that it takes/gives
> a PKCS#11 URI according to RFC7512?
> 
> I suppose for PK11_Get{Token,Module}URI() we do at least want a comment that
> the returned string needs to be freed with P11URI_FreeString(). 
> 
> >  2. The command line utilities could be updated with new options that use
> > these new functions.  I don't know which is most appropriate, but it seems
> > like certutil is probably the one to look at.  Adding code here also serves
> > as a demonstration of use, which is nice in support of documentation.  It
> > could help with testing.
> 
> Yes. Actually it's already working in many cases; we deliberately made
> PK11_FindCert{s,}FromNickname() fall back to doing the lookup by URI if the
> "nickname" string actually is a URI. This means that *various* tools will
> automatically Just Work. I'd been doing my manual testing with curl, but the
> patch in comment #45 exercises the same in tstclnt, in the SSL tests.
> 
> There's a little more we can do in certutil to make it fully
> RFC7512-literate, especially in the way it handles the list of tokens it
> wants to operate on (including the support for "all"). Varun was working on
> that, but seems to have disappeared at the end of GSoC. I *believe* he'll be
> back after a bit of a rest, but I'll also take a look at it myself.
> 
> For now though, the main purpose of all this work is already being tested
> through the tstclnt tests.

Per c47, I think you probably need some more test coverage there. Also, really you should
write some tests that are much closer to the actual token, not dependent on the libssl
tests.


> Fedora packaging guidelines (as well as common
> sense and decency) require that when I plug in my Yubikey, I should be able
> to use the certificate therein by handing
> 'pkcs11:manufacturer=piv_II;id=%01' to *any* application which would accept
> a filename for a cert, and it should Just Work™. And with these patches —
> and the fixes in bug 1296263 so the correct tokens actually get loaded —
> that's now working.
> 
> > David, if you need access to the try servers, I'm sure that any one of the
> > team would be happy to coordinate with you.
> 
> Thanks.

	David Woodhouse Assignee
	Comment 49 • 8 years ago

(In reply to Eric Rescorla (:ekr) from comment #47)
> Thsi seems like pretty incomplete test coverage. What happens if you provide
> bogus URIs of various stripes, for instance?

Yes, this isn't complete test coverage; I'm working on more. Just as soon as I've finished diagnosing the failure in the existing one, which showed up once I expanded my test invocation back to running all.sh instead of just ssl/ssl.sh. Turns out the token changes its name in FIPS mode, so it stops finding the cert.

At this point, access to the try servers sounds like it would be a very good idea... please.

> What if you have two keys with similar URIs, does the right one get
> selected, etc.?

The URI is just a search term (URI is kind of a misnomer, qv). If you pass a URI which matches more than one cert, there is no "right one". The difference between PK11_FindCertFromURI() and PK11_FindCertsFromURI() is precisely the same as between the similarly-named nickname-based functions: one returns the full list, while the other chooses the "best one".

	David Woodhouse Assignee
	Comment 50 • 8 years ago

SSL tests fixed to work in FIPS mode too, where the token name differs.

	Eric Rescorla (:ekr)
	Comment 51 • 8 years ago

dwmw2: here is how you get try access:
https://www.mozilla.org/en-US/about/governance/policies/commit/

	Robert Relyea
	Comment 52 • 8 years ago

Comment on attachment 8795224 [details] [diff] [review]
0015-Fix-assertion-off-by-one-in-NSSTrustDomain_FindToken.patch

Review of attachment 8795224 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rre;uea

	Robert Relyea
	Comment 53 • 8 years ago

Comment on attachment 8795360 [details] [diff] [review]
0016-Tweak-SSL-tests-to-exercise-PKCS-11-URI-matching.patch

Review of attachment 8795360 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea

	Kai Engert (:KaiE:)
	Comment 54 • 7 years ago

I've been asked to summarize the status here.

It seems that David has provided tests, so we could try to land again, is that correct?

If we had a full set of patches, I could help out, and run an experimental "try" build for you.

However, it seems the patches have bitrotted. The first patch in the series doesn't apply any more. there are conflicts.

So, before we can proceed, someone needs to merge everything to trunk.

We shouldn't have idled almost 4 months since the last patches were provided.

	David Woodhouse Assignee
	Comment 55 • 7 years ago

Apologies, that's partly my fault. As you say, I've provided tests... but they are a bare minimum. I meant to provide more complete test coverage and then push for it all to be merged, but got kind of distracted with new job. I will endeavour to at least update Varun's patches to current HEAD, in the next few weeks unless someone (Varun?) beats me to it.

	Daiki Ueno [:ueno]
	Comment 56 • 7 years ago

I have tried to rebase the patches against the trunk.  The try build is now running at:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=04ee623220c78b4b7b24498fc93dc17f91753e99
(in the previous run, only a minor memory leak is detected, so this time it should be fine).

Aside from the rebasing, I replaced the PKCS#11 URI parser code in comment 6, which is based on p11-kit, with a minimal implementation suitable for the use in NSS.  The test coverage still might not be sufficient, but I think it would be easier to write tests against the new parser code.

I will attach the patches below.

	Daiki Ueno [:ueno]
	Comment 57 • 7 years ago

This obsoletes attachment 8782913 [details] [diff] [review].

	Daiki Ueno [:ueno]
	Comment 59 • 7 years ago

This obsoletes attachment 8782904 [details] [diff] [review].  Changes are rebase only.

	Daiki Ueno [:ueno]
	Comment 60 • 7 years ago

This obsoletes attachment 8782907 [details] [diff] [review].  Changes are rebase only.

	Daiki Ueno [:ueno]
	Comment 61 • 7 years ago

This obsoletes attachment 8782916 [details] [diff] [review].  Changes are rebase only.

	Daiki Ueno [:ueno]
	Comment 62 • 7 years ago

This obsoletes attachment 8782921 [details] [diff] [review].  This is adjusted to use the new parser API.

	Daiki Ueno [:ueno]
	Comment 63 • 7 years ago

This obsoletes attachment 8782922 [details] [diff] [review].  This is adjusted to use the new parser API.

	Daiki Ueno [:ueno]
	Comment 64 • 7 years ago

This obsoletes attachment 8782923 [details] [diff] [review].  Rebase only, except s/free/PORT_Free/.

	Daiki Ueno [:ueno]
	Comment 65 • 7 years ago

This obsoletes attachment 8782925 [details] [diff] [review].  Rebase only.

	Daiki Ueno [:ueno]
	Comment 66 • 7 years ago

This obsoletes attachment 8782926 [details] [diff] [review].  Adjusted to use the new parser API.

	Daiki Ueno [:ueno]
	Comment 67 • 7 years ago

This obsoletes attachment 8782927 [details] [diff] [review].  Adjusted to use the new parser API.

	Daiki Ueno [:ueno]
	Comment 68 • 7 years ago

This obsoletes attachment 8782929 [details] [diff] [review].  Rebase only.

	Daiki Ueno [:ueno]
	Comment 69 • 7 years ago

This obsoletes attachment 8795360 [details] [diff] [review].  Rebase only.

	Daiki Ueno [:ueno]
	Comment 70 • 7 years ago

That's all so far.  The following bustage fixes shouldn't be necessary:
attachment 8794867 [details] [diff] [review]
attachment 8794872 [details] [diff] [review]
attachment 8795224 [details] [diff] [review]

	Robert Relyea
	Comment 71 • 7 years ago

Comment on attachment 8864874 [details] [diff] [review]
0001-util-add-minimal-PKCS-11-URI-parser.patch

Review of attachment 8864874 [details] [diff] [review]:
-----------------------------------------------------------------

r- for 2 issues:

1) we aren't always validating that the named attributes only have valid characters in them. In particular, vendor attributes aren't checked (others are implicitly checked since they match known attributes. This could lead to cross scripting attacks. This one needs to be fixed.
2) It appears we are not handling vendor attributes correctly. Our comparison function will always indicate that vendor attributes match. This can be fixed in the underlying comparison function. This probably should be fixed since we seem to have a test case for it.

In addition, please fixed the miszpelling of 'available'

All other comments are bike shedding, and need not be fixed.

::: gtests/util_gtest/util_pkcs11uri_unittest.cc
@@ +95,5 @@
> +                  "pkcs11:token=aaa;manufacturer=bbb");
> +  TestParseFormat("pkcs11:manufacturer=bbb;token=aaa",
> +                  "pkcs11:token=aaa;manufacturer=bbb");
> +  TestParseFormat("pkcs11:manufacturer=bbb;token=aaa;vendor2=ddd;vendor1=ccc",
> +                  "pkcs11:token=aaa;manufacturer=bbb;vendor1=ccc;vendor2=ddd");

Hmm, this test will fail in the current code.

::: lib/util/pkcs11uri.c
@@ +133,5 @@
> +
> +/* URI encoding functions. */
> +static char *
> +pk11uri_Escape(PLArenaPool *arena, const char *value, size_t length,
> +               const char *availale)

available is misspelled. This tells us if we need to encode the character as %xx format or not.

@@ +153,5 @@
> +            if (ret != SECSuccess) {
> +                goto fail;
> +            }
> +        } else {
> +            buf[0] = *p;

This is fine, but unnecessary. You could just cast p to unsigned const char and pass it in. No big deal and you do have to do this form the const '\0' below so you can keep it. Just struck me as odd.

@@ +167,5 @@
> +        goto fail;
> +    }
> +
> +    result = buffer.data;
> +    buffer.data = NULL;

Probably should also zero out buffer.allocate and buffer.len.
what you have here is sufficient for the existing pk11uri_DestroyBuffer(), and buffer then goes out of scope, but it does leave buffer in an inconsistent state should it every attempted to be used again.

Not a bug.

@@ +224,5 @@
> +        goto fail;
> +    }
> +
> +    result = buffer.data;
> +    buffer.data = NULL;

Same comment above. If you change he above, change here as well. Otherwise you can just leave them.

@@ +232,5 @@
> +
> +    return result;
> +}
> +
> +/* Functions for manipulating attributes array. */

NOTE: unknown names are greater than any known names. Two different unknown names will compare as equal (0).

@@ +265,5 @@
> +pk11uri_CompareQueryAttributeName(const char *a, const char *b)
> +{
> +    return pk11uri_CompareByPosition(a, b, qattr_names, PR_ARRAY_SIZE(qattr_names));
> +}
> +

The following function does not check the validity of name. If the caller can accept invalid names, then it's possible it will queue up invalid attributes in the list.
NOTE: this function adopts name and value on success.

@@ +386,5 @@
> +        unsigned char sep[1];
> +        char *escaped;
> +        PK11URIAttributeListEntry *attr = &attrs->attrs[i];
> +
> +        if (!is_first) {

or (i != 0 )?

@@ +495,5 @@
> +            }
> +        }
> +
> +        if (j == num_attr_names) {
> +            ret = pk11uri_InsertAttributeList(dest_vattrs,

OK, where we are inserting vendor allowed (unknown names). if vendoar_allow_duplicate is FALSE, we will only allow 1 vendor specific name here. (NOTE: we haven't checked if name has any forbidden values.).

@@ +621,5 @@
> +                break;
> +            }
> +        }
> +        if (i == num_attr_names) {
> +            ret = pk11uri_InsertAttributeListEscaped(vattrs,

again we are inserting names that haven't been validated.

	Robert Relyea
	Comment 72 • 7 years ago

Comment on attachment 8864875 [details] [diff] [review]
0002-pk11wrap-add-pk11_MatchString.patch

Review of attachment 8864875 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pk11wrap/pk11slot.c
@@ +1077,4 @@
>  }
>  
>  /*
> + * check if a null-terminatd string matches with a PKCS11 Static Label

We don't already have a function the does this?

	Robert Relyea
	Comment 73 • 7 years ago

Comment on attachment 8864879 [details] [diff] [review]
0003-Remove-duplication-in-PK11_FindCert-s-FromNickname.patch

Review of attachment 8864879 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea

	Robert Relyea
	Comment 74 • 7 years ago

Comment on attachment 8864885 [details] [diff] [review]
0007-Add-PK11_GetModuleURI-function.patch

Review of attachment 8864885 [details] [diff] [review]:
-----------------------------------------------------------------

Fix bug where libraryManufacturer is set rather than description.

::: lib/pk11wrap/pk11util.c
@@ +616,5 @@
> +    PK11_MakeString(NULL, libraryDescription, (char *)info.libraryDescription,
> +                    sizeof(info.libraryDescription));
> +    if (*libraryDescription != '\0') {
> +        attrs[nattrs].name = PK11URI_PATTR_LIBRARY_DESCRIPTION;
> +        attrs[nattrs].value = libraryManufacturer;

value should be libraryDescription.

	Robert Relyea
	Comment 75 • 7 years ago

Comment on attachment 8864892 [details] [diff] [review]
0011-Add-PK11_FindCertsFromURI-and-PK11_FindCertFromURI.patch

Review of attachment 8864892 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/nss/nss.def
@@ +461,5 @@
>  PK11_DEREncodePublicKey;
>  PK11_ExtractKeyValue;
>  PK11_FindCertsFromNickname;
> +PK11_FindCertFromURI;
> +PK11_FindCertsFromURI;

Be sure to add these to the proper versioned section of nss.def. (Same with all the .def file changes.

	Daiki Ueno [:ueno]
	Comment 76 • 7 years ago

Thank you for the review.  I am updating patches which should address the issues.

(In reply to Robert Relyea from comment #71)

> 1) we aren't always validating that the named attributes only have valid
> characters in them. In particular, vendor attributes aren't checked (others
> are implicitly checked since they match known attributes. This could lead to
> cross scripting attacks. This one needs to be fixed.

There are actually two distinct code paths: one is for parsing and the other is for creating a URI object from attribute arrays.  The latter didn't have any validation on the attribute names, so I added one now.

> 2) It appears we are not handling vendor attributes correctly. Our
> comparison function will always indicate that vendor attributes match. This
> can be fixed in the underlying comparison function. This probably should be
> fixed since we seem to have a test case for it.

Sorry, I don't get it.  The comparison functions are for reordering the attributes so that the output of PK11URI_Format() be reproducible.  For vendor attributes, they are sorted alphabetically with strcmp().

> In addition, please fixed the miszpelling of 'available'

Fixed now.

> All other comments are bike shedding, and need not be fixed.
> 
> ::: gtests/util_gtest/util_pkcs11uri_unittest.cc
> @@ +95,5 @@
> > +                  "pkcs11:token=aaa;manufacturer=bbb");
> > +  TestParseFormat("pkcs11:manufacturer=bbb;token=aaa",
> > +                  "pkcs11:token=aaa;manufacturer=bbb");
> > +  TestParseFormat("pkcs11:manufacturer=bbb;token=aaa;vendor2=ddd;vendor1=ccc",
> > +                  "pkcs11:token=aaa;manufacturer=bbb;vendor1=ccc;vendor2=ddd");
> 
> Hmm, this test will fail in the current code.

It actually runs in the nss-try build and succeeds, so I am not sure how it could fail.

> ::: lib/util/pkcs11uri.c
> @@ +133,5 @@
> > +
> > +/* URI encoding functions. */
> > +static char *
> > +pk11uri_Escape(PLArenaPool *arena, const char *value, size_t length,
> > +               const char *availale)
> 
> available is misspelled. This tells us if we need to encode the character as
> %xx format or not.

Fixed.

> @@ +153,5 @@
> > +            if (ret != SECSuccess) {
> > +                goto fail;
> > +            }
> > +        } else {
> > +            buf[0] = *p;
> 
> This is fine, but unnecessary. You could just cast p to unsigned const char
> and pass it in. No big deal and you do have to do this form the const '\0'
> below so you can keep it. Just struck me as odd.

Yes, that is simpler.  Fixed.

> > +/* Functions for manipulating attributes array. */
> 
> NOTE: unknown names are greater than any known names. Two different unknown
> names will compare as equal (0).

Actually, the code assumes that p11uri_CompareByPosition() is always called for known attributes.  I have added an assert there.

> > +        if (j == num_attr_names) {
> > +            ret = pk11uri_InsertAttributeList(dest_vattrs,
> 
> OK, where we are inserting vendor allowed (unknown names). if
> vendoar_allow_duplicate is FALSE, we will only allow 1 vendor specific name
> here. (NOTE: we haven't checked if name has any forbidden values.).

Yes, as defined in RFC7512, only vendor query attributes can have duplicated names.

> @@ +621,5 @@
> > +                break;
> > +            }
> > +        }
> > +        if (i == num_attr_names) {
> > +            ret = pk11uri_InsertAttributeListEscaped(vattrs,
> 
> again we are inserting names that haven't been validated.

Fixed.

	Robert Relyea
	Comment 77 • 7 years ago

Comment on attachment 8867231 [details] [diff] [review]
0001-util-add-minimal-PKCS-11-URI-parser.patch

Review of attachment 8867231 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I see where I was confused below. The compare is fine.

::: lib/util/pkcs11uri.c
@@ +489,5 @@
> +        for (; *p != '\0'; p++) {
> +            if (strchr(PK11URI_ATTR_NM_CHAR, *p) == NULL) {
> +                return SECFailure;
> +            }
> +        }

This is fine, but you only really need to do this for the vendor attribute case.

@@ +520,5 @@
> +            /* Vendor attribute. */
> +            ret = pk11uri_InsertToAttributeList(dest_vattrs,
> +                                                name, value,
> +                                                strcmp,
> +                                                vendor_allow_duplicate);

OH, this is what I missed. You are changed the comparison function and you added the values to a different list. That means this function is not inserting vendor specific attributes into the generic list, so the compare above should never encounter a non-standard attribute.

@@ +650,5 @@
> +            /* Vendor attribute. */
> +            ret = pk11uri_InsertToAttributeListEscaped(vattrs,
> +                                                       name_start, name_length,
> +                                                       value_start, value_length,
> +                                                       strcmp,

And this is the parsing case, putting the attributes in a second list sorted with strcmp instead of compare_name.

@@ +688,5 @@
> +    ret = pk11uri_ParseAttributes(&p, "?", ';', PK11URI_PCHAR,
> +                                  pattr_names, PR_ARRAY_SIZE(pattr_names),
> +                                  &result->pattrs, &result->vpattrs,
> +                                  pk11uri_ComparePathAttributeName,
> +                                  PR_FALSE, PR_FALSE);

So according to your comments, the RFC allows only one vendor specific attribute in the query, but it's the path that seems more restrictive.

@@ +700,5 @@
> +        ret = pk11uri_ParseAttributes(&p, "", '&', PK11URI_QCHAR,
> +                                      qattr_names, PR_ARRAY_SIZE(qattr_names),
> +                                      &result->qattrs, &result->vqattrs,
> +                                      pk11uri_CompareQueryAttributeName,
> +                                      PR_FALSE, PR_TRUE);

Here we are allowing more than one vendor query attribute (actually we allow more than one vendor attribute of the same name as well).

	Robert Relyea
	Comment 78 • 7 years ago

> Sorry, I don't get it.  The comparison functions are for reordering the attributes so that the output of PK11URI_Format() be
>  reproducible.  For vendor attributes, they are sorted alphabetically with strcmp().

yeah, my bad I missed that the vendor attributes and the regular attributes are actually on two separate lists.

> Yes, as defined in RFC7512, only vendor query attributes can have duplicated names.
Ignore my comment above. I misread this as 'only one vendor query attribute', not that only query attributes can have duplicate names.  The code and our comment matches.

> There are actually two distinct code paths: one is for parsing and the other is for creating a URI object from attribute arrays. 
> The latter didn't have any validation on the attribute names, so I added one now.

I think the parsing case is already validating the names in order to actually parse the line correctly.

	Robert Relyea
	Comment 79 • 7 years ago

Comment on attachment 8864881 [details] [diff] [review]
0004-Make-NSSTrustDomain_FindTokenByName-take-a-ref-on-th.patch

Review of attachment 8864881 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it took so long, I wanted to review this in context.

	Robert Relyea
	Comment 80 • 7 years ago

OK, you are one copy and paste error from having everything ready to go!

bob

	Daiki Ueno [:ueno]
	Comment 81 • 7 years ago

Typo fix in the comment: s/null-terminatd/null-terminated/.

	Daiki Ueno [:ueno]
	Comment 82 • 7 years ago

Moved the new symbol under the NSS_3.31 section in nss.def, as suggested in comment 75.

	Daiki Ueno [:ueno]
	Comment 83 • 7 years ago

Fixed the copy-and-paste error (comment 74) and moved the new symbol under the NSS_3.31 section (comment 75).

	Daiki Ueno [:ueno]
	Comment 84 • 7 years ago

Moved the new symbols under the NSS_3.31 section as suggested in comment 75.

	Daiki Ueno [:ueno]
	Comment 85 • 7 years ago

Thank you for the thorough review, Bob.

I have updated the remaining patches with typo and symbol versioning fixes, and rerun the nss-try build:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=83acb83133675203f49f8779edca3bfe856ff92e

Kai, could you merge those patches?

	Kai Engert (:KaiE:)
	Comment 86 • 7 years ago

(In reply to Daiki Ueno [:ueno] from comment #85)
> 
> Kai, could you merge those patches?

Sure. There are many patches, and I see some duplicate patch numbers. Could you please mark all patches, which are no longer up to date, and which shouldn't get checked in, as obsolete?

	Daiki Ueno [:ueno]
	Comment 87 • 7 years ago

Sorry, I don't have privilege to change the status of patches submitted by other person.

Could you pick the patches submitted by myself and not marked as obsolete?  Other patches shouldn't be necessary anymore.

	Kai Engert (:KaiE:)
	Comment 88 • 7 years ago

(In reply to Daiki Ueno [:ueno] from comment #87)
> I don't have privilege to change the status of patches submitted by
> other person.

Maybe it's time to follow this process and ask for more permissions?
  https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html


> Could you pick the patches submitted by myself and not marked as obsolete? 
> Other patches shouldn't be necessary anymore.

Ok, will try that.

	Kai Engert (:KaiE:)
	Comment 89 • 7 years ago

13 patches from Daiki checked in:

https://hg.mozilla.org/projects/nss/rev/c4aa6d8bf3c7
https://hg.mozilla.org/projects/nss/rev/c2debc2d60a3
https://hg.mozilla.org/projects/nss/rev/715d0cc07543
https://hg.mozilla.org/projects/nss/rev/497ec29ce188
https://hg.mozilla.org/projects/nss/rev/d0f047a649fb
https://hg.mozilla.org/projects/nss/rev/3a70b3c599de
https://hg.mozilla.org/projects/nss/rev/3719828a9ea8
https://hg.mozilla.org/projects/nss/rev/7547568f0ae7
https://hg.mozilla.org/projects/nss/rev/e987f7429687
https://hg.mozilla.org/projects/nss/rev/b9f6998af978
https://hg.mozilla.org/projects/nss/rev/61cc7d8fd499
https://hg.mozilla.org/projects/nss/rev/a5009ce53ec4
https://hg.mozilla.org/projects/nss/rev/57e38a8407b3

	Kai Engert (:KaiE:)
	Comment 90 • 7 years ago

In bug 1366761 I've added automatic detection of ABI changes.

To approve that the API additions from this bug are permissible ABI changes, I've made the following commit, which adds these new APIs as expected items in the abidiff report:
https://hg.mozilla.org/projects/nss/rev/713b15751ad2

	Kai Engert (:KaiE:)
	Comment 91 • 7 years ago

Could you please suggest a brief explanation of this new feature for the NSS release notes? In particular, maybe a brief summary WHERE these new URIs can be used?