Last Comment Bug 392780 - nsNSSCertificateDB::FindCertByDBKey() crashes on invalid input
: nsNSSCertificateDB::FindCertByDBKey() crashes on invalid input
Status: RESOLVED FIXED
: crash, verified1.8.1.8
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9beta1
Assigned To: Kaspar Brand
:
:
Mentors:
Depends on:
Blocks: 278689
  Show dependency treegraph
 
Reported: 2007-08-19 06:07 PDT by Kaspar Brand
Modified: 2007-10-04 14:40 PDT (History)
5 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Improve input validation in FindCertByDBKey() (1.99 KB, patch)
2007-08-19 06:07 PDT, Kaspar Brand
rrelyea: review+
Details | Diff | Splinter Review
patch v2 (2.03 KB, patch)
2007-08-21 22:38 PDT, Kaspar Brand
rrelyea: review+
kaie: superreview+
dveditz: approval1.8.1.8+
bzbarsky: approval1.9+
Details | Diff | Splinter Review

Description Kaspar Brand 2007-08-19 06:07:09 PDT
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.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2007-08-21 17:45:26 PDT
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.
Comment 2 Robert Relyea 2007-08-21 17:58:21 PDT
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.
Comment 3 Kaspar Brand 2007-08-21 22:38:15 PDT
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).
Comment 4 Robert Relyea 2007-08-22 10:51:22 PDT
Comment on attachment 277669 [details] [diff] [review]
patch v2

r+ rrelyea.

Looks good.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2007-08-31 13:01:28 PDT
Kai, please SR the latest patch for this bug.
If it needs approval, please request that.
Comment 6 Daniel Veditz [:dveditz] 2007-09-06 16:59:18 PDT
Is this patch appropriate for the 1.8 branch as well?
Comment 7 Kaspar Brand 2007-09-06 22:12:29 PDT
(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).
Comment 8 Daniel Veditz [:dveditz] 2007-09-11 10:33:42 PDT
Kai: is this something you can check in for Kaspar?
Comment 9 Reed Loden [:reed] (use needinfo?) 2007-09-11 10:36:48 PDT
(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 Kai Engert (:kaie) 2007-09-12 11:44:46 PDT
I will check it in once we have approvals.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2007-09-16 12:48:03 PDT
Comment on attachment 277669 [details] [diff] [review]
patch v2

a=bzbarsky
Comment 12 Reed Loden [:reed] (use needinfo?) 2007-09-17 13:46:52 PDT
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
Comment 13 Daniel Veditz [:dveditz] 2007-09-24 11:59:55 PDT
Comment on attachment 277669 [details] [diff] [review]
patch v2

approved for 1.8.1.8, a=dveditz for release-drivers
Comment 14 Reed Loden [:reed] (use needinfo?) 2007-09-27 15:12:42 PDT
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
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-04 14:39:14 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.