364323 - (CVE-2007-0009) SSL2 server vulnerability reported

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

This vulnerability was reported to [e-mail] by email from 
[e-mail], 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.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Wan-Teh Chang]

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

Comment 5

[Robert Relyea]

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

[Robert Relyea]

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).

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Daniel Veditz [:dveditz]]

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?

Comment 9

[Wan-Teh Chang]

Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
See bug 363070 comment 13.

Comment 10

[Daniel Veditz [:dveditz]]

We should be able to remove the security sensitive flag on this bug other than comment 1, any disagreement?