321584 - NSS PKCS12 decoder fails to import bags without nicknames

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Subtitle: PKCS12 decoder nickname collision callback prototype deficient

The PKCS12 standard defines optional (user) "friendly name" attributes 
(also known as nicknames) for objects in the file.  NSS requires them.  
Windows allows them, but does not require them.  At least one other product 
that uses PKCS12 files for import/export of user certs never puts friendly 
names into PKCS12 files that it creates, and ignores friendly names in files 
that it inputs.  NSS is essentially unable to import PKCS12 files produced 
without friendly names.

NSS's PKCS12 decoder API features a callback API by which the decoder can
ask the calling application to come up with a new/better nickname. 
It is used in the cases where the PKCS12 decoder has found a "bag" in the
pkcs12 file that 
(a) has no nickname, or 
(b) has a nickname that is already used in the cert DB as the nickname 
for another cert with a different subject name.  

This "nickname collision" callback API was intended to allow an application
to import PKCS12 files that lacked nicknames, or had duplicative nicknames.
But sadly the API (prototype) for this "nickname collision" callback is not 
well designed.  In particular, the callback function arguments provide no 
information about which "bag" (or cert, or key) is the cause of the 
callback.  

If the PKCS12 file being decoded contains multiple certs (as is common),
the callback function has no way of knowing which of those certs has 
the missing or in-use nickname.  Regardless of the number of certs in the
file, the callback has no access to the information in the cert for which 
it is being called, information with which it might be able to construct 
a new nickname, or prompt the user to do so.  

Consequently, all NSS-based programs that import PKCS12 files are 
effectively unable to import PKCS12 files whose certs lack PKCS12 
friendly names.  Mozilla works around this by using a "default" 
nickname string, which obviously does not help the user to know
which cert is named by it.  pk12util simply fails without trying to
find a new name.  

The nickname collision callback has this prototype:

typedef SECItem * (PR_CALLBACK * SEC_PKCS12NicknameCollisionCallback)(
          SECItem *old_nickname, PRBool *cancel, void *arg);

The "arg" value is a value that the application provides to the decoder
when the decoder context is created, the third argument to the function
SEC_PKCS12DecoderStart.  Since this value is established before the 
decoder starts, it CANNOT contain any information that identifies the 
specific "bag" (cert, key) within the decoded file that has the problem.

I found 3 implemementations of the callback function in mozilla's source
repository, and NONE of them use the "arg" value AT ALL.  

There are several possible ways to attempt to fix this, including:
a) change the callback prototype, adding one or more missing arguments.
b) change the decoder to pass a different value for the "arg" argument
to the callback function. (same prototype, different void* value)
c) create a new callback prototype that includes the additional argument(s),
and create a new variant of SEC_PKCS12DecoderValidateBags that uses the 
new prototype instead of the old one.  

Choice a breaks source and binary compatibility.  It would not even work 
with the existing callback functions that ignore the "arg" argument, and 
so is unacceptable.

Choice c is the most elegant, most backwards compatible, and most work. 
An API addition of that magnitude should wait for the next minor NSS release.

Choice b would continue to work with the existing known callback functions
(all of which ignore the arg argument).  If there are any callback 
functions that, unknown to us, actually use the arg argument now, this 
change would break them.  Also, ideally, the callback would pass BOTH
the appliation-supplied info and the decoder context specific info 
(as with choice c), rather than just one or the other (as with choice b).

I propose to implement choice b in the short term, but I am open to being 
dissuaded.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Preliminary patch 1 (checked in on trunk)

This patch doesn't fix any of the problems described in this bug.
It is merely a large cleanup of the existing pkcs12 decoder.  
It eliminates MANY erroneous PORT_SetError calls that set a false
SEC_ERROR_NO_MEMORY error code, hiding the correct (and previously
set) error code.
It adds MANY PORT_SetError calls, to set SEC_ERROR_INVALID_ARGS 
when a function detects that the arguments it was passed were
invalid.  
It fixes at least one crash (using a null pointer after testing it 
for being NULL :-).

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch 2: fixes this bug, also includes patch 1 above

This patch should differ from the first patch by only a few lines.
These changes are the ones that fix the issues with the NSS PKCS12
library reported in this bug.
The next patch (to follow) changes pk12util to use the new definition
of the nickname collision callback arguments.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch 3, pk12util patch, implements new nickname collision callback (checked in on trunk)

Comment 4

[Neil Williams]

Comment on attachment 206901 [details] [diff] [review]
Preliminary patch 1 (checked in on trunk) 

Pretty extensive cleanup.

Comment 5

[Neil Williams]

Comment on attachment 207062 [details] [diff] [review]
Patch 2: fixes this bug, also includes patch 1 above

This patch obsoletes previous, I believe, and the following patch is to be applied in addition?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Use this link to see the differences between the first two patches:
<https://bugzilla.mozilla.org/attachment.cgi?oldid=206901&action=interdiff&newid=207062&headers=1>

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 206901 [details] [diff] [review]
Preliminary patch 1 (checked in on trunk) 

Checked in on trunk
Checking in p12d.c;     new revision: 1.31; previous revision: 1.30
Checking in p12local.c; new revision: 1.9;  previous revision: 1.8

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch 2: (by itself, excluding patch 1) (checked in on trunk)

Patch 2 above includes patch 1, too.
This patch is the diffs between patches 1 and 2 above.
This is patch 2, without patch 1 also being included.
This is what I expect to check in, now that patch 1 has been 
checked in.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 207063 [details] [diff] [review]
patch 3, pk12util patch, implements new nickname collision callback (checked in on trunk)

This patch needs to be checked in at the same time as patch 2.  So, the checkin for patch 2 needs to wait for this patch to be reviewed.

Comment 10

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 11

[Neil Williams]

Comment on attachment 207063 [details] [diff] [review]
patch 3, pk12util patch, implements new nickname collision callback (checked in on trunk)

This patch looks good but I noticed a quibble with the previous one that's makes it harder to verify this one:
sec_pkcs12_validate_cert (which is private to p12d.c) takes as a last argument wincx which it no longer uses, passing leafCert instead. I'd recommend removing that arg from the function definition.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Checked in on trunk
lib/pkcs12/p12d.c;       new revision: 1.35; previous revision: 1.34
cmd/pk12util/pk12util.c; new revision: 1.35; previous revision: 1.34

Need second reviews for branch.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 207063 [details] [diff] [review]
patch 3, pk12util patch, implements new nickname collision callback (checked in on trunk)

Alexei, please SR both patches attached to this bug.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218454 [details] [diff] [review]
Patch 2: (by itself, excluding patch 1) (checked in on trunk)

Alexei, please SR both patches attached to this bug.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 206901 [details] [diff] [review]
Preliminary patch 1 (checked in on trunk) 

Alexei, please SR all 3 patches attached to this bug, for the 3.11 branch.

Comment 16

[Alexei Volkov]

Comment on attachment 206901 [details] [diff] [review]
Preliminary patch 1 (checked in on trunk) 

r=alexei.volkov.bugs

Comment 17

[Alexei Volkov]

Comment on attachment 207063 [details] [diff] [review]
patch 3, pk12util patch, implements new nickname collision callback (checked in on trunk)


Just a miner notice:
cancel is no longer set by the function. It is not a problem for the function that calls this callback since it preset cancel to PR_TRUE before the call, but would to explicitelly set it to PR_FALSE.
>+#if 0
>     /* XXX not handled yet  */
>     *cancel = PR_TRUE;
>     return NULL;
> #else

Comment 18

[Alexei Volkov]

Comment on attachment 218454 [details] [diff] [review]
Patch 2: (by itself, excluding patch 1) (checked in on trunk)

r=alexei.volkov.bugs

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

This is fixed on the trunk, and I just don't have time to fix it on the branch.