Closed Bug 1162897 Opened 5 years ago Closed 3 years ago

Allow certificates to be specified by RFC7512 PKCS#11 URI

Categories

(NSS :: Libraries, enhancement)

3.18
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dwmw2, Assigned: dwmw2)

References

Details

Attachments

(29 files, 7 obsolete files)

6.85 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.81 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
43.34 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
6.10 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
2.81 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.94 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.69 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
7.22 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
3.36 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
8.29 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1004 bytes, patch
rrelyea
: review+
Details | Diff | Splinter Review
7.25 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
7.21 KB, patch
KaiE
: review+
Details | Diff | Splinter Review
2.45 KB, patch
KaiE
: review+
Details | Diff | Splinter Review
802 bytes, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.43 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
7.34 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
2.28 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
6.36 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.60 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
9.54 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
4.74 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.02 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.43 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
37.25 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.82 KB, patch
Details | Diff | Splinter Review
3.95 KB, patch
Details | Diff | Splinter Review
3.30 KB, patch
Details | Diff | Splinter Review
9.52 KB, patch
Details | Diff | Splinter Review
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.
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.
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... :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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/ ?
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.
Attachment #8782904 - Flags: review?(rrelyea)
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.
Attachment #8782907 - Flags: review?(rrelyea)
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.
Attachment #8782913 - Flags: review?(rrelyea)
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).
Attachment #8782916 - Flags: review?(rrelyea)
$ 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
Attachment #8782921 - Flags: review?(rrelyea)
Add PK11_GetModuleURI() function
Attachment #8782922 - Flags: review?(rrelyea)
/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
...
Attachment #8782923 - Flags: review?(rrelyea)
Rename and export find_objects_by_template()

We're going to want this for PK11_FindCertFromURI() and friends.
Attachment #8782925 - Flags: review?(rrelyea)
Add NSSTrustDomain_FindTokensByURI()
Attachment #8782926 - Flags: review?(rrelyea)
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.
Attachment #8782927 - Flags: review?(rrelyea)
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! :)
Attachment #8782929 - Flags: review?(rrelyea)
For reference and ease of testing, all the above commits are in
https://github.com/varunnaganathan/nss/commits/20160819
Attachment #8782904 - Flags: review?(rrelyea) → review+
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.
Attachment #8782907 - Flags: review?(rrelyea) → review+
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
Attachment #8782916 - Flags: review?(rrelyea) → review+
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
Attachment #8782921 - Flags: review?(rrelyea) → review+
Attachment #8782922 - Flags: review?(rrelyea) → review+
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;)
Attachment #8782923 - Flags: review?(rrelyea) → review+
Attachment #8782925 - Flags: review?(rrelyea) → review+
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.
Attachment #8782926 - Flags: review?(rrelyea) → review+
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?
(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... :)
(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->
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.
Attachment #8782913 - Flags: review?(rrelyea) → review+
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.
Attachment #8782929 - Flags: review?(rrelyea) → 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?

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).
> 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'
> 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
(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.
> 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
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().
Attachment #8782927 - Flags: review?(rrelyea) → review+
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.
Attachment #8787785 - Flags: review?(kaie)
Attachment #8787785 - Flags: review?(kaie) → review?(rrelyea)
Flags: needinfo?(kaie)
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.)
(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.)
Flags: needinfo?(kaie)
Assigning to David, because he is working on it.
Assignee: nobody → dwmw2
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?
Attachment #8794867 - Flags: review?(rrelyea)
Oops, I hadn't noticed that this was a disguised linker script. This should fix it.
Attachment #8794868 - Flags: review?(rrelyea)
(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.
Comment on attachment 8794867 [details] [diff] [review]
0013-Fix-clang-format-bustage.patch

r=kaie on this whitespace-only change
Attachment #8794867 - Flags: review?(rrelyea) → review+
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
Attachment #8794868 - Flags: review?(rrelyea) → review-
> NSS 3.27 is already completed, please change the version to 3.28

Fixed; thanks.
Attachment #8794868 - Attachment is obsolete: true
Attachment #8794872 - Flags: review?(kaie)
Attachment #8794872 - Flags: review?(kaie) → review+
Comment on attachment 8787785 [details] [diff] [review]
bug1162897-20160819-20160902.patch

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

r+
Attachment #8787785 - Flags: review?(rrelyea) → review+
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.
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?
Attachment #8795224 - Flags: review?(rrelyea)
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.
Attachment #8795226 - Flags: review?(rrelyea)
(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.
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.?
(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.
(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".
SSL tests fixed to work in FIPS mode too, where the token name differs.
Attachment #8795226 - Attachment is obsolete: true
Attachment #8795226 - Flags: review?(rrelyea)
Attachment #8795360 - Flags: review?(rrelyea)
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
Attachment #8795224 - Flags: review?(rrelyea) → review+
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
Attachment #8795360 - Flags: review?(rrelyea) → review+
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.
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.
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.
Attachment #8864874 - Flags: review?(rrelyea)
Attachment #8864875 - Flags: review?(rrelyea)
This obsoletes attachment 8782921 [details] [diff] [review].  This is adjusted to use the new parser API.
Attachment #8864884 - Flags: review?(rrelyea)
This obsoletes attachment 8782922 [details] [diff] [review].  This is adjusted to use the new parser API.
Attachment #8864885 - Flags: review?(rrelyea)
This obsoletes attachment 8782923 [details] [diff] [review].  Rebase only, except s/free/PORT_Free/.
Attachment #8864886 - Flags: review?(rrelyea)
This obsoletes attachment 8782926 [details] [diff] [review].  Adjusted to use the new parser API.
Attachment #8864890 - Flags: review?(rrelyea)
This obsoletes attachment 8782927 [details] [diff] [review].  Adjusted to use the new parser API.
Attachment #8864892 - Flags: review?(rrelyea)
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.
Attachment #8864874 - Flags: review?(rrelyea) → review-
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?
Attachment #8864875 - Flags: review?(rrelyea) → review+
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
Attachment #8864879 - Flags: review?(rrelyea) → review+
Attachment #8864883 - Flags: review?(rrelyea) → review+
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.
Attachment #8864885 - Flags: review?(rrelyea) → review-
Attachment #8864884 - Flags: review?(rrelyea) → review+
Attachment #8864886 - Flags: review?(rrelyea) → review+
Attachment #8864887 - Flags: review?(rrelyea) → review+
Attachment #8864890 - Flags: review?(rrelyea) → review+
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.
Attachment #8864892 - Flags: review?(rrelyea) → review+
Attachment #8864894 - Flags: review?(rrelyea) → review+
Attachment #8864895 - Flags: review?(rrelyea) → review+
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.
Attachment #8864874 - Attachment is obsolete: true
Attachment #8867231 - Flags: review?(rrelyea)
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).
Attachment #8867231 - Flags: review?(rrelyea) → review+
> 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.
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.
Attachment #8864881 - Flags: review?(rrelyea) → review+
OK, you are one copy and paste error from having everything ready to go!

bob
Typo fix in the comment: s/null-terminatd/null-terminated/.
Attachment #8864875 - Attachment is obsolete: true
Moved the new symbol under the NSS_3.31 section in nss.def, as suggested in comment 75.
Attachment #8864884 - Attachment is obsolete: true
Fixed the copy-and-paste error (comment 74) and moved the new symbol under the NSS_3.31 section (comment 75).
Attachment #8864885 - Attachment is obsolete: true
Moved the new symbols under the NSS_3.31 section as suggested in comment 75.
Attachment #8864892 - Attachment is obsolete: true
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?
Flags: needinfo?(kaie)
(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?
Flags: needinfo?(kaie) → needinfo?(dueno)
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.
Flags: needinfo?(dueno)
(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.
See Also: → 1365193
Blocks: 1366187
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
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?
Blocks: 1439350
You need to log in before you can comment on or make changes to this bug.