160805 - Convert NSS code to use SEC_QuickDERDecodeItem

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

Wherever possible in NSS, DER decoding code should be converted to use this
function in place of SEC_ASN1DecodeItem. This will save memory and CPU. The
changes needed to do this are very minor :
1) the caller must keep the data in src
2) the caller must pass an arena to the decoder

Many places already comply with those requirements, and will only need a symbol
change. Places that don't need arena or free the input will need more extensive
changes.

This bug is a placeholder which is expected to receive a lot of small patches in
various areas of NSS to implement the same change accross the board.

Comment 1

[Julien Pierre]

Attachment: Large patch to replace SEC_ASN1DecodeItem calls with SEC_QuickDERDecodeItem on "lib" directories

Comment 2

[Julien Pierre]

I have attached a fairly large patch that replaces the calls to
SEC_ASN1DecodeItem with calls to SEC_QuickDERDecodeItem.

I primarily went after the instances of SEC_ASN1DecodeItem that already used an
arena, since these calls can be directly replaced with calls to the new decoder.
The hard part was determining in which places BER may be expected instead of
DER, so as not to patch these calls. The one that gave me trouble was the P12
code, as some calls are not just under pkcs12 but also in the softoken and
pk11wrap. This is what the review should focus on - make sure I didn't patch any
places that shouldn't be because they can accept BER.

There is also one patch that converts a non-arena usage to arena in secname.c . 

I have run all.sh on a tip build with this patch and everything is green. I have
also used Mozilla and had the same success with the patch as without.

I believe this set of changes is safe to check in. The benefit is a performance
improvement, most probably in the certificate decoding. I did not have time to
run any performance tests, but if the effect of the quick DER decoder is as
significant on certs as it was on CRLs, then this could be a big win for some
apps that manipulate a lot of certs like PSM.

There should also be a phase 2 - look at the DER usages of SEC_ASN1DecodeItem
that don't use arenas. They should be revisited whenever possible to be able to
use the new decoder, which requires an arena. But that's more complicated and
may not make the cut in 3.6 like this patch should.

Comment 3

[Julien Pierre]

Would anyone care to review this patch ?
It would provide a performance improvement. If there is no time to review the
entire patch or if we are only concerned about cert decoding performance for
now, the review can be limited to the cert files for NSS 3.6.

Comment 4

[Robert Relyea]

Comment on attachment 95523 [details] [diff] [review]
Large patch to replace SEC_ASN1DecodeItem calls with SEC_QuickDERDecodeItem on "lib" directories

As far as I can tell it looks good, a context diff would be preferable. All the
objects I see are DER not BER objects, and they all appear to be using the
arena.

Comment 5

[Julien Pierre]

Attachment: context diff

Comment 6

[Julien Pierre]

I attached the full context diff of the same patches. Since it's already been
reviewed, it is now checked in to the tip.

I am keeping this bug open however as there is still a second half to it - the
conversion of code that doesn't use arenas.

Comment 7

[Julien Pierre]

There is a problem with this patch.
Unlike SEC_ASN1DecodeItem, SEC_QuickDERDecodeItem does not make a copy of the
DER input. Rather, the result always points back into the DER input.
If the input was not allocated in the arena and is freed, then the result of the
decoder becomes invalid.

The solution for this is to make a copy of the DER input into the arena.
However, this will use more memory, so we don't want to do it by default.
This means the caller should do it.

We need to re-review all 47 cases of calls to SEC_ASN1QuickDERDecodeItem in the
patch that went in last friday to ensure that is the case.

There is at least one instance with cert extensions that Bob found where the DER
didn't come from the arena, so the decoded data became invalid.

Comment 8

[Julien Pierre]

Attachment: Patch for incorrect SEC_QuickDERDecodeItem usages

This patch makes a copy of the SECItem in all functions that call it which
don't free the arena before returning.

This ensures that there is never any decoded data pointing to a buffer outside
the arena, which may get freed separately.

Comment 9

[Julien Pierre]

I checked in the patch and removed the copy in SEC_QuickDERDecodeItem.
Bob, it would still be helpful if a new review was done of the remaining usages
of quickder, just in case I missed any.

Some of the cases were not obvious, like decoding done in multiple steps. In
those cases, I only did the copy at the first step, not in the following
intermediate steps, to conserve memory.

Comment 10

[Wan-Teh Chang]

Attachment: Fix a double free in CERT_DecodeAVAValue

There is a pre-existing double free in CERT_DecodeAVAValue.
As part of a patch for this bug the PORT_Free calls were
replaced by PORT_ArenaFree calls, and the double free
results in an infinite loop in NSPR's FreeArenaList function.

This patch fixes that.	Since this is a pre-existing problem,
I will open a separate bug.

Comment 11

[Wan-Teh Chang]

Attachment: Fix a leak and a double free in CERT_DecodeAVAValue

I found that the double free in CERT_DecodeAVAValue
is not a pre-existing bug but rather was introduced
by an earlier patch for this bug.

I also found that my previous fix is wrong.  Here
is a correct fix.

1. It uses the current name 'PLArenaPool' instead
of the old name 'PRArenaPool'.

2. It fixes the problem that the arena is not freed
if SEC_QuickDERDecodeItem fails.

3. There are two ways to fix the double free.  I
choose the simpler fix.  I allocate 'utf8Val' in the
arena.	This makes it simple to free all the data
allocated in this function: just free the arena.

In the interest of time, I've checked in this fix,
but I would really appreciate a code review.

Comment 12

[Julien Pierre]

I think we should continue this effort in 3.7 and go after the places where DER
is used but an arena was not, and see if it is possible to change the memory
management easily. I did not even attempt it for those cases in 3.6 .

Also, I only went over the nss/lib/* subdirectories, completely ignoring the
cmd/* directories . As Nelson found out, there is code that could benefit from
QuickDER there too, especially as it appears to have fewer bugs.

Comment 13

[Wan-Teh Chang]

I suggest that we divide the NSS source tree into
five pieces, open a bug for each, and use this bug
as the tracking bug.  It's too much work for one
person to make the requested change to the entire
tree.

Bob, could you take care of this and assign the
sub-bugs?

Comment 14

[Robert Relyea]

Adding dependencies for the 5 separate bugs to update the tree.

Comment 15

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 16

[Julien Pierre]

Nelson pointed out that one of the optimizations made to softoken was incorrect
and requires backing out :

"pk11_unwrapPrivateKey now calls SEC_QuickDERDecodeItem to decode 
unwrapped private keys.  This is OK for keys that were wrapped by our
token, because our token always DER-encodes private keys. But the PKCS 11
spec says that private keys are BER encoded, so private keys from another
implementation might fail to decode now that this function has switched to
the DER-only decoder."

I will attach a patch to fix the regression.

Comment 17

[Julien Pierre]

Attachment: fix a regression in NSS 3.6 & up with incorrect QuickDER usage

Comment 18

[Julien Pierre]

Comment on attachment 111664 [details] [diff] [review]
fix a regression in NSS 3.6 & up with incorrect QuickDER usage

Checked this patch into the tip (NSS 3.8).

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v	<--  pkcs11c.c
new revision: 1.43; previous revision: 1.42
done

Comment 19

[Wan-Teh Chang]

Julien, could you open a separate bug for this regression?
Thanks.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.