Last Comment Bug 364319 - (CVE-2007-0008) SSL2 client vulnerability in non-DEBUG builds
(CVE-2007-0008)
: SSL2 client vulnerability in non-DEBUG builds
Status: RESOLVED FIXED
[sg:critical] [needs testscase]
: fixed1.8.0.10, fixed1.8.1.2
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4
: All All
: P1 critical (vote)
: 3.11.5
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-18 23:56 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2007-02-23 13:28 PST (History)
7 users (show)
dveditz: blocking1.8.1.2+
dveditz: blocking1.8.0.10+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (checked in) (955 bytes, patch)
2006-12-19 00:11 PST, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
wtc: superreview+
Details | Diff | Review
patch v2 (1.93 KB, patch)
2006-12-19 14:41 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2006-12-18 23:56:33 PST
This vulnerability was reported to security@mozilla.org by email from 
vendor-disclosure@idefense.com, 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) 2006-12-18 23:58:31 PST
Taking.  
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-12-19 00:11:02 PST
Created attachment 249088 [details] [diff] [review]
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 2006-12-19 07:49:35 PST
Comment on attachment 249088 [details] [diff] [review]
patch v1 (checked in)

r=wtc.
Comment 4 Wan-Teh Chang 2006-12-19 08:24:19 PST
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) 2006-12-19 14:41:58 PST
Created attachment 249159 [details] [diff] [review]
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 2006-12-19 21:05:32 PST
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 2006-12-20 11:19:26 PST
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 2006-12-21 17:27:45 PST
Comment on attachment 249088 [details] [diff] [review]
patch v1 (checked in)

r+ = relyea
Comment 9 Nelson Bolyard (seldom reads bugmail) 2007-01-02 21:46:22 PST
patch v1 checked in.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2007-01-03 03:45:49 PST
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 2007-01-18 14:20:00 PST
Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
See bug 363070 comment 13.

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