Bug 364323 (CVE-2007-0009)

SSL2 server vulnerability reported

RESOLVED FIXED in 3.11.5

Status

NSS
Libraries
P1
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

({fixed1.8.0.10, fixed1.8.1.2})

3.11.5
fixed1.8.0.10, fixed1.8.1.2
Bug Flags:
blocking1.8.1.2 +
blocking1.8.0.10 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] server-only [needs testcase] details in comment 1 private to give servers time to upgrade)

Attachments

(1 attachment)

This vulnerability was reported to security@mozilla.org by email from 
vendor-disclosure@idefense.com, bearing the subject IDEF1994.  It says:
"This vulnerability was reported to iDefense by regenrecht."

According to the report, NSS "may allow an attacker to cause a stack-based 
buffer overflow and execute arbitrary code on the affected application."

The described attack occurs in SSL2 processing, in code executed only by
processes acting as SSL2 servers.  This vulnerability does not affect clients.
Disabling SSL2 is believed to be a complete defense.  Products with SSL2 
disabled are believed to be not at risk from this vulnerability.

As of this writihg, the bug is not yet confirmed.  More details will be 
provided if/when it is confirmed.
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Whiteboard: [sg:critical?] server-only
(Assignee)

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → 3.11.5
(Assignee)

Comment 2

11 years ago
Created attachment 249156 [details] [diff] [review]
patch v1 (checked in)

This patch includes the patch for bug 364319.  

Among other changes, this patch changes some variable names to 
try to use them consistently.  In particular, names ending in Bits are 
used for key lengths in bits, while names ending in Size or Len refer 
to lengths in bytes.
Attachment #249156 - Flags: superreview?(wtchang)
Attachment #249156 - Flags: review?(rrelyea)

Comment 3

11 years ago
Comment on attachment 249156 [details] [diff] [review]
patch v1 (checked in)

r=wtc.  Some suggested changes.

>+    sslServerCerts *  sc   = ss->serverCerts + kt_rsa;

This line doesn't need to be moved.

>     unsigned int      keySize;

You may want to add a comment for this variable.  Or you can
do without this variable (see below).

>+    int               modulusLen;

Why isn't this unsigned int?

>+    PRUint16          allowed;  /* cipher kinds enabled and allowed by policy */

Nit: may want to declare this as unsigned int to be consistent
with two other instances of this variable in this file.

>+    if (!(allowed & (1 << cipher))) {

Nit: change 1 to 1U to be consistent with two other instances
of similar code in this file.

>+    keySize = ssl_Specs[cipher].keyLen;
>+    if (keyBits != keySize * BPB) {

We can do without the variable 'keySize' by rewriting this as

      if (keyBits != ssl_Specs[cipher].keyLen *BPB) {

which looks similar to the checks for ckLen and caLen below.

>+    modulusLen = PK11_GetPrivateModulusLen(sc->SERVERKEY);
>+    if (modulusLen == -1) {
>+	/* XXX If the key is bad, then PK11_PubDecryptRaw will fail below. */
>+	modulusLen = ekLen;
>+    }

I suggest that you restore the comment that you omitted.

Why did you mark this comment XXX?

>+    if (ekLen > modulusLen || ekLen + ckLen < keySize) {

If modulusLen equals -1 above, we set modulusLen to ekLen.  In
that case, the test ekLen > modulusLen is useless, so ekLen can
be arbitrarily large.

So I suggest that we add another check here, e.g.,

      if (ekLen > modulusLen || ekLen > 512 || ekLen + ckLen < keySize) {

(512 bytes = 4096 bits.  I haven't seen a larger RSA modulus used
in practice.  You can also use 1024 instead.)

Can you explain the ekLen + ckLen < keySize test?  Can we use
ekLen + ckLen != keySize instead?

>     /* Cheaply verify that PKCS#1 was used to format the encryption block */
>+    if ((kbuf[0] != 0x00) || (kbuf[1] != 0x02) || (dk[-1] != 0x00)) {

Let's consider not verifying cheaply.
Attachment #249156 - Flags: superreview?(wtchang) → superreview+

Comment 4

11 years ago
Please ignore my previous comment about doing without the
variable 'keySize'.  I was wrong.

Comment 5

11 years ago
>+    modulusLen = PK11_GetPrivateModulusLen(sc->SERVERKEY);
>+    if (modulusLen == -1) {
>+	/* XXX If the key is bad, then PK11_PubDecryptRaw will fail below. */
>+	modulusLen = ekLen;
>+    }

We should be able to just fail if we can't get the modulusLen. It means either the key isn't an RSA key (we should have failed earlier), or the PKCS #11 module is just hosed. Since this is server only code, it's highly unlikely that any PKCS #11 module for a hardware accellerate would not return the modulus for the private key.

Comment 6

11 years ago
Comment on attachment 249156 [details] [diff] [review]
patch v1 (checked in)

r+ We should probably add some of the bug comments to the code. In particular why ekLen check was needed. The PKCS #11 module will check ekLen, but only to make sure it is big enough to hold the modulus.

Also if we are worried about the modulus check, it's OK to call PK11PubDecryptRaw with a too big ekLen, You can use the returned ddLen value to determine the real modulus size. Our current check of moduluslen and ddLen is a sanity check on the underlying PKCS #11 module (That is, it doesn't matter what data is passed by the attacker, the PKCS #11 module will always return 'moduluslen' data for ddLen unless the module itself is broken).
Attachment #249156 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 7

11 years ago
Patch committed. 

Improve checking of received SSL2 records.
Bug 364319, bug 364323. r=rrelyea, wtchang

on trunk:
Checking in sslcon.c; new revision: 1.35; previous revision: 1.34

On branch:
Checking in sslcon.c; new revision: 1.28.2.7; previous revision: 1.28.2.6

Wan-Teh, to answer some of your questions:

modulusLen is signed because it is assigned a value from function 
PK11_GetPrivateModulusLen which may return -1, and we test for the value -1.

I incorporated most of your other suggested changes, including testint ekLen
for an upper bound of 1024 (the limit currently imposed in freebl).
I'm willing to include a stronger check on the RSA block format, but have 
not yet done so.
(Assignee)

Updated

11 years ago
Attachment #249156 - Attachment description: patch v1 → patch v1 (checked in)
The Mozilla team would like this fix (or more specifically the fix for bug 364319) in our upcoming 2.0.0.2 and 1.5.0.10 releases. Now, however, we've changed to pulling NSS by NSS-approved tags rather than just adding patches to a client branch -- overall a good thing, but are there plans to create a supported NSS tag with this patch included in the near future?
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Alias: CVE-2007-0009
Summary: SSL2 server vulnerability reported → CVE-2007-0009 SSL2 server vulnerability reported

Updated

11 years ago
Summary: CVE-2007-0009 SSL2 server vulnerability reported → SSL2 server vulnerability reported

Comment 9

11 years ago
Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
See bug 363070 comment 13.
Keywords: fixed1.8.0.10, fixed1.8.1.2

Updated

11 years ago
Whiteboard: [sg:critical?] server-only → [sg:critical?] server-only [needs testcase]
We should be able to remove the security sensitive flag on this bug other than comment 1, any disagreement?
Whiteboard: [sg:critical?] server-only [needs testcase] → [sg:critical?] server-only [needs testcase] details in comment 1 private to give servers time to upgrade
Group: security
You need to log in before you can comment on or make changes to this bug.