364319 - (CVE-2007-0008) SSL2 client vulnerability in non-DEBUG builds

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 IDEF1973.  It says:
"This vulnerability was reported to iDefense by regenrecht."

An SSL server cert with an RSA public key that is too small to encrypt
the entire SSL2 "Master Secret" will cause optimized (not DEBUG) NSS 
builds to crash, and/or execute attacker-supplied code (on some platforms).
Disabling SSL2 is a complete defense.  Products with SSL2 disabled are 
believed to be invulnerable.

The bug has been present in NSS since at least NSS 3.4 and probably longer.
Function ssl_FormatSSL2Block starts with an assertion that the modulus is
long enough to properly encode the data to be encrypted, but that only has
effect in DEBUG builds.  In non-DEBUG builds, when the modulus is too short,
the code overruns the end of the too-short buffer.  The fix is to detect 
too-short modulii in all builds, not just DEBUG builds.  This probably 
should not be an ASSERT at all, but just a run time check.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Taking.  

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 (checked in)

Bob, Wan-Teh, please review.

Others: remember that FF2 already has SSL2 disabled, and therefore is immune.

Comment 3

[Wan-Teh Chang]

Comment on attachment 249088 [details] [diff] [review]
patch v1 (checked in)

r=wtc.

Comment 4

[Wan-Teh Chang]

I reviewed all the other assertions in sslcon.c.
Let's do something about this closely related assertion in sslcon.c:

    PORT_Assert((modulusLen - rek.len) > 12);

Note that > 12 is the same as >= 13.  13 is greater than
(3 + RSA_BLOCK_MIN_PAD_LEN) by 2.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

Incorporates Wan-Teh's suggestion (I think).

Wan-Teh, is this what you hand in mind?
If so, please mark patch v1 obsolete and ask Bob to review this one.

Comment 6

[Wan-Teh Chang]

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

Nelson, if you agree that the "> 12" in that assertion is wrong
and should be ">= 11", we should simply remove that assertion.
The check you added in this patch is redundant; it's already
performed in the ssl_FormatSSL2Block call above.  Thanks.

Comment 7

[Wan-Teh Chang]

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

I have an idea for how to deal with the assertion in
ssl2_ClientSetupSessionCypher: move the following code,
which sets up the padding for version 2 rollback detection,
to ssl_FormatSSL2Block.  This requires passing 'ss' or
a boolean flag as a new argument to ssl_FormatSSL2Block
though.

    /* Set up the padding for version 2 rollback detection. */
    /* XXX We should really use defines here */
    if (ss->opt.enableSSL3 || ss->opt.enableTLS) {
        PORT_Assert((modulusLen - rek.len) > 12);
        PORT_Memset(eblock + modulusLen - rek.len - 8 - 1, 0x03, 8);
    }

If this code is in ssl_FormatSSL2Block, it'll be clear
that the assertion is redundant and can be simply removed.

Comment 8

[Robert Relyea]

Comment on attachment 249088 [details] [diff] [review]
patch v1 (checked in)

r+ = relyea

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

patch v1 checked in.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

The checkin was

> 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

Comment 11

[Wan-Teh Chang]

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