314115 - SEC_QuickDERDecodeItem modifies SECItem.type field during decoding process

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Alexei Volkov]

Found a regression with SEC_ASN1DecodeItem function. SECItem.type field get reset to siBuffer during decoding. 

Adding line fixes the problem for siUsignInteger case.

--- quickder.c  2 Apr 2005 05:33:41 -0000       1.22
+++ quickder.c  28 Oct 2005 00:02:12 -0000
@@ -860,6 +860,7 @@
     if ((SECSuccess == rv) && (PR_TRUE == save))
     {
         SECItem* destItem = (SECItem*) ((char*)dest + templateEntry->offset);
+        temp.type = destItem->type;
         if (destItem)
         {
             *(destItem) = temp;

Comment 1

[Julien Pierre]

Please also try this fix, which is more generic than the other one as it causes the type field in the destination to always be left untouched, rather than just for SEC_ASN1_INTEGER .

Index: quickder.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/util/quickder.c,v
retrieving revision 1.22
diff -r1.22 quickder.c
865c865,866
<             *(destItem) = temp;
---
>             destItem->data = temp.data;
>             destItem->len = temp.len;

Comment 2

[Alexei Volkov]

Julien, last two lines fix regression as well.

Comment 3

[Wan-Teh Chang]

Is this a regression because something worked in an old
NSS release no longer works, or because something that
works in SEC_ASN1DecodeItem doesn't work in SEC_QuickDERDecodeItem?

Should the caller set destItem->type before calling
SEC_QuickDERDecodeItem?

Comment 4

[Julien Pierre]

Wan-Teh,

This is a bug in the QuickDER decoder, and a difference in behavior with the classic decoder, not technically a regression, but something we need to fix.
The only time dest->type needs to be set prior to calling the decoder is for the SEC_ASN1_INTEGER case. The decoder removes leading zeroes if the value is siUnsignedInteger . Otherwise, the type field should be ignored. In all cases, the value of type should be left alone accross the call to either decoder, because the type field is only treated as an input parameter (even though it is a subfield in the output argument ...). This is a hack that was introduced in NSS a long-time ago, and unfortunately one that we have to continue to support.

Comment 5

[Wan-Teh Chang]

Julien,

Thanks for the explanation.  I hope you can add some comments
that explain:
1. When does the caller need to set the SECItem's type field
before calling SEC_ASN1DecodeItem or SEC_QuickDERDecodeItem?

2. Does SEC_ASN1DecodeItem or SEC_QuickDERDecodeItem set the
SECItem's type field?

Comment 6

[Julien Pierre]

Wan-Teh,

1) There is already a comment in the QuickDER code about siUnsignedInteger :

            case SEC_ASN1_INTEGER:
                {
                    /* remove leading zeroes if the caller requested siUnsignedInteger
                       This is to allow RSA key operations to work */
                    SECItem* destItem = (SECItem*) ((char*)dest + templateEntry->offset);
                    if (destItem && (siUnsignedInteger == destItem->type))

The only other place where I think more comments are warranted is in the tech note 1 about the decoders .

2) QuickDER sets the type to 0 (siBuffer) as a result of PORT_ArenaZAlloc . But this only applies to SECItems that QuickDER itself allocated, for POINTER, SET OF and SEQUENCE OF, or subcomponents of these  .
A quick look at secasn1d.c shows the same is true for the classic decoder .

Comment 7

[Julien Pierre]

Attachment: fix

Comment 8

[Wan-Teh Chang]

Comment on attachment 201223 [details] [diff] [review]
fix

r=wtc.  Please generate unified context diffs
(cvs diff -u) in the future.

Comment 9

[Wan-Teh Chang]

Julien:

The comments I requested need to be added to the public
header secasn1.h.  We use comments in our public headers
as a replacement for real documentation.  Most of the NSS
users won't read the .c files in our source tree. 

Comment 10

[Julien Pierre]

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

Checking in secasn1.h;
/cvsroot/mozilla/security/nss/lib/util/secasn1.h,v  <--  secasn1.h
new revision: 1.14; previous revision: 1.13
done

I added the following to secasn1.h :

/* Both classic ASN.1 and QuickDER have a feature that removes leading zeroes
   out of SEC_ASN1_INTEGER if the caller sets siUnsignedInteger in the type
   field of the target SECItem prior to calling the decoder. Otherwise, the
   type field is ignored and untouched. For SECItem that are dynamically
   allocated (from POINTER, SET OF, SEQUENCE OF) the decoder sets the type
   field to siBuffer. */