392780 - nsNSSCertificateDB::FindCertByDBKey() crashes on invalid input

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kaspar]

Attachment: 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)]

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]

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]

Attachment: 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]

Comment on attachment 277669 [details] [diff] [review]
patch v2

r+ rrelyea.

Looks good.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Kai, please SR the latest patch for this bug.
If it needs approval, please request that.

Comment 6

[Daniel Veditz [:dveditz] back 2026-04-08]

Is this patch appropriate for the 1.8 branch as well?

Comment 7

[Kaspar]

(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] back 2026-04-08]

Kai: is this something you can check in for Kaspar?

Comment 9

[Reed Loden [:reed]]

(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:]]

I will check it in once we have approvals.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277669 [details] [diff] [review]
patch v2

a=bzbarsky

Comment 12

[Reed Loden [:reed]]

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] back 2026-04-08]

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]]

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 (dead)]

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.