466197 - CERT_DecodeCertFromPackage may modify its 'certbuf' input buffer

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Wan-Teh Chang]

Dan Kegel asked if CERT_DecodeCertFromPackage should take a
const char * 'certbuf' input parameter.

I looked into this and found two difficulties.  Both problems
are in the CERT_DecodeCertPackage function called  by
CERT_DecodeCertFromPackage.

1., CERT_DecodeCertPackage uses our SECItem structures to
point to portions of the input buffer.  Unfortunately
SECItem's 'data' pointer is a  "unsigned char*" pointer,
and using 'const SECItem' won't help.

2. I found one code path where CERT_DecodeCertPackage
stores a null character '\0' in the input buffer so it
can pass a shorter string to ATOB_AsciiToData:
http://mxr.mozilla.org/security/source/security/nss/lib/pkcs7/certread.c#384
 
381     if ( certbegin && certend ) {
382         unsigned int binLen;
383 
384         *certend = 0;
385         /* convert to binary */
386         bincert = ATOB_AsciiToData(certbegin, &binLen);
387         if (!bincert) {
388             rv = SECFailure;
389             goto loser;
390         }
391 
392         /* now recurse to decode the binary */
393         rv = CERT_DecodeCertPackage((char *)bincert, binLen, f, arg);
394         
395     } else {

But it doesn't restore that character afterwards.  We
should at least fix the function so that it restores the
input buffer on return.  We should also document (in cert.h
at least) that CERT_DecodeCertFromPackage and
CERT_DecodeCertPackage may modify its input buffer.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Well, this function should never modify its input buffer.  
It should make a copy and modify that.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.