95311 - SEC_ASN1DecodeItem asserts when decoding SPKI

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Jamie Nicolson]

SECKEY_ImportDERPublicKey expects a raw, DER-encoded public key. If you instead
give it a DER-encoded SubjectPublicKeyInfo, the ASN1 decoder asserts. I will
attach a simple program to reproduce the error.

Comment 1

[Jamie Nicolson]

Attachment: program that reproduces the error

Comment 2

[Jamie Nicolson]

Attachment: SubjectPublicKeyInfo, input for the test program

Comment 3

[Jamie Nicolson]

To reproduce:

1. Build the program, linked against libnss3.so.
2. Run it with "a.out spki.encoded"

Comment 4

[Wan-Teh Chang]

Julien, you are now the owner of our ASN.1 decoder.

Comment 5

[Julien Pierre]

Lowering priority to P2 with Wan-teh's agreement.

Comment 6

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 7

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 8

[Julien Pierre]

Attachment: patch to use quick DER in SECKEY_ImportDERPublicKey

This patch uses SEC_QuickDERDecodeItem instead of SEC_ASN1DecodeItem .
With Jamie's test program, the output is :

C:\nss\37\mozilla\dist\WIN954.0_DBG.OBJ\bin>spki spki.der
Read 94 bytes from file
ACK

C:\nss\37\mozilla\dist\WIN954.0_DBG.OBJ\bin>

Comment 9

[Julien Pierre]

When I switch to the superior QuickDER decoder, the assertion disappeared :)
Jamie, is this the expected output from your test program ?

Comment 10

[Jamie Nicolson]

Yes, this is the correct behavior. QuickDER fixes this bug.

Comment 11

[Wan-Teh Chang]

We should look at the assertion failure in the
old ASN.1 decoder because it is still being
used in some places.  What is the assertion
failure?

Comment 12

[Jamie Nicolson]

Assertion failure: 0, at secasn1d.c:1220

This is in the function sec_asn1d_free_child:

if (error && state->top->their_pool == NULL) {
        /*
         * XXX We need to free anything allocated.
         */
        PORT_Assert (0);
    }

Comment 13

[Julien Pierre]

I'm having some doubts here.
Jamie, here is the output of derdump on the input you provided :

(strange)/export/home/jpierre/nss/37/mozilla/dist/SunOS5.8_DBG.OBJ/bin{52}
derdump -i spki.der
C-Sequence  (92)
   C-Sequence  (13)
      Object Identifier  (9)
         1 2 840 113549 1 1 1 (PKCS #1 RSA Encryption)
      NULL  (0)
   Bit String  (75)
      00 30 48 02 41 00 e0 8e 42 20 7f 77 86 df 7f 45 99 2d 9b 77 16 ba 
      ed a9 51 1f 71 5e fd ce 2e 79 26 5b 0d 27 2f a5 b0 2b ea 32 66 3f 
      4d e3 35 cb 10 5e f2 10 07 49 3b 6b 06 f5 f3 cd 5d 73 c0 56 a2 b7 
      00 2d 56 d5 02 03 01 00 01 

And here is the template that's being passed to the decoder :

const SEC_ASN1Template SECKEY_RSAPublicKeyTemplate[] = {
    { SEC_ASN1_SEQUENCE, 0, NULL, sizeof(SECKEYPublicKey) },
    { SEC_ASN1_INTEGER, offsetof(SECKEYPublicKey,u.rsa.modulus), },
    { SEC_ASN1_INTEGER, offsetof(SECKEYPublicKey,u.rsa.publicExponent), },
    { 0, }
};

I don't see how this can possibly work .
I traced the old decoder and it fails because it expects a tag of "2" (for the
integer) which is not found, as shown in the derdump output.

I don't know why quickder allowed this to decode. I'm going to debug this further.

Comment 14

[Julien Pierre]

Actually the patch was bad, it would call quickder without an arena, which would
cause it to always fail, without even looking at the input. So we don't want to
do it that way. I will attach a new patch that properly creates an arena in the
key structure and uses it for decoding.

Jamie, I'm not sure if I understand what the purpose of your test was and what
you expect us to do here.

Were you purposedly feeding invalid data to the function and expecting the
decoder to fail ? If so, was the bug that it asserted rather than just return a
failure ?

Comment 15

[Julien Pierre]

Never mind that coment, the patch was alright WRT arena management, I just
didn't apply it correctly on my tree ;)

Comment 16

[Julien Pierre]

OK, I think I understand the meaning of those assertions now, with the comments
nearby.

The problem in this case is that something has already been decoded before we
hit the decode error. Ie. it does not happen on the first tag.
At that point, SEC_ASN1DecodeItem chokes, because it has no way of freeing the
data before returning a failure, since it was allocated with PR_Malloc rather
than in an arena. The assert is to warn that there is a memory leak. The only
way to avoid it is to use an arena. This is a design issue with
SEC_ASN1DecodeItem. The only possible fix would be to maintain a list of all
pointers allocated with PR_Malloc (ie. a pseudo arena) within the decoder, that
could be freed at the top-level SEC_ASN1DecodeItem upon failure. Still, even in
success cases, the caller is likely to forget to free some memory without that
list ... My take on this is that you should always use arenas even when using
SEC_ASN1DecodeItem, and the assertion should just be removed as there is no easy
fix for the leak.

SEC_QuickDERDecodeItem does not have the problem of possibly leaking memory in
failure cases, because it always requires an arena, and always requires the DER
input to stick around.

Comment 17

[Julien Pierre]

Attachment: comment memory leak assertion in SEC_ASN1DecodeItem

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

I think this assert should remain here.  The real fix is as noted in bug 175163

Comment 19

[Jamie Nicolson]

You are correct, the data in the test program does not match the template. The
point is, the decoder should never assert just because bad data is passed in. It
should recover gracefully and return an error message. QuickDER does that, which
is the correct behavior.

Comment 20

[Wan-Teh Chang]

We should not use assertions on bad input data.
Assertions should only be used to validate the
assumptions the correctness of our code relies
on or to verify the results of our code.

I think this assertion should be replaced by
comments or a warning message printed to stderr
in debug builds.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Wan-teh, when you say "on bad data input", do you mean bad DER data (only)?
or do you mean "any bad arguments passed to the function?"

I'd like to suggest that we change the function so that it asserts that 
arena is not NULL immediately on input.  IOW, declare that it is a programming
error for this function to be called with a NULL arena, and then fix what 
breaks.  In non-debug builds, it would detect a null arena and return an
error PR_INVALID_ARGS_ERROR or equiv.  

Comment 22

[Wan-Teh Chang]

Nelson,

I mean any bad arguments passed to the function.  Bad
arguments passed to a function should be reported with
an "invalid argument" error code, not an assertion failure.

There are legitimate reasons to pass bad arguments to
a function.  For example, a test program may pass bad
arguments to a function to verify it fails with the
expected error codes.

Your suggested change would break backward compatibility.
It is difficult but possible to use SEC_ASN1DecodeItem
with a NULL arena without memory leaks.  We can deprecate
that usage but we can't make it an error until NSS 4.0.

Comment 23

[Julien Pierre]

Attachment: remove assertion and replace it with comments

Comment 24

[Julien Pierre]

Checking in secasn1d.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1d.c,v  <--  secasn1d.c
new revision: 1.18; previous revision: 1.17
done

Referencing http://bugzilla.mozilla.org/show_bug.cgi?id=175163 .

Comment 25

[Wan-Teh Chang]

Comment on attachment 103916 [details] [diff] [review]
remove assertion and replace it with comments

Julien, I hate to be nit-picking, but I need to ask
you to use the same comment block formatting style
that the original code is using.  Thanks.

Comment 26

[Julien Pierre]

Fixed

Checking in secasn1d.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1d.c,v  <--  secasn1d.c
new revision: 1.19; previous revision: 1.18
done

Comment 27

[Julien Pierre]

Should I also check in the fix to use QuickDER here ?
I had only removed the assertion from secasn1d.c . QuickDER can be used here
which would always avoid the memory leak even in error cases such as this one.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Do we know/expect that the input will always be DER and not BER here?

Comment 29

[Julien Pierre]

Nelson,

I assume that's the case given that the patch is done in a function named
SECKEY_ImportDERPublicKey .

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

D'oh!  Then yes, you should use QuickDer here, IMO.

Comment 31

[Julien Pierre]

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.22; previous revision: 1.21
done

I just realized we need additional code to copy the DER input into the arena.
There should also be code to free the arena if the function fails. I will attach
an incremental patch.

Comment 32

[Julien Pierre]

Attachment: copy the DER input key to the arena, and free the arena upon decoding failure

incremental patch for 102407

Comment 33

[Julien Pierre]

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.23; previous revision: 1.22
done