Bug 1228555 - Remove support for SSLv2

Bug 1228555 - Remove support for SSLv2

[ekr-rietveld, 2016-01-31 02:23:10]

Tim, this is quite large so I am going to have to review it in pieces. I was
only able to get through cmd/ tonight, but I am sending you comments so that you
can get started.

https://codereview.appspot.com/286030043/diff/1/cmd/lib/secutil.c
File cmd/lib/secutil.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/lib/secutil.c#newcode3724
cmd/lib/secutil.c:3724: SSLVersionRange *vrange, PRBool *enableSSL2)
Is this actually a public API? I suspect it is not and therefore you can remove
the SSLv2 code entirely.

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:2558: cptr = ssl3CipherSuites;
Is there a reason to maintain this temporary in this code and in the similar
code in strsclnt and probably selfserv?

https://codereview.appspot.com/286030043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:1119: cptr = ssl3CipherSuites;
Tabs? Also, why are we maintaining the temporary

https://codereview.appspot.com/286030043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:1494: ssl3stats->hsh_sid_cache_hits,
Extra space.

https://codereview.appspot.com/286030043/diff/1/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/tstclnt/tstclnt.c#newcode...
cmd/tstclnt/tstclnt.c:1302: cptr = ssl3CipherSuites;
Tabs, redundant temporary

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyserv.c
File cmd/vfyserv/vfyserv.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyserv.c#newcode531
cmd/vfyserv/vfyserv.c:531: cptr = ssl3CipherSuites;
Temporary, tabs

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyutil.c
File cmd/vfyserv/vfyutil.c (left):

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyutil.c#oldcode26
cmd/vfyserv/vfyutil.c:26: 
It seems like neither this nor the ssl3ciphersuites code is in use. Maybe we
should remove that as well

[ekr-rietveld, 2016-01-31 02:23:11]

Tim, this is quite large so I am going to have to review it in pieces. I was
only able to get through cmd/ tonight, but I am sending you comments so that you
can get started.

[ekr-rietveld, 2016-01-31 20:45:03]

Wan-Teh, in case you are interested.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h#newcode51
lib/ssl/SSLerrs.h:51: "Unrecognized SSL error code.")
1. Fix tabs

2. Given how many of these there are now (4) I suggest doing a macro:

#define UNUSED_ERROR(x) ER3(SSL_ERROR_UNUSED_##x, (SSL_ERROR_BASE + x),\
  "Unrecognized SSL error_code.")


3. Please double-check this file for other errors which are used only in SSLv2
(and potentially not at all). For instance SSL_ERROR_EXPORT_ONLY_SERVER and
SSL_ERROR_US_ONLY_SERVER

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h#newcode381
lib/ssl/SSLerrs.h:381: "Unrecognized SSL error code.")
See above comment about macro

https://codereview.appspot.com/286030043/diff/1/lib/ssl/manifest.mn
File lib/ssl/manifest.mn (left):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/manifest.mn#oldcode25
lib/ssl/manifest.mn:25: ssl3gthr.c \
I see you've moved the contents of ssl3gthr.c. That seems unnecessary.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode348
lib/ssl/sslimpl.h:348: int           state;	/* see GS_ values below. */
Please fix tabs as you change these.

MT: I would vote to also format these more sensibly in prep for clang-format.
Thoughts?

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode365
lib/ssl/sslimpl.h:365: /* DoRecv uses them to extract application data.
Removing the previous line removes the referent for "them". Please fix.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode386
lib/ssl/sslimpl.h:386: ** For SSL3, the plaintext portion is 5 bytes long. For
DTLS it is 13.
Not just SSL3. TLS too.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode388
lib/ssl/sslimpl.h:388: unsigned char hdr[13];				/* ssl 3 or dtls */
Nit: ssl3

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode1599
lib/ssl/sslimpl.h:1599: extern SECStatus ssl3_ConstructCipherSpecs(sslSocket
*ss);
Note to self: what does this do?

[mt, 2016-01-31 23:13:42]

I found one serious problem, and maybe there is a latent bug, but I need to look
more carefully at ssl_Do1stHandshake() because it can't be the case that this
generates an infinite loop in the current code.  See below for details.

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (left):

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c#oldco...
cmd/selfserv/selfserv.c:250: "F    SSL2 DES 192 EDE3 CBC WITH MD5\n"
Looks like we get the capital letters back.

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1850: rv = SSL_OptionSet(model_sock, SSL_SECURITY,
enabledVersions.min != 0);
I often wonder why this option even exists.

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:2558: cptr = ssl3CipherSuites;
careful with tabs

https://codereview.appspot.com/286030043/diff/1/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/tstclnt/tstclnt.c#newcode...
cmd/tstclnt/tstclnt.c:1302: cptr = ssl3CipherSuites;
On 2016/01/31 02:23:10, ekr-webrtc wrote:
> Tabs, redundant temporary

Is that true?  The value is incremented and dereferenced in the loop.  This is
an extraordinarily byzantine way of indexing a table based on a=0, b=1, and A=0,
B=1.  I believe that the temporary is needed in the crazy implementation.  Or
you could make this clearer and easier to read:

ndx = tolower(ndx) - 'a';
if (ndx < PR_ARRAY_SIZE(ssl3CipherSuites)) {
  cipher = ssl3CipherSuites[ndx];
}

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h#newcode50
lib/ssl/SSLerrs.h:50: ER3(SSL_ERROR_UNUSED_14,				(SSL_ERROR_BASE + 14),
Please revert the name change.  It's fine to change the text, but we probably
need to keep the name so that code continues to compile.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c
File lib/ssl/sslcon.c (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode132
lib/ssl/sslcon.c:132: ss->handshake = 0;
NULL

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode142
lib/ssl/sslcon.c:142: ssl2_BeginClientHandshake(sslSocket *ss)
A rename seems in order.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode195
lib/ssl/sslcon.c:195: while (sid) {  /* this isn't really a loop */
This might not be the right patch, but this "not really a loop" crap is much
easier to write and reason about if you make a separate function.  That function
can return instead of breaking out of a non-loop.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode202
lib/ssl/sslcon.c:202: if (!sidVersionEnabled) {
OK, if SSL_ALL_VERSIONS_DISABLED is true, shouldn't we abort?  That leaves this
condition as a simple if (sid->version >= min && sid->version <= max) which
removes the need for the temporary.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode238
lib/ssl/sslcon.c:238: goto loser;
This check is redundant with the call to ssl2_CheckConfigSanity

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslerr.h#newcode39
lib/ssl/sslerr.h:39: /* error 14 is obsolete */
Revert

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslerr.h#newcode180
lib/ssl/sslerr.h:180: /* error 117 is obsolete */
Revert

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslgathr.c
File lib/ssl/sslgathr.c (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslgathr.c#newcode2
lib/ssl/sslgathr.c:2: * Gather (Read) entire SSL3 records from socket into
buffer.
I think that I would have deleted *this* file and moved the small number of
pieces that were left into ssl3gthr.c instead.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode287
lib/ssl/sslimpl.h:287: unsigned int enableSSL2		: 1;  /*  8 */
You can rename this one to unusedBit8 if you like.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode292
lib/ssl/sslimpl.h:292: unsigned int v2CompatibleHello	: 1;  /* 13 */
bit13

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode348
lib/ssl/sslimpl.h:348: int           state;	/* see GS_ values below. */
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> Please fix tabs as you change these.
> 
> MT: I would vote to also format these more sensibly in prep for clang-format.
> Thoughts?

I figure that we'll just clobber it with the reformat, so I could go either way.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode388
lib/ssl/sslimpl.h:388: unsigned char hdr[13];				/* ssl 3 or dtls */
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> Nit: ssl3

Just remove the trailing comment.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode1137
lib/ssl/sslimpl.h:1137: unsigned int     sizeCipherSpecs;
I think that these two can be safely removed.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode1507
lib/ssl/sslimpl.h:1507: * SSL3 specific routines
No point in saying SSL3-specific here, since that's all that is left and all of
them apply to TLS as well.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode1599
lib/ssl/sslimpl.h:1599: extern SECStatus ssl3_ConstructCipherSpecs(sslSocket
*ss);
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> Note to self: what does this do?

I think that I've satisfied myself that this isn't needed.  It is only used in
sslv2 code.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (left):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslproto.h#oldcode54
lib/ssl/sslproto.h:54: #define SSL_MT_CLIENT_CERTIFICATE               8
Here's the catch: this is a public header.  That means that we can't really
remove these #defines if we intend to keep source compatibility.

Just put a fat warning over the top of these, and maybe move all the ssl2 stuff
right to the bottom together.  You can remove any existing explanation of what
they are.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (left):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#oldcode123
lib/ssl/sslsecur.c:123: }
Are you absolutely certain that this code is no longer necessary?  See
ssl3_SetAlwaysBlock.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode38
lib/ssl/sslsecur.c:38: * 1.   SECWouldBlock was returned by one of the callback
functions, via
Don't need the "1." any more.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode70
lib/ssl/sslsecur.c:70: if (ss->handshake == 0) {
NULL

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode74
lib/ssl/sslsecur.c:74: } while (rv != SECFailure);
I think that this is better stated as:

while (ss->handshake && rv == SECSuccess) {
  /* asserts */
  rv = (*ss->handshake)(ss);
}

That's a change to how this works, but I think that the change is correct.  From
what I can see, if this function is called after someone has set ss->handshake
to ssl3_SetAlwaysBlock() prior to the handshake completing then this will loop
forever.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode179
lib/ssl/sslsecur.c:179: ss->handshake = ssl2_BeginClientHandshake;
You should rename ssl2_Begin{Server|Client}Handshake to remove the 3.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode408
lib/ssl/sslsecur.c:408: PORT_Assert(!ss->firstHsDone);
IMPORTANT: This is a major change that will break existing code.

The check for firstHsDone just prevents useless calls to ssl_Do1stHandshake,
this function can be called once the handshake is complete.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode1091
lib/ssl/sslsecur.c:1091: if (!ss->firstHsDone) {
This smacks of double-checked locking, with two variables.  firstHsDone is
checked outside the lock, then handshake is checked inside it.

Maybe we need to look at whether ss->handshake != NULL is sufficient to
determine that the handshake isn't complete.  I think that it is.  Then maybe
this check can be made more efficient, since that will lead us to needing to
acquire and release the lock every time we send or receive, which is crap. 
Maybe firstHsDone can be retained as an atomic or something.

[mt, 2016-02-04 02:20:38]

On 2016/01/31 23:13:42, mt wrote:
> Maybe we need to look at whether ss->handshake != NULL is sufficient to
> determine that the handshake isn't complete.  I think that it is.  Then maybe
> this check can be made more efficient, since that will lead us to needing to
> acquire and release the lock every time we send or receive, which is crap. 
> Maybe firstHsDone can be retained as an atomic or something.

Ignore this.  We need firstHsDone for other reasons (damned renegotiation).

[ttaubert, 2016-02-04 17:32:42]

https://codereview.appspot.com/286030043/diff/1/cmd/lib/secutil.c
File cmd/lib/secutil.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/lib/secutil.c#newcode3724
cmd/lib/secutil.c:3724: SSLVersionRange *vrange, PRBool *enableSSL2)
On 2016/01/31 02:23:10, ekr-webrtc wrote:
> Is this actually a public API? I suspect it is not and therefore you can
remove
> the SSLv2 code entirely.

Done.

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1850: rv = SSL_OptionSet(model_sock, SSL_SECURITY,
enabledVersions.min != 0);
On 2016/01/31 23:13:41, mt wrote:
> I often wonder why this option even exists.

I don't know either. It seems that we somehow have support for insecure sockets,
which probably means unencrypted. This doesn't seem useful but probably
follow-up work.

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:2558: cptr = ssl3CipherSuites;
On 2016/01/31 02:23:10, ekr-webrtc wrote:
> Is there a reason to maintain this temporary in this code and in the similar
> code in strsclnt and probably selfserv?

Done.

https://codereview.appspot.com/286030043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:2558: cptr = ssl3CipherSuites;
On 2016/01/31 23:13:41, mt wrote:
> careful with tabs

Done.

https://codereview.appspot.com/286030043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:1119: cptr = ssl3CipherSuites;
On 2016/01/31 02:23:10, ekr-webrtc wrote:
> Tabs? Also, why are we maintaining the temporary

Done.

https://codereview.appspot.com/286030043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:1494: ssl3stats->hsh_sid_cache_hits,
On 2016/01/31 02:23:10, ekr-webrtc wrote:
> Extra space.

Done.

https://codereview.appspot.com/286030043/diff/1/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/tstclnt/tstclnt.c#newcode...
cmd/tstclnt/tstclnt.c:1302: cptr = ssl3CipherSuites;
On 2016/01/31 23:13:41, mt wrote:
> On 2016/01/31 02:23:10, ekr-webrtc wrote:
> > Tabs, redundant temporary
> 
> Is that true?  The value is incremented and dereferenced in the loop.  This is
> an extraordinarily byzantine way of indexing a table based on a=0, b=1, and
A=0,
> B=1.  I believe that the temporary is needed in the crazy implementation.  Or
> you could make this clearer and easier to read:
> 
> ndx = tolower(ndx) - 'a';
> if (ndx < PR_ARRAY_SIZE(ssl3CipherSuites)) {
>   cipher = ssl3CipherSuites[ndx];
> }

Done.

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyserv.c
File cmd/vfyserv/vfyserv.c (right):

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyserv.c#newcode531
cmd/vfyserv/vfyserv.c:531: cptr = ssl3CipherSuites;
On 2016/01/31 02:23:10, ekr-webrtc wrote:
> Temporary, tabs

Done.

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyutil.c
File cmd/vfyserv/vfyutil.c (left):

https://codereview.appspot.com/286030043/diff/1/cmd/vfyserv/vfyutil.c#oldcode26
cmd/vfyserv/vfyutil.c:26: 
On 2016/01/31 02:23:10, ekr-webrtc wrote:
> It seems like neither this nor the ssl3ciphersuites code is in use. Maybe we
> should remove that as well

It's used by vfyserv.c.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h#newcode50
lib/ssl/SSLerrs.h:50: ER3(SSL_ERROR_UNUSED_14,				(SSL_ERROR_BASE + 14),
On 2016/01/31 23:13:41, mt wrote:
> Please revert the name change.  It's fine to change the text, but we probably
> need to keep the name so that code continues to compile.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/SSLerrs.h#newcode51
lib/ssl/SSLerrs.h:51: "Unrecognized SSL error code.")
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> 2. Given how many of these there are now (4) I suggest doing a macro:
> 
> #define UNUSED_ERROR(x) ER3(SSL_ERROR_UNUSED_##x, (SSL_ERROR_BASE + x),\
>   "Unrecognized SSL error_code.")

Added, using it for 5 and 10 only because I had to revert my changes back to the
original error names.

> 3. Please double-check this file for other errors which are used only in SSLv2
> (and potentially not at all). For instance SSL_ERROR_EXPORT_ONLY_SERVER and
> SSL_ERROR_US_ONLY_SERVER

Deprecated a few more.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/manifest.mn
File lib/ssl/manifest.mn (left):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/manifest.mn#oldcode25
lib/ssl/manifest.mn:25: ssl3gthr.c \
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> I see you've moved the contents of ssl3gthr.c. That seems unnecessary.

If I didn't do that we'd be left with a tiny sslgathr.c that lists two small
functions, and a ssl3gthr.c that has most the actual code. Is your preference to
keep both? Keep only ssl3gthr.c and move the two functions over?

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c
File lib/ssl/sslcon.c (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode132
lib/ssl/sslcon.c:132: ss->handshake = 0;
On 2016/01/31 23:13:41, mt wrote:
> NULL

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode142
lib/ssl/sslcon.c:142: ssl2_BeginClientHandshake(sslSocket *ss)
On 2016/01/31 23:13:41, mt wrote:
> A rename seems in order.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode195
lib/ssl/sslcon.c:195: while (sid) {  /* this isn't really a loop */
On 2016/01/31 23:13:41, mt wrote:
> This might not be the right patch, but this "not really a loop" crap is much
> easier to write and reason about if you make a separate function.  That
function
> can return instead of breaking out of a non-loop.

Turned it into a normal branch, which I think is quite readable. Let me know if
you think we should have a separate function here.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode202
lib/ssl/sslcon.c:202: if (!sidVersionEnabled) {
On 2016/01/31 23:13:41, mt wrote:
> OK, if SSL_ALL_VERSIONS_DISABLED is true, shouldn't we abort?  That leaves
this
> condition as a simple if (sid->version >= min && sid->version <= max) which
> removes the need for the temporary.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslcon.c#newcode238
lib/ssl/sslcon.c:238: goto loser;
On 2016/01/31 23:13:41, mt wrote:
> This check is redundant with the call to ssl2_CheckConfigSanity

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslerr.h#newcode39
lib/ssl/sslerr.h:39: /* error 14 is obsolete */
On 2016/01/31 23:13:41, mt wrote:
> Revert

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslerr.h#newcode180
lib/ssl/sslerr.h:180: /* error 117 is obsolete */
On 2016/01/31 23:13:41, mt wrote:
> Revert

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslgathr.c
File lib/ssl/sslgathr.c (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslgathr.c#newcode2
lib/ssl/sslgathr.c:2: * Gather (Read) entire SSL3 records from socket into
buffer.
On 2016/01/31 23:13:41, mt wrote:
> I think that I would have deleted *this* file and moved the small number of
> pieces that were left into ssl3gthr.c instead.

Yeah, I thought not having a "3" in the name would be nice but it definitely
makes the patch smaller.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode287
lib/ssl/sslimpl.h:287: unsigned int enableSSL2		: 1;  /*  8 */
On 2016/01/31 23:13:42, mt wrote:
> You can rename this one to unusedBit8 if you like.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode292
lib/ssl/sslimpl.h:292: unsigned int v2CompatibleHello	: 1;  /* 13 */
On 2016/01/31 23:13:42, mt wrote:
> bit13

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode348
lib/ssl/sslimpl.h:348: int           state;	/* see GS_ values below. */
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> Please fix tabs as you change these.
> 
> MT: I would vote to also format these more sensibly in prep for clang-format.
> Thoughts?

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode365
lib/ssl/sslimpl.h:365: /* DoRecv uses them to extract application data.
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> Removing the previous line removes the referent for "them". Please fix.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode386
lib/ssl/sslimpl.h:386: ** For SSL3, the plaintext portion is 5 bytes long. For
DTLS it is 13.
On 2016/01/31 20:45:03, ekr-webrtc wrote:
> Not just SSL3. TLS too.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode388
lib/ssl/sslimpl.h:388: unsigned char hdr[13];				/* ssl 3 or dtls */
On 2016/01/31 23:13:42, mt wrote:
> On 2016/01/31 20:45:03, ekr-webrtc wrote:
> > Nit: ssl3
> 
> Just remove the trailing comment.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode1507
lib/ssl/sslimpl.h:1507: * SSL3 specific routines
On 2016/01/31 23:13:42, mt wrote:
> No point in saying SSL3-specific here, since that's all that is left and all
of
> them apply to TLS as well.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslimpl.h#newcode1599
lib/ssl/sslimpl.h:1599: extern SECStatus ssl3_ConstructCipherSpecs(sslSocket
*ss);
On 2016/01/31 23:13:42, mt wrote:
> On 2016/01/31 20:45:03, ekr-webrtc wrote:
> > Note to self: what does this do?
> 
> I think that I've satisfied myself that this isn't needed.  It is only used in
> sslv2 code.

Darn, of course. And I spent time minimizing that function not realizing that
cipherSpecs/sizeCiphersSpecs are never used. Thanks!

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (left):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslproto.h#oldcode54
lib/ssl/sslproto.h:54: #define SSL_MT_CLIENT_CERTIFICATE               8
On 2016/01/31 23:13:41, mt wrote:
> Here's the catch: this is a public header.  That means that we can't really
> remove these #defines if we intend to keep source compatibility.
> 
> Just put a fat warning over the top of these, and maybe move all the ssl2
stuff
> right to the bottom together.  You can remove any existing explanation of what
> they are.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (left):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#oldcode123
lib/ssl/sslsecur.c:123: }
On 2016/01/31 23:13:42, mt wrote:
> Are you absolutely certain that this code is no longer necessary?  See
> ssl3_SetAlwaysBlock.

Yeah, we need that back. I think I was confused by the old "This code must
continue to loop on SECWouldBlock" comment above. It seems like we don't want to
do that. Blame isn't helpful as that's been around since the initial commit.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode38
lib/ssl/sslsecur.c:38: * 1.   SECWouldBlock was returned by one of the callback
functions, via
On 2016/01/31 23:13:42, mt wrote:
> Don't need the "1." any more.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode70
lib/ssl/sslsecur.c:70: if (ss->handshake == 0) {
On 2016/01/31 23:13:42, mt wrote:
> NULL

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode74
lib/ssl/sslsecur.c:74: } while (rv != SECFailure);
On 2016/01/31 23:13:42, mt wrote:
> I think that this is better stated as:
> 
> while (ss->handshake && rv == SECSuccess) {
>   /* asserts */
>   rv = (*ss->handshake)(ss);
> }
> 
> That's a change to how this works, but I think that the change is correct. 
From
> what I can see, if this function is called after someone has set ss->handshake
> to ssl3_SetAlwaysBlock() prior to the handshake completing then this will loop
> forever.

Agreed, that version makes a lot more sense to me too. We just had this comment
(see below) that I didn't quite understand.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode179
lib/ssl/sslsecur.c:179: ss->handshake = ssl2_BeginClientHandshake;
On 2016/01/31 23:13:42, mt wrote:
> You should rename ssl2_Begin{Server|Client}Handshake to remove the 3.

Done.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode408
lib/ssl/sslsecur.c:408: PORT_Assert(!ss->firstHsDone);
On 2016/01/31 23:13:42, mt wrote:
> IMPORTANT: This is a major change that will break existing code.
> 
> The check for firstHsDone just prevents useless calls to ssl_Do1stHandshake,
> this function can be called once the handshake is complete.

Ah, okay. Thanks for shedding some light onto this. Wasn't quite sure about the
different paths available here and when they're taken. Reverted.

https://codereview.appspot.com/286030043/diff/1/lib/ssl/sslsecur.c#newcode1091
lib/ssl/sslsecur.c:1091: if (!ss->firstHsDone) {
On 2016/01/31 23:13:42, mt wrote:
> This smacks of double-checked locking, with two variables.  firstHsDone is
> checked outside the lock, then handshake is checked inside it.
> 
> Maybe we need to look at whether ss->handshake != NULL is sufficient to
> determine that the handshake isn't complete.  I think that it is.  Then maybe
> this check can be made more efficient, since that will lead us to needing to
> acquire and release the lock every time we send or receive, which is crap. 
> Maybe firstHsDone can be retained as an atomic or something.

Didn't touch this based on your later comment saying that we need firstHsDone
for renegotiation.

[ttaubert, 2016-02-04 17:34:19]

Rebased and all feedback addressed.

[ttaubert, 2016-02-29 15:11:30]

Rebased again after all the clang-formatting. Will upload the SSLv2 client hello
as a second patch.

[mt, 2016-03-02 03:28:30]

LGTM

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/ssl.h
File lib/ssl/ssl.h (left):

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/ssl.h#oldcode41
lib/ssl/ssl.h:41: #define SSL_IS_SSL2_CIPHER(which) (((which)&0xfff0) == 0xff00)
You can't delete this, sadly.  You can define it to be PR_FALSE though.

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/sslimpl.h#newcode378
lib/ssl/sslimpl.h:378: unsigned int recordPadding; /* ssl2 only */
Delete this please.  See other review.

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/sslimpl.h#newcode683
lib/ssl/sslimpl.h:683: } u;
Did you consider collapsing the union and struct here?  Or is that something for
a follow-up.

https://codereview.appspot.com/286030043/diff/30001/lib/util/ciferfam.h
File lib/util/ciferfam.h (left):

https://codereview.appspot.com/286030043/diff/30001/lib/util/ciferfam.h#oldco...
lib/util/ciferfam.h:17: #define CIPHER_FAMILY_SSL2			"SSLv2"
Mark as deprecated and keep.

[wtc1, 2016-03-02 04:53:57]

Review comments on patch set 4:

Please consider my review as supplemental. I will try to
review it again carefully. The changes are smaller than I
expected. Good job!

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/SSLerrs.h#newcode12
lib/ssl/SSLerrs.h:12: "Unrecognized SSL error code.")
We should preserve these error messages.

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/ssl.h
File lib/ssl/ssl.h (left):

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/ssl.h#oldcode41
lib/ssl/ssl.h:41: #define SSL_IS_SSL2_CIPHER(which) (((which)&0xfff0) == 0xff00)
On 2016/03/02 03:28:30, mt wrote:
> You can't delete this, sadly.  You can define it to be PR_FALSE though.

I think we should keep the macro's original definition.

[ttaubert, 2016-03-02 13:15:47]

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/SSLerrs.h#newcode12
lib/ssl/SSLerrs.h:12: "Unrecognized SSL error code.")
On 2016/03/02 04:53:57, wtc1 wrote:
> We should preserve these error messages.

Okay, I think it doesn't hurt to do that.

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/ssl.h
File lib/ssl/ssl.h (left):

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/ssl.h#oldcode41
lib/ssl/ssl.h:41: #define SSL_IS_SSL2_CIPHER(which) (((which)&0xfff0) == 0xff00)
On 2016/03/02 04:53:57, wtc1 wrote:
> On 2016/03/02 03:28:30, mt wrote:
> > You can't delete this, sadly.  You can define it to be PR_FALSE though.
> 
> I think we should keep the macro's original definition.

I restored the original definition, I think it makes sense to do that as long as
we keep the original SSLv2 cipher suite code points in sslproto.h.

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/sslimpl.h#newcode378
lib/ssl/sslimpl.h:378: unsigned int recordPadding; /* ssl2 only */
On 2016/03/02 03:28:30, mt wrote:
> Delete this please.  See other review.

Done.

https://codereview.appspot.com/286030043/diff/30001/lib/ssl/sslimpl.h#newcode683
lib/ssl/sslimpl.h:683: } u;
On 2016/03/02 03:28:30, mt wrote:
> Did you consider collapsing the union and struct here?  Or is that something
for
> a follow-up.

Definitely considered that but that touches a lot of lines which I thought would
complicate the review here somewhat. Filed bug 1252795 as a follow-up, happy to
work on it when this lands.

https://codereview.appspot.com/286030043/diff/30001/lib/util/ciferfam.h
File lib/util/ciferfam.h (left):

https://codereview.appspot.com/286030043/diff/30001/lib/util/ciferfam.h#oldco...
lib/util/ciferfam.h:17: #define CIPHER_FAMILY_SSL2			"SSLv2"
On 2016/03/02 03:28:30, mt wrote:
> Mark as deprecated and keep.

Done.

[ekr-rietveld, 2016-03-02 18:35:33]

ttaubert,

I will try to review this over the next few days.

[ekr-rietveld, 2016-03-08 13:25:56]

https://codereview.appspot.com/286030043/diff/40001/cmd/lib/secutil.c
File cmd/lib/secutil.c (right):

https://codereview.appspot.com/286030043/diff/40001/cmd/lib/secutil.c#newcode...
cmd/lib/secutil.c:3788: }
Is it actaually possible here to have max < min? That seems bad.

https://codereview.appspot.com/286030043/diff/40001/cmd/lib/secutil.c#newcode...
cmd/lib/secutil.c:3788: }
Is it actaually possible here to have max < min? That seems bad.

https://codereview.appspot.com/286030043/diff/40001/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (left):

https://codereview.appspot.com/286030043/diff/40001/cmd/tstclnt/tstclnt.c#old...
cmd/tstclnt/tstclnt.c:229: "(required for SSL2)\n", "-O");
Currently required for TLS 1.3. Would you mind replacing this line with that.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#oldcode...
lib/ssl/ssl3con.c:9402: */
Shouldn't you fix this comment, not just delete it

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2495: if (spec->version == SSL_LIBRARY_VERSION_3_0) {
Was < ever sensible here?

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5306: : ssl_LookupSID(&ss->sec.ci.peer, ss->sec.ci.port,
ss->peerID, ss->url);
Is there a reason not to consolidate the SID-handling code from
ssl_BeginClientHandshake()? It seems like its there to do v2/v3 detection based
on SID

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1406: #define SSL_GET_SERVER_PUBLICKEY(sock, type)            
                              \
Why change this? Everything else is named SSL3_

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3gthr.c#newcode37
lib/ssl/ssl3gthr.c:37: }
Shouldn't these be called ssl3?

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c
File lib/ssl/sslcon.c (left):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#oldcode1241
lib/ssl/sslcon.c:1241: ss->handshake = 0; /* makes ssl_Do1stHandshake call
ss->nextHandshake.*/
These changes seem gratuitous

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c
File lib/ssl/sslcon.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#newcode111
lib/ssl/sslcon.c:111: */
This comment doesn't seem to match the code, which just seems to check version
#s.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#newcode177
lib/ssl/sslcon.c:177: ss->version = sid->version;
This does not appear to match line 3083, which sets the version to v3.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#newcode181
lib/ssl/sslcon.c:181: }
I don't get this. We are asserting that it's empty and then freeing it? Silly.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslproto.h#newcod...
lib/ssl/sslproto.h:273: #define SSL_EN_DES_192_EDE3_CBC_WITH_MD5        0xFF07
Why are these needed at all?

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (left):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#oldcode92
lib/ssl/sslsecur.c:92: ss->nextHandshake = 0;
Are you sure this never happens in v3?

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#newcode64
lib/ssl/sslsecur.c:64: while (ss->handshake && rv == SECSuccess) {
Why isn't this causeing problems on SECWouldBlock?

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:411: }
Why do we need this arm?

https://codereview.appspot.com/286030043/diff/40001/lib/util/ciferfam.h
File lib/util/ciferfam.h (right):

https://codereview.appspot.com/286030043/diff/40001/lib/util/ciferfam.h#newco...
lib/util/ciferfam.h:17: #define CIPHER_FAMILY_SSL2			"SSLv2" /* deprecated */
tabs -> spaces

[ekr-rietveld, 2016-03-08 14:03:33]

Note: I didn't study anything in tests/pkcs11 or ssl.sh, sslcov.txt or
sslstress.txt

[ttaubert, 2016-03-08 19:19:52]

https://codereview.appspot.com/286030043/diff/40001/cmd/lib/secutil.c
File cmd/lib/secutil.c (right):

https://codereview.appspot.com/286030043/diff/40001/cmd/lib/secutil.c#newcode...
cmd/lib/secutil.c:3788: }
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Is it actaually possible here to have max < min? That seems bad.

Added a check for this.

https://codereview.appspot.com/286030043/diff/40001/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (left):

https://codereview.appspot.com/286030043/diff/40001/cmd/tstclnt/tstclnt.c#old...
cmd/tstclnt/tstclnt.c:229: "(required for SSL2)\n", "-O");
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Currently required for TLS 1.3. Would you mind replacing this line with that.

Done.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#oldcode...
lib/ssl/ssl3con.c:9402: */
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Shouldn't you fix this comment, not just delete it

Removed it because this special case doesn't exist anymore.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2495: if (spec->version == SSL_LIBRARY_VERSION_3_0) {
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Was < ever sensible here?

Looks like that was wrong indeed.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5306: : ssl_LookupSID(&ss->sec.ci.peer, ss->sec.ci.port,
ss->peerID, ss->url);
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Is there a reason not to consolidate the SID-handling code from
> ssl_BeginClientHandshake()? It seems like its there to do v2/v3 detection
based
> on SID

That's a good point. I think we could indeed merge those two and there very
likely is a lot of duplicate work. I don't understand all of it very well though
and it seems like there is some stuff we do here (e.g. duplicate localCert) that
the other path doesn't do.

How about looking at this in a follow-up, later when we find some time for it?

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1406: #define SSL_GET_SERVER_PUBLICKEY(sock, type)            
                              \
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Why change this? Everything else is named SSL3_

Thought it makes sense to work towards a world without ssl3_ prefixes but okay,
reverted it.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3gthr.c#newcode37
lib/ssl/ssl3gthr.c:37: }
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Shouldn't these be called ssl3?

We can do that, all the other gather functions seemed to still have the ssl_
prefix. Done.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c
File lib/ssl/sslcon.c (left):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#oldcode1241
lib/ssl/sslcon.c:1241: ss->handshake = 0; /* makes ssl_Do1stHandshake call
ss->nextHandshake.*/
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> These changes seem gratuitous

I thought it made sense to use NULL here when I had to touch the line anyway to
remove the outdated comment.

The trace was removed because ss->gs.recordLen is gone, it was only used for
SSL2. Readded at the bottom of ssl3_GatherData().

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c
File lib/ssl/sslcon.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#newcode111
lib/ssl/sslcon.c:111: */
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> This comment doesn't seem to match the code, which just seems to check version
> #s.

Done.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#newcode177
lib/ssl/sslcon.c:177: ss->version = sid->version;
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> This does not appear to match line 3083, which sets the version to v3.

Removed. This only made sense when sending an SSL2 client hello.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslcon.c#newcode181
lib/ssl/sslcon.c:181: }
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> I don't get this. We are asserting that it's empty and then freeing it? Silly.

Yeah... removed that.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslproto.h#newcod...
lib/ssl/sslproto.h:273: #define SSL_EN_DES_192_EDE3_CBC_WITH_MD5        0xFF07
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Why are these needed at all?

In comment #4 mt said that we need to keep those defines as this is a public
header.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (left):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#oldcode92
lib/ssl/sslsecur.c:92: ss->nextHandshake = 0;
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Are you sure this never happens in v3?

I simplified the code so we don't need this anymore, nextHandshake and
securityHandshake are gone, ss->handshake is set to NULL when we're done with
the handshake.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#newcode64
lib/ssl/sslsecur.c:64: while (ss->handshake && rv == SECSuccess) {
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Why isn't this causeing problems on SECWouldBlock?

A good description of why the XXX_1 hacks were introduced is here:

https://mxr.mozilla.org/nss/source/lib/ssl/sslgathr.c#127

We needed it to properly support a v3 handshake record on a v2 connection.
SECWouldBlock was used as a magic value to return all the way back to
ssl_Do1stHandshake() and start over.

I think we don't need this anymore, so a SECWouldBlock returned from
ssl3_HandleRecord() should simply cause ssl_Do1stHandshake() to set
PR_WOULD_BLOCK_ERROR and the calling function to fail.

I assume here that we wouldn't want to loop when blocked on UI or a callback,
right? This is all rather complicated, I appreciate a few more looks at this.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:411: }
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> Why do we need this arm?

The first arm gathers another handshake in a connection the has a version number
set before already. The second arm when firstHsDone=false means that this is the
first handshake. Could probably turn the !firstHsDone into an assertion but the
outcome would be the same.

https://codereview.appspot.com/286030043/diff/40001/lib/util/ciferfam.h
File lib/util/ciferfam.h (right):

https://codereview.appspot.com/286030043/diff/40001/lib/util/ciferfam.h#newco...
lib/util/ciferfam.h:17: #define CIPHER_FAMILY_SSL2			"SSLv2" /* deprecated */
On 2016/03/08 13:25:55, ekr-webrtc wrote:
> tabs -> spaces

Done.

[ekr-rietveld, 2016-03-11 09:02:40]

LGTM

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5306: : ssl_LookupSID(&ss->sec.ci.peer, ss->sec.ci.port,
ss->peerID, ss->url);
On 2016/03/08 19:19:52, ttaubert wrote:
> On 2016/03/08 13:25:55, ekr-webrtc wrote:
> > Is there a reason not to consolidate the SID-handling code from
> > ssl_BeginClientHandshake()? It seems like its there to do v2/v3 detection
> based
> > on SID
> 
> That's a good point. I think we could indeed merge those two and there very
> likely is a lot of duplicate work. I don't understand all of it very well
though
> and it seems like there is some stuff we do here (e.g. duplicate localCert)
that
> the other path doesn't do.
> 
> How about looking at this in a follow-up, later when we find some time for it?


Sure. File a bug

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslproto.h#newcod...
lib/ssl/sslproto.h:273: #define SSL_EN_DES_192_EDE3_CBC_WITH_MD5        0xFF07
On 2016/03/08 19:19:52, ttaubert wrote:
> On 2016/03/08 13:25:55, ekr-webrtc wrote:
> > Why are these needed at all?
> 
> In comment #4 mt said that we need to keep those defines as this is a public
> header.

OK. Maybe later.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#newcode64
lib/ssl/sslsecur.c:64: while (ss->handshake && rv == SECSuccess) {
On 2016/03/08 19:19:52, ttaubert wrote:
> On 2016/03/08 13:25:55, ekr-webrtc wrote:
> > Why isn't this causeing problems on SECWouldBlock?
> 
> A good description of why the XXX_1 hacks were introduced is here:
> 
> https://mxr.mozilla.org/nss/source/lib/ssl/sslgathr.c#127
> 
> We needed it to properly support a v3 handshake record on a v2 connection.
> SECWouldBlock was used as a magic value to return all the way back to
> ssl_Do1stHandshake() and start over.
> 
> I think we don't need this anymore, so a SECWouldBlock returned from
> ssl3_HandleRecord() should simply cause ssl_Do1stHandshake() to set
> PR_WOULD_BLOCK_ERROR and the calling function to fail.
> 
> I assume here that we wouldn't want to loop when blocked on UI or a callback,
> right? This is all rather complicated, I appreciate a few more looks at this.

I buy this argument

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:411: }
On 2016/03/08 19:19:52, ttaubert wrote:
> On 2016/03/08 13:25:55, ekr-webrtc wrote:
> > Why do we need this arm?
> 
> The first arm gathers another handshake in a connection the has a version
number
> set before already. The second arm when firstHsDone=false means that this is
the
> first handshake. Could probably turn the !firstHsDone into an assertion but
the
> outcome would be the same.

Yes, please make it an assert. That would make it much clearer.

https://codereview.appspot.com/286030043/diff/60001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/286030043/diff/60001/lib/ssl/SSLerrs.h#newcode459
lib/ssl/SSLerrs.h:459: "SSL received a missing_extenson alert.")
Nit: "missing extension". Can you test?

[ttaubert, 2016-03-11 10:53:20]

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5306: : ssl_LookupSID(&ss->sec.ci.peer, ss->sec.ci.port,
ss->peerID, ss->url);
On 2016/03/11 09:02:39, ekr-webrtc wrote:
> On 2016/03/08 19:19:52, ttaubert wrote:
> > On 2016/03/08 13:25:55, ekr-webrtc wrote:
> > > Is there a reason not to consolidate the SID-handling code from
> > > ssl_BeginClientHandshake()? It seems like its there to do v2/v3 detection
> > based
> > > on SID
> > 
> > That's a good point. I think we could indeed merge those two and there very
> > likely is a lot of duplicate work. I don't understand all of it very well
> though
> > and it seems like there is some stuff we do here (e.g. duplicate localCert)
> that
> > the other path doesn't do.
> > 
> > How about looking at this in a follow-up, later when we find some time for
it?
> 
> 
> Sure. File a bug

Filed bug 1255758.

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/286030043/diff/40001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:411: }
On 2016/03/11 09:02:39, ekr-webrtc wrote:
> On 2016/03/08 19:19:52, ttaubert wrote:
> > On 2016/03/08 13:25:55, ekr-webrtc wrote:
> > > Why do we need this arm?
> > 
> > The first arm gathers another handshake in a connection the has a version
> number
> > set before already. The second arm when firstHsDone=false means that this is
> the
> > first handshake. Could probably turn the !firstHsDone into an assertion but
> the
> > outcome would be the same.
> 
> Yes, please make it an assert. That would make it much clearer.

Done.

https://codereview.appspot.com/286030043/diff/60001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/286030043/diff/60001/lib/ssl/SSLerrs.h#newcode459
lib/ssl/SSLerrs.h:459: "SSL received a missing_extenson alert.")
On 2016/03/11 09:02:39, ekr-webrtc wrote:
> Nit: "missing extension". Can you test?

Fixed.