1228555 - Remove support for SSLv2

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 2

[Alicja Kario]

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

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1228555-Remove-support-for-SSLv2.patch, v2

Comment 4

[Eric Rescorla (:ekr)]

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

Comment 5

[Alicja Kario]

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

Comment 6

[Eric Rescorla (:ekr)]

(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

Comment 7

[Alicja Kario]

(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

Comment 8

[Martin Thomson [:mt:]]

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.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1228555-Remove-support-for-SSLv2.patch, v3

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 13

[Martin Thomson [:mt:]]

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.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1228555-Remove-support-for-SSLv2.patch, v4

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch, v2

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

New patches up.

Comment 17

[Martin Thomson [:mt:]]

r=mt on both, thanks.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch, v3

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch, v4

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1228555-Remove-support-for-SSLv2-r-mt-wtc-ekr.patch, v5

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch, v5

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1228555-Remove-support-for-SSLv2-r-mt-wtc-ekr.patch, v6

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1228555-Remove-support-for-SSLv2-r-mt-wtc-ekr.patch, v7

r=mt,wtc,ekr

https://hg.mozilla.org/projects/nss/rev/0c19072b8537

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Pushed a small follow-up to make Windows builders happy:

https://hg.mozilla.org/projects/nss/rev/52f57b0c5444

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

And another follow-up for Windows (my VM doesn't reproduce these failures unfortunately...):

https://hg.mozilla.org/projects/nss/rev/2fceba406eee

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch, v6

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/65e7b5f695ee

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8730688 [details] [diff] [review]
0002-Bug-1228555-Re-add-support-for-SSLv2-client-hellos-i.patch, v6

r=mt,ekr

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Obligatory follow-up to fix Windows bustage:

https://hg.mozilla.org/projects/nss/rev/05148b61d15a

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

One more for Windows:

https://hg.mozilla.org/projects/nss/rev/61667a88c53c

Comment 31

[Alicja Kario]

The server does not break the connection upon receiving the SSLv2-compatible ClientHello, requiring the connection to timeout for abort.

Comment 32

[Martin Thomson [:mt:]]

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

Comment 33

[Eric Rescorla (:ekr)]

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.

Comment 34

[Martin Thomson [:mt:]]

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.