The default bug view has changed. See this FAQ.

nsNSSCertificateDB::FindCertByDBKey() crashes on invalid input

RESOLVED FIXED in mozilla1.9beta1

Status

()

Core
Security: PSM
--
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Kaspar Brand, Assigned: Kaspar Brand)

Tracking

({crash, verified1.8.1.8})

Trunk
mozilla1.9beta1
crash, verified1.8.1.8
Points:
---
Bug Flags:
blocking1.8.1.8 +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 277279 [details] [diff] [review]
Improve input validation in FindCertByDBKey()

As some sort of follow-up to bug 379190, there are actually more things which can go wrong in nsNSSCertificateDB::FindCertByDBKey() - what it expects as input is, in this sequence:

[1] 4 bytes encoding the moduleID (currently unused, so always set to zero)

[2] 4 bytes encoding the slotID (also unused, currently)

[3] 4 bytes encoding the length of the serial number

[4] 4 bytes encoding the length of the DER encoded issuer DN

[5] the serial number, with the length specified by [3]

[6] the DER encoded issuer DN, with the length specified by [4]

However, FindCertByDBKey() doesn't do any validation on this input (other than checking for an empty aDBkey argument, as added with the fix from bug 379190), so it's pretty trivial to crash PSM with bogus input - just try evaluating

Components.classes["@mozilla.org/security/x509certdb;1"].getService(Components.interfaces.nsIX509CertDB).findCertByDBKey("aa",null)

in the Error Console, and you get a crash.

I propose the attached patch, which adds a few length checks before stuffing the values into a CERTIssuerAndSN struct and calling CERT_FindCertByIssuerAndSN().

In the first case, I took the liberty of changing NS_ERROR_FAILURE to NS_ERROR_INVALID_ARG - which seems more appropriate to me, when looking at the descriptions from http://lxr.mozilla.org/mozilla/source/xpcom/base/nsError.h.
Attachment #277279 - Flags: review?(kengert)
(Assignee)

Updated

10 years ago
Blocks: 278689
Comment on attachment 277279 [details] [diff] [review]
Improve input validation in FindCertByDBKey()

Bob, I think this is the patch for which you were calling in bug 392208 comment 10.  Please review it.
Attachment #277279 - Flags: review?(rrelyea)

Comment 2

10 years ago
Comment on attachment 277279 [details] [diff] [review]
Improve input validation in FindCertByDBKey()

r+ with one potential fix.

if dummy is not null and keyItem.len is < NS_NSS_LONG*4 then we will leak keyItem.data (better than crashing..)

The simple fix is to set keyItem.data to NULL before the decode call and do a PR_FREEIF() in the if (!dummy ||... ) case.
Attachment #277279 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 3

10 years ago
Created attachment 277669 [details] [diff] [review]
patch v2

Also free keyItem.data if len is < NS_NSS_LONG*4, as suggested by Bob (keyItem.data is already initialized with nsnull in the declaration).
Attachment #277279 - Attachment is obsolete: true
Attachment #277669 - Flags: superreview?(kengert)
Attachment #277669 - Flags: review?(rrelyea)
Attachment #277279 - Flags: review?(kengert)

Comment 4

10 years ago
Comment on attachment 277669 [details] [diff] [review]
patch v2

r+ rrelyea.

Looks good.
Attachment #277669 - Flags: review?(rrelyea) → review+
(Assignee)

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Kai, please SR the latest patch for this bug.
If it needs approval, please request that.
Flags: blocking1.9?

Updated

10 years ago
Attachment #277669 - Flags: superreview?(kengert)
Attachment #277669 - Flags: superreview+
Attachment #277669 - Flags: approval1.9?
Attachment #277669 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Keywords: crash → checkin-needed
Is this patch appropriate for the 1.8 branch as well?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.7?
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> Is this patch appropriate for the 1.8 branch as well?

Yes, in my opinion. I did a quick test with Tb 2.0.0.6 - if applied, the patch will successfully prevent the crash reported in bug 392208, e.g. (or the one described in comment #0 above, fwiw).
Flags: blocking1.8.1.7? → blocking1.8.1.7+
Assignee: kengert → mozbugzilla
Keywords: crash
Attachment #277669 - Flags: approval1.9+ → approval1.9?
Attachment #277669 - Flags: approval1.8.1.7?
Kai: is this something you can check in for Kaspar?
(In reply to comment #8)
> Kai: is this something you can check in for Kaspar?

Needs approval1.9 and approval1.8.1.7 first though...

Comment 10

10 years ago
I will check it in once we have approvals.
Comment on attachment 277669 [details] [diff] [review]
patch v2

a=bzbarsky
Attachment #277669 - Flags: approval1.9? → approval1.9+
Checking in security/manager/ssl/src/nsNSSCertificateDB.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp,v  <--  nsNSSCertificateDB.cpp
new revision: 1.31; previous revision: 1.30
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Comment on attachment 277669 [details] [diff] [review]
patch v2

approved for 1.8.1.8, a=dveditz for release-drivers
Attachment #277669 - Flags: approval1.8.1.8? → approval1.8.1.8+
MOZILLA_1_8_BRANCH:

Checking in security/manager/ssl/src/nsNSSCertificateDB.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp,v  <--  nsNSSCertificateDB.cpp
new revision: 1.15.20.12; previous revision: 1.15.20.11
done
Keywords: fixed1.8.1.8
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.8pre) Gecko/20071003 BonEcho/2.0.0.8pre

I now get this js error in the console, with the code in comment 0:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIX509CertDB.findCertByDBKey]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: javascript: Components.classes["@mozilla.org/security/x509certdb;1"].getService(Components.interfaces.nsIX509CertDB).findCertByDBKey("aa",null) :: <TOP_LEVEL> :: line 1"  data: no]

With a 2.0.0.6 build, I crash when clicking on the evaluate button a few times.
Keywords: fixed1.8.1.8 → verified1.8.1.8

Updated

10 years ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.