cmsutil segfaults when cert doesn't contain email address

RESOLVED FIXED in 3.10

Status

P2
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: mj, Assigned: nelson)

Tracking

({crash})

3.9.2
3.10
crash

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

14 years ago
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
tom@netauth.com 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 jimi@netauth.com -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 tom@netauth.com -f passwd -t "P,P,P" -i tom.crt -d .

cmsutil -E -r tom@netauth.com -i jimi.txt -d . -p foobar -o jimi.env

Actual Results:  
cmsutil -E -r tom@netauth.com -i jimi.txt -d . -p foobar -o jimi.env
Segmentation fault (core dumped)

Expected Results:  
cmsutil would create a CMS enveloped data message for tom@netauth.com, jimi.env

cmsutil segfaults

Updated

14 years ago
Keywords: crash
(Assignee)

Comment 1

14 years ago
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.
Assignee: wchang0222 → nelson
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → 3.10
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Hardware: PC → All
Version: unspecified → 3.9.2
(Assignee)

Comment 2

14 years ago
Changing component to libraries, since the bugs are in the shared libs.
Component: Tools → Libraries
(Assignee)

Comment 3

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

Comment 4

14 years ago
Comment on attachment 167325 [details] [diff] [review]
patch v1

Wan-Teh please review.
Attachment #167325 - Flags: review?(wchang0222)

Comment 5

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

Comment 6

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

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

Comment 8

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 9

14 years ago
*** Bug 279138 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.