241013 - [FIX]nsNSSComponent::VerifySignature leaks on errors

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 240661 comment 9 and bug 240661 comment 10.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This conflicts with the patches in bug 240661, but oh, well.  I'll merge in
either direction as needed.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -b, FOR REVIEW ONLY

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146528 [details] [diff] [review]
Same as diff -b, FOR REVIEW ONLY

John, would you review?  Nelson, if you have time to take a look, that would
also be much appreciated...

Who'd be a good sr for changes to this code?

Comment 4

[John G. Myers]

Comment on attachment 146528 [details] [diff] [review]
Same as diff -b, FOR REVIEW ONLY

I don't think the fingerprint can have non-ASCII characters, so full UTF-8
conversion isn't absolutely necessary  there.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146528 [details] [diff] [review]
Same as diff -b, FOR REVIEW ONLY

Brendan, would you sr?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Taking...

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Several suggestions about this patch:

1. I think you should ignore the return value from Update, and base your
decision entirely on the return value from Finish.  I do not know that it
can never be true that update returns an error bug finish also returns a
non-null value, so I think the assertion might conceivably hit.

2. You could replace the sequence of calls to the decoder (start, update, 
finish) with a single call to SEC_PKCS7DecodeItem().

3. NSS has an old PKCS7 decoder, and a new CMS decoder.  CMS is a major 
new revision of the old PKCS7 standard.  It allows signatures to contain a
superset of the things that were allowed in PKCS7.  AFAIK, the function 
being patched here is that last remaining use of the old PKCS7 decoder in 
mozilla.  All other calls have been converted to use the CMS decoder, IINM.
So, it might be good to start using the new NSS CMS decoder in this function,
rather than continuing to use the old PKCS7 decoder

However, the conversion may be non-trivial.  The CMS decoder function that 
is closest to SEC_PKCS7DecodeItem() is NSS_CMSMessage_CreateFromDER() 
(which should have been named FromBER).  So, the suggestion to convert to
the CMS decoder should probably be in another RFE.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146528 [details] [diff] [review]
Same as diff -b, FOR REVIEW ONLY

Yeah, item 3 sounds like a little more than I'm willing to do given my general
lack of knowledge in this code....  I'll work on doing item 2 (which obviates
item 1 if I read correctly).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.. Using SEC_PKCS7DecodeItem() means shoehorning my data into a SECItem,
which means converting my |const char*| to |unsigned char*|.  I guess I could
just reinterpret_cast it.... :(

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch using Nelson't suggestion 2

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -b, for review only

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146632 [details] [diff] [review]
Same as diff -b, for review only

I went with siEncodedCertBuffer as the type, but that could well be totally
wrong.	If someone knows what the actual type is (not that SEC_PKCS7DecodeItem
cares), please tell me....

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

r=MisterSSL for the diffs between the two patches.  

There are very few places where si.type is used.  
I think you made the right choice.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146632 [details] [diff] [review]
Same as diff -b, for review only

brendan, could you please sr?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 146632 [details] [diff] [review]
Same as diff -b, for review only

sr=[e-mail]

/be

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in on the trunk for Mozilla 1.8a.