174885 - SEC_ASN1DecodeItem returns NULL and SECSuccess

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

This bug is a continuation of bug 174188.

See bug 174188 for instructions on how to reproduce.

When SEC_ASN1DecodeItem is called with CERT_CertificateRequestTemplate
and the cert request contains an empty set of attributes, SEC_ASN1DecodeItem 
returns SECSuccess, but the pointer named attributes in the parsed
CERTCertificateRequest contains NULL.  It should contain a non-NULL pointer 
to an array of pointers, the first of which is NULL.

In NSS 3.7, SECU_PrintCertificateRequest calls SEC_QuickDERDecodeItem 
instead of SEC_ASN1DecodeItem, because SEC_QuickDERDecodeItem returns the 
proper value in cr->attributes.  I believe this is correct, because 
(I think) a certificate request should always be DER encoded.  However,
if a certificate request is permitted to be BER encoded, then
SECU_PrintCertificateRequest should be changed back to use SEC_ASN1DecodeItem again.

SEC_ASN1DecodeItem should be fixed for this case.

Comment 1

[Wan-Teh Chang]

Julien, could you take a look at this?

Nelson, I think this is a design decision rather than a
bug of our classic ASN.1 decoder.  But I agree with your
suggested change.  It will simplify the clients' code
(no need to test for a NULL pointer) and match the
behavior of the new QuickDER decoder.

I believe the code in question is in lib/util/secasn1d.c:

890     if (state->underlying_kind & SEC_ASN1_GROUP) {
            ...
900         if (state->contents_length != 0 || state->indefinite) {
                ...
916         } else {
917             /*
918              * A group of zero; we are done.
919              * XXX Should we store a NULL here?  Or set state to
920              * afterGroup and let that code do it?
921              */
922             state->place = afterEndOfContents;
923         }
924         return;
925     }

The "XXX" comment shows that this is a design decision
that the original author was not sure about.

Maybe we can try what the comment suggests, setting
state->place to afterGroup?

Comment 2

[Julien Pierre]

I would call this behavior a bug.

If the component is OPTIONAL, then there is no way for the application to tell
if the component is present but empty, as opposed to missing.

Setting a pointer that point to an array containing only a terminating NULL
allows differentiation.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I agree that this is a bug, but for a different reason.  
I think it is an error for the decoder to return SECSuccess, indicating that
no error occurred, when the returned struct contains a NULL pointer for a 
non-OPTIONAL member.  SECSuccess means, all the required members were present.

In PKCS#10, the attributes member of the CertificateRequestInfo sequence
is IMPLICIT, but not OPTIONAL.  It is declared:

  attributes [0] IMPLICIT Attributes

where the type Attributes is defined as 

Attributes ::= SET OF Attribute

In the case we're discussing, the SET OF Attribute is present, but is empty.
So, the array of pointers to the SECItems for the Attribute(s) should exist,
but the first pointer should be NULL.  

SEC_QuickDERDecodeItem gets it right.   (Good job!)

Comment 4

[Wan-Teh Chang]

OK, you are right. This is a bug.  It is important to
differentiate between an empty set and a missing set.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

At the risk of belaboring this point, I will mention that in order to encode 
the empty set of Attributes, the NSS Encoder requires the pointer to the array 
of pointers to the Attributes to be non-NULL, but allows the first pointer in 
the array to be NULL.   

I think it is appropriate for the decoder to produce as output the same values 
that the encoder requires as input.  

Comment 6

[Wan-Teh Chang]

Nelson, there is no doubt in my mind that this is a bug now.

Comment 7

[Wan-Teh Chang]

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

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Taking bug.  I believe that bug 204555 is another manifestation of this bug.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

When SET OF or SEQUENCE OF contains no items, allocate an array of pointers to 

the items, and make the first item in that array be NULL, rather than having
the pointer to the array itself be NULL.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 123860 [details] [diff] [review]
patch v1

This patch fixes one case were the decoder returns a NULL pointer for a
required item.	There may be others, but this one has been experienced numerous
times.

Comment 12

[Wan-Teh Chang]

Comment on attachment 123860 [details] [diff] [review]
patch v1

r=wtc.

In sec_asn1d_concat_group, I believe that we can also simply change

     if (state->subitems_head != NULL) {

to

    if (placep != NULL) {

and remove the "PORT_Assert (placep != NULL);" assertion and the
"else if (placep != NULL)" block.  The code in the
"if (state->subitems_head != NULL)" block actually also works if
state->subitems_head == NULL.

Comment 13

[Wan-Teh Chang]

Comment on attachment 123860 [details] [diff] [review]
patch v1

If you decide to make the change I suggested, we probaly should
replace the assertion in the original if block:

    PORT_Assert (placep != NULL);

by this assertion outside the new if block:

    PORT_Assert (state->subitems_head == NULL || placep != NULL);

to be equivalent to the original assertion.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 incorporates wtc's review comments

Comment 15

[Wan-Teh Chang]

Comment on attachment 123942 [details] [diff] [review]
patch v2 incorporates wtc's review comments

>-	*placep = group;
>-
> 	item = state->subitems_head;
> 	while (item != NULL) {
> 	    *group++ = item->data;
> 	    item = item->next;
> 	}
> 	*group = NULL;
>+	*placep = group;

You can't move the "*placep = group" assignment.  With
your change, *placep will be pointing at the end of the
array, not the beginning of the array.	(Note the group++
increment in the while loop.)

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3. corrected above error.

all.sh failed with patch v2.  It succeeds with this patch.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 123958 [details] [diff] [review]
patch v3. corrected above error. 

This patch is the smallest version yet.

Comment 18

[Wan-Teh Chang]

Comment on attachment 123958 [details] [diff] [review]
patch v3. corrected above error. 

r=wtc.

I'm having second thoughts whether we need that assertion
at all.  I suggested it simply because it was in the
original code.	In the original code, the assertion means
we can dereference 'placep' without checking.  In the new
code, we dereference 'placep' only if it is not NULL, so
it is not obvious what the assertion means.

What do you think, Nelson?

Comment 19

[Wan-Teh Chang]

Comment on attachment 123958 [details] [diff] [review]
patch v3. corrected above error. 

Requesting mozilla 1.4 approval.  This bug is the underlying
cause of bug 204555, a mozilla 1.4 blocker.  The risk of this
patch is low.

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 123958 [details] [diff] [review]
patch v3. corrected above error. 

a=sspitzer

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Fic checked in as rev 1.26 on trunk.
Will mark fixed when checked in on NSS 3.8 branch.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Fix checked in on NSS_3_8_BRANCH as rev  1.19.26.2
marking fixed in NSS 3.8.1.

Comment 23

[Wan-Teh Chang]

Comment on attachment 123958 [details] [diff] [review]
patch v3. corrected above error. 

I checked in this patch on MOZILLA_1_4_BRANCH for mozilla 1.4 final.

Comment 24

[Wan-Teh Chang]

Patch checked into NSS_3_7_BRANCH for NSS 3.7.7 Beta 1.

Comment 25

[Wan-Teh Chang]

Patch checked in on the NSS_3_4_BRANCH (3.4.4).