263596 - cmsutil segfaults when cert doesn't contain email address

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Mike jackson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3

The failure occurs because the cert identified by the nickname
[e-mail] does not have an email address in the cert.
NSS looks for the email address in the cert, and finds none.

CERT_FindSMimeProfile calls PK11_FindSMimeProfile with a NULL pointer
for the emailAddr argument, and the latter function does not check
the pointer for NULL before calling strlen with it.

Reproducible: Always
Steps to Reproduce:

echo foobar > passwd

dd if=/dev/urandom of=rand.seed count=1

certutil -N -f passwd -d .

certutil -S -s "cn=netauth ca,dc=netauth,dc=com" -n "netauth.com" \
    -f passwd -z rand.seed -x -t "C,C,C" -d .

certutil -R -s "cn=jimi hendrix,ou=people,dc=netauth,dc=com" \
    -z rand.seed -f passwd -o jimi.req -d .

certutil -C -i jimi.req -o jimi.crt -f passwd -z rand.seed \
    -c netauth.com -d .

certutil -A -n [e-mail] -f passwd -t "P,P,P" -i jimi.crt -d .

certutil -R -s "cn=tom jones,ou=people,dc=netauth,dc=com" \
    -z rand.seed -f passwd -o tom.req -d .

certutil -C -i tom.req -o tom.crt -f passwd -z rand.seed \
    -c netauth.com -d .

certutil -A -n [e-mail] -f passwd -t "P,P,P" -i tom.crt -d .

cmsutil -E -r [e-mail] -i jimi.txt -d . -p foobar -o jimi.env

Actual Results:  
cmsutil -E -r [e-mail] -i jimi.txt -d . -p foobar -o jimi.env
Segmentation fault (core dumped)

Expected Results:  
cmsutil would create a CMS enveloped data message for [e-mail], jimi.env

cmsutil segfaults

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Thanks for reporting this, and quoting my analysis.
I reproduced it on Windows. It's platform independent.
I used a single line of text for jimi.txt.   Contents were not important.

Another oddity I found, and plan to fix, is that PK11_FindSMimeProfile sets
error SEC_ERROR_NO_KRL if it cannot find any slot that contains the 
requested SMIME profile.  The code contains a comment about looping over
fortezza tokens, which seems mistaken.

I'm targetting this bug for 3.10, but am willing to target it to 3.9.3 RTM 
if Julien agrees.

My proposed fix is to change both CERT_FindSMimeProfile and
PK11_FindSMimeProfile to detect NULL (or empty) emailAddr strings.
CERT_FindSMimeProfile will return NULL, which will cause smime_choose_cipher in
libsmime to do the right thing in the absense of a profile.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Changing component to libraries, since the bugs are in the shared libs.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Patch adds two missing tests for invalid/unexpected inputs,
and fixes one comment. 

Patch does not change the error code returned when no Profile
is found.  We don't have an error code for that situation.
The existing error code is aguably as good as any, until we 
get an error code that means "Profile not found" or just 
"requested thing not found".

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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

Wan-Teh please review.

Comment 5

[Wan-Teh Chang]

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

r=wtc.

In certdb/stanpcertdb.c, do you think we should test
for !cert->emailAddr || !cert->emailAddr[0] in
__CERT_AddTempCertToPerm?

In pk11wrap/pk11nobj.c, we have

>-	/* loop through all the fortezza tokens */
>+	/* loop through all the slots */

Should this be "loop through all the tokens" instead?

Note that there are other "loop through all the fortezza tokens"
comments in lib/pk11wrap/*.c.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, thanks for the quick review.  Regarding the question:

> In certdb/stanpcertdb.c, do you think we should test
> for !cert->emailAddr || !cert->emailAddr[0] in
> __CERT_AddTempCertToPerm?

I think you are asking if CERT_AddTempCertToPerm should require that 
a cert have an email address in order to save it in the cert DB.
Only certs that are going to be used for email need email addresses.
SSL certs, object signing certs and CA certs generally do not.
But maybe you are asking something else. (if so, please ask again)

Perhaps you are suggesting that another error code would be better.
would SEC_ERROR_NO_EMAIL_CERT be a better choice?  
consider the recommended error text for that case.

There may be many places where this comment about fortezza cards needs
to be fixed.  I suspect it was a copy-n-paste error that was never fixed.
Please ask Bob about that.  

Comment 7

[Wan-Teh Chang]

Nelson, please ignore the question you responded
to in comment 6.

I was doing a mechanical search for all other
functions in stanpcertdb.c that use cert->emailAddr.
__CERT_AddTempCertToPerm is such a function, and
I asked that question without paying attention to
what __CERT_AddTempCertToPerm does.

I was not suggesting that another error code
would be better, but I like SEC_ERROR_NO_EMAIL_CERT.

Bob, could you answer my second question in comment
5?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Checking in lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.67; previous revision: 1.66
done
Checking in lib/pk11wrap/pk11nobj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11nobj.c,v  <--  pk11nobj.c
new revision: 1.3; previous revision: 1.2
done

Comment 9

[Frank Wein [:mcsmurf]]

*** Bug 279138 has been marked as a duplicate of this bug. ***