491918 - Infinite recursion when encoding a NSS enveloped/digested data

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

Attachment: Reproducer, see comment 0

Forwarded from https://bugzilla.redhat.com/show_bug.cgi?id=499444

Attempting to encode a CMS message that contains enveloped (digested (data))
causes infinite recursion and stack overflow.

To reproduce, compile the attached file with
   gcc $(pkg-config --cflags --libs nss glib-2.0) repro.c
and run
   ./a.out a/path/to/a/certificate.pem > x
This will correctly create an enveloped message.

If you redefine DIGESTED in the file to "1" and recompile,
   ./a.out a/path/to/a/certificate.pem > x
will crash with a stack overflow.

AFAICS this is going on:
* When secasn1e.c finishes encoding the contentType field of
NSSCMSEncryptedContentInfoTemplate, sec_asn1e_next_in_sequence() gets called.
* ...next_in_sequence does the "after" field notification before changing any
ASN1 state.
* The "after" notification ends up in nss_cms_before_data(), which creates a
completely new encoding context for the "child" digested_data, and starts
encoding it.
* Encoding the "header" of digested_data (algorithm ID, nested raw data OID)
requires more than 16 bytes, so nss_cms_encoder_work_data() will get the
encrypted data and call SEC_ASN1EncoderUpdate() with it in the enveloped_data
context.
* As noted above, the ASN1 context was not changed before calling the "after"
field notification of contentType, so SEC_ASN1EncoderUpdate() again thinks the
"contentType" field was just finished encoding, calls the "after" notification
again, and the whole process (recursively) repeats.

See also bug 491914, a different bug related to the calling
SEC_ASN1EncoderUpdate() from within a notifier called
sec_asn1e_next_in_sequence ().

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Kai, I can't tell if the analysis above is yours or the unnamed original 
reporter's analysis.  

The shortest advice I can give is to study the NSS test program cmsutil to 
see how it does this stuff.  It does this correctly, and does not experience
infinite loops.  If the bug reporter's program is trying to do something
that cmsutil does not attempt, please advise what that is.

I wonder if the writer of the analysis is aware that, when encoding encrypted
(that is, enveloped) data, there are TWO completely separate instances of the
ASN.1 encoder running simultaneously; one encoding the input to the encryptor
and one encoding the output of the encryptor.  If one is not aware of this,
it is easy to get confused about which of the two encoders is being watched,
and form incorrect conclusions about how it is working.

Comment 2

[Miloslav Trmač]

I'm the original reporter, and it is my analysis.

cmsutil does not implement digested data at all, so it also doesn't implement the nesting of "digested data" inside an "enveloped" wrapper.  Thunderbird does not implement digested data either.

I know there are two encoders; the problem is that during a notification from the "outer" encoder, the "inner" encoder is started and the output of the "inner" encoder" is fed back into the "outer" encoder while still in the process of notification, when the "outer" encoder is not expecting this.

Comment 3

[Robert Relyea]

Attachment: Patch supplied by nalin (not yet poperly formated.

Unreviewed, unformated patch for this issue.

Comment 4

[Nalin Dahyabhai]

(In reply to comment #3)
> Created attachment 502956 [details] [diff] [review]
> Patch supplied by nalin (not yet poperly formated.
> 
> Unreviewed, unformated patch for this issue.

Yeah, there are some things it still doesn't get right (the case where the message contains just data, I think), but emitting child data at the right point is the important part.

Comment 5

[Nalin Dahyabhai]

Attachment: test case (when invoked with -e -D -d dbdir -n recipient_nickname)

The test program I'm using for building and parsing messages.

Comment 6

[Nalin Dahyabhai]

Attachment: proposed patch

Fixes encoding of one CMS structure inside of another, and the decoder losing pointers to nested CMS structures.

Comment 7

[Robert Relyea]

Attachment: Nalin's latest patch for this issue

Comment 8

[Robert Relyea]

Comment on attachment 508610 [details] [diff] [review]
Nalin's latest patch for this issue

r+ rrelyea

Comment 9

[Robert Relyea]

Trunk;

Checking in cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.10; previous revision: 1.9
done

Branch:

cvs commit: Examining .
Checking in cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.6.66.4; previous revision: 1.6.66.3
done