Closed Bug 1228555 Opened 6 years ago Closed 3 years ago

Remove support for SSLv2

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox45 affected)

RESOLVED FIXED
Tracking Status
firefox45 --- affected

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 10 obsolete files)

5.00 MB, patch
ttaubert
: review+
ttaubert
: checked-in+
Details | Diff | Splinter Review
40.97 KB, patch
ttaubert
: review+
ttaubert
: checked-in+
Details | Diff | Splinter Review
No description provided.
Status: NEW → ASSIGNED
Depends on: 1229787
Depends on: 1229899
Depends on: 1229942
Depends on: 1230084
Depends on: 1230086
Patch at: https://codereview.appspot.com/286030043/

This removes quite a lot of code. I'll try to outline some major changes here:

1) The directory tests/pkcs11/ was removed as those tests seem of rather historic value. They haven't been updated/touched in a while and I don't think they are run by default. mt suggested its removal in https://codereview.appspot.com/277190043/

2) We no longer support v2 client hellos.

3) ssl2_ConstructCipherSpecs() and ssl3_ConstructV2CipherSpecsHack() were removed. The code is now unified in ssl3_ConstructCipherSpecs().

4) ssl_Do1stHandshake() was simplified as our handshake code's complexity decreased. I think we can get by without |nextHandshake| and |securityHandshake|. |ss->handshake| is set to 0 only when ssl_GatherRecord1stHandshake() succeeds. I'm not entirely sure if I got the behavior for non-blocking sockets right here, in case of SECWouldBlock. Looking forward to feedback.

There are a few things I didn't remove because I think we can't do that to stay API/ABI backwards compatible. I'd like to be wrong here though, and would happily remove more SSLv2 references if we can. (Or grudgingly re-add code we can't [yet] remove.)
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
I don't like the removal of SSLv2-style Client Hello's.

This breaks support of formerly very widely deployed clients like Java 6 and IE 6. Ones, that still have (limited) support available.

I don't think it is necessary - a SSLv3 Client Hello (or TLSv1.0 without extensions) has exact same functionality and impact on security of connections.

ability to parse just a single, very specific message, coming as the first one in connection shouldn't be very hard to preserve
(In reply to Hubert Kario from comment #2)
> I don't like the removal of SSLv2-style Client Hello's.
> 
> This breaks support of formerly very widely deployed clients like Java 6 and
> IE 6. Ones, that still have (limited) support available.
> 
> I don't think it is necessary - a SSLv3 Client Hello (or TLSv1.0 without
> extensions) has exact same functionality and impact on security of
> connections.

Well, generally it's getting increasingly important to have those extensions.


> ability to parse just a single, very specific message, coming as the first
> one in connection shouldn't be very hard to preserve

It's a lot of code.
Flags: needinfo?(ekr)
> Well, generally it's getting increasingly important to have those extensions.

as long as extensions like Extended Master Secret or Encrypt then MAC are regarded as optional by NSS, TLS Client Hello and SSLv2-style client hello provide the same security guarantees

> It's a lot of code.

SSLv2 in general, yes. Just SSLv2 client hello? I'd be surprised... In python it's something like 30 lines of code.
(In reply to Hubert Kario from comment #5)
> > Well, generally it's getting increasingly important to have those extensions.
> 
> as long as extensions like Extended Master Secret or Encrypt then MAC are
> regarded as optional by NSS, TLS Client Hello and SSLv2-style client hello
> provide the same security guarantees

Not really, because you need extensions to do > SHA-1 signatures for TLS 1.2
and as a practical matter you need them for ECDHE.


> > It's a lot of code.
> 
> SSLv2 in general, yes. Just SSLv2 client hello? I'd be surprised... In
> python it's something like 30 lines of code.

It's almost 200 lines of code, not to mention the weird mechanics for switch-hitting
in the current version:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#8731
(In reply to Eric Rescorla (:ekr) from comment #6)
> Not really, because you need extensions to do > SHA-1 signatures for TLS 1.2
> and as a practical matter you need them for ECDHE.

Neither of which is a problem for implementations that do use SSLv2 hello's

if you want guards against negotiation of TLSv1.2 or ECC with SSLv2 Hello's, I'm fine with that

> > > It's a lot of code.
> > 
> > SSLv2 in general, yes. Just SSLv2 client hello? I'd be surprised... In
> > python it's something like 30 lines of code.
> 
> It's almost 200 lines of code, not to mention the weird mechanics for
> switch-hitting
> in the current version:
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.
> c#8731

which is neither large nor complex, most of it is just code duplicated from SSLv3 hello handler or simple error handling
Clearing n-i until there is a new patch to review.  FWIW, I would rather see two patches, one to remove everything and another to restore the v2-compatible CH.
Flags: needinfo?(martin.thomson)
Yeah, I had two patches initially but then "lost" that separation when rebasing on top of the clang-formatting. I'll pull them apart again before uploading a new revision today or tomorrow.
Attachment #8715898 - Attachment is obsolete: true
New patch up at: https://codereview.appspot.com/286030043/

Re-adding the SSLv2 client hello support and tests with this: https://codereview.appspot.com/287280043
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
So, despite what was claimed, adding a compatible client hello was complex.  This is not surprising.  I've a few suggestions on rietveld, but I think that we're close.
Flags: needinfo?(martin.thomson)
Blocks: 1252795
Attachment #8724737 - Attachment is obsolete: true
New patches up.
Flags: needinfo?(martin.thomson)
Blocks: 1252849
r=mt on both, thanks.
Flags: needinfo?(martin.thomson)
Blocks: 1255758
Keywords: leave-open
Pushed a small follow-up to make Windows builders happy:

https://hg.mozilla.org/projects/nss/rev/52f57b0c5444
And another follow-up for Windows (my VM doesn't reproduce these failures unfortunately...):

https://hg.mozilla.org/projects/nss/rev/2fceba406eee
https://hg.mozilla.org/projects/nss/rev/65e7b5f695ee
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(ekr)
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8730688 [details] [diff] [review]
0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch, v6

r=mt,ekr
Attachment #8730688 - Flags: review+
Attachment #8730688 - Flags: checked-in+
Obligatory follow-up to fix Windows bustage:

https://hg.mozilla.org/projects/nss/rev/05148b61d15a
Depends on: 1257129
Target Milestone: --- → 3.24
Blocks: 1258968
The server does not break the connection upon receiving the SSLv2-compatible ClientHello, requiring the connection to timeout for abort.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
> The server does not break the connection upon receiving the SSLv2-compatible ClientHello, requiring the connection to timeout for abort.

That's expected.  The SSLv2 ClientHello puts the version number in the same place that SSLv3/TLS puts a length.  The SSLv2-compatible ClientHello is rarely 770+ octets, so the server waits, patiently.

This is what it means to not support SSLv2 fully.
I agree with MT here. It's extra work to abort cleanly on v2 CH. Given the low incidence of SSLv2 CH (otherwise it wouldn't be safe to disable this), that seems like a small price to pay.
We do still have support for SSLv2 ClientHello, but it is disabled by default.  If you want an affordance in selfserv for tests that want to test the SSLv2 ClientHello, then please open another bug and we can discuss it.  It's going to be removed in a few releases, so we need to work out how to manage that.  This isn't the right place for that.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.