The default bug view has changed. See this FAQ.

SEC_ASN1DecodeItem returns NULL and SECSuccess

RESOLVED FIXED in 3.8.1

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

({fixed1.4})

3.8.1
fixed1.4
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [3.7.7][3.4.4])

Attachments

(1 attachment, 2 obsolete attachments)

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

15 years ago
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?
Assignee: wtc → jpierre
Priority: -- → P2
Target Milestone: --- → 3.7

Comment 2

15 years ago
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.
(Assignee)

Comment 3

15 years ago
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

15 years ago
OK, you are right. This is a bug.  It is important to
differentiate between an empty set and a missing set.
(Assignee)

Comment 5

15 years ago
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

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

Comment 7

15 years ago
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
(Assignee)

Comment 8

14 years ago
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
(Assignee)

Comment 9

14 years ago
Taking bug.  I believe that bug 204555 is another manifestation of this bug.
Assignee: jpierre → nelsonb
(Assignee)

Comment 10

14 years ago
Created attachment 123860 [details] [diff] [review]
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.
(Assignee)

Comment 11

14 years ago
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.
Attachment #123860 - Flags: review?(wtc)

Comment 12

14 years ago
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.
Attachment #123860 - Flags: superreview?(relyea)
Attachment #123860 - Flags: review?(wtc)
Attachment #123860 - Flags: review+

Comment 13

14 years ago
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.
(Assignee)

Comment 14

14 years ago
Created attachment 123942 [details] [diff] [review]
patch v2 incorporates wtc's review comments
(Assignee)

Updated

14 years ago
Attachment #123860 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #123942 - Flags: superreview?(relyea)
Attachment #123942 - Flags: review?(wtc)

Comment 15

14 years ago
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.)
Attachment #123942 - Flags: review?(wtc) → review-
(Assignee)

Comment 16

14 years ago
Created attachment 123958 [details] [diff] [review]
patch v3. corrected above error. 

all.sh failed with patch v2.  It succeeds with this patch.
Attachment #123942 - Attachment is obsolete: true
(Assignee)

Comment 17

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

This patch is the smallest version yet.
Attachment #123958 - Flags: review?(wtc)

Comment 18

14 years ago
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?
Attachment #123958 - Flags: review?(wtc) → review+

Comment 19

14 years ago
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.
Attachment #123958 - Flags: approval1.4?
Comment on attachment 123958 [details] [diff] [review]
patch v3. corrected above error. 

a=sspitzer
Attachment #123958 - Flags: approval1.4? → approval1.4+
(Assignee)

Comment 21

14 years ago
Fic checked in as rev 1.26 on trunk.
Will mark fixed when checked in on NSS 3.8 branch.
Blocks: 204555
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → 3.8.1
(Assignee)

Comment 22

14 years ago
Fix checked in on NSS_3_8_BRANCH as rev  1.19.26.2
marking fixed in NSS 3.8.1.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 23

14 years ago
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.

Updated

14 years ago
Keywords: fixed1.4

Comment 24

14 years ago
Patch checked into NSS_3_7_BRANCH for NSS 3.7.7 Beta 1.
Whiteboard: [3.7.7]
(Assignee)

Updated

14 years ago
Blocks: 208996
(Assignee)

Updated

14 years ago
Blocks: 207740

Comment 25

14 years ago
Patch checked in on the NSS_3_4_BRANCH (3.4.4).
Whiteboard: [3.7.7] → [3.7.7][3.4.4]

Updated

14 years ago
Attachment #123942 - Flags: superreview?(rrelyea0264) → superreview+

Updated

14 years ago
Attachment #123860 - Flags: superreview?(rrelyea0264) → superreview+

Updated

14 years ago
Blocks: 211049
You need to log in before you can comment on or make changes to this bug.