Closed Bug 263596 Opened 20 years ago Closed 20 years ago

cmsutil segfaults when cert doesn't contain email address

Categories

(NSS :: Libraries, defect, P2)

3.9.2
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mj, Assigned: nelson)

References

Details

(Keywords: crash)

Attachments

(1 file)

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
Keywords: crash
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
Status: NEW → ASSIGNED
Hardware: PC → All
Version: unspecified → 3.9.2
Changing component to libraries, since the bugs are in the shared libs.
Component: Tools → Libraries
Attached patch patch v1Splinter Review
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 on attachment 167325 [details] [diff] [review]
patch v1

Wan-Teh please review.
Attachment #167325 - Flags: review?(wchang0222)
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+
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.  
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?
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
Closed: 20 years ago
Resolution: --- → FIXED
*** 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.

Attachment

General

Created:
Updated:
Size: