Almost there. I have a few suggestions for improvements. https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/libssl_internals.c File external_tests/ssl_gtest/libssl_internals.c (right): https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/libssl_internals.c#newcode51 external_tests/ssl_gtest/libssl_internals.c:51: ...
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/libs...
File external_tests/ssl_gtest/libssl_internals.c (right):
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:51: uint8_t *msg, size_t msg_len)
On 2016/03/02 00:12:03, mt wrote:
> Can you explain why this exists please?
Done.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:53: sslSocket *ss = (sslSocket
*)fd->secret;
On 2016/03/02 00:12:02, mt wrote:
> ssl_FindSocket(fd)
Done.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:82: sslSocket *ss = (sslSocket
*)fd->secret;
On 2016/03/02 00:12:03, mt wrote:
> FindSocket
Done.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc (right):
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc:31: , ciphers_(0)
On 2016/03/02 00:12:03, mt wrote:
> is the comma convention here supported by clang-format?
No, corrected.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc:109:
client_->SetPacketFilter(nullptr);
On 2016/03/02 00:12:03, mt wrote:
> The usual practice here is to keep the filter in place and set a flag; that
> ensures that you can have multiple filters in use.
Ah, okay. That's how I did it before but it seemed more elegant without a flag.
Re-added.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc:137: void
SetAvailableCipherSuites(uint16_t cipher) {
On 2016/03/02 00:12:03, mt wrote:
> A plural name doesn't make sense.
Corrected.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc:184: // Test
negotiating an EC suite (works only with TLS 1.2).
On 2016/03/02 00:12:03, mt wrote:
> Do you know why this is so?
Ohhh, that's because I chose a GCM suite. Thanks for making me double-check
this, replaced it with TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc:226:
RequireSafeNegotiation();
On 2016/03/02 00:12:03, mt wrote:
> RequireSafeRenegotiation();
>
> It's not clear that negotiation is ever safe here.
Done.
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_connect.cc (right):
https://codereview.appspot.com/287280043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.cc:214: void
TlsConnectTestBase::ConnectExpectServerFail() {
On 2016/03/02 00:12:03, mt wrote:
> This is a worry. In what circumstances does the server fail, but the client
> succeed, and can we correct that somehow?
There were a few dealing with a malformed v2 client hello in
ssl3_HandleV2ClientHello(). I fixed those to send an alert.
> I think that I can see how this happens, but you should at least explain why
in
> all the cases you use this function.
>
> If possible, you should try to ensure that the server sends a fatal alert to
the
> client and then test that the alert was received and acted upon.
Removed ConnectExpectServerFail() as we have alerts now.
https://codereview.appspot.com/287280043/diff/1/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):
https://codereview.appspot.com/287280043/diff/1/lib/ssl/ssl3gthr.c#newcode75
lib/ssl/ssl3gthr.c:75: gs->recordPadding = 0;
On 2016/03/02 00:12:03, mt wrote:
> I think that you can create a v2-compatible-specific pipeline for this
> information, since it is only used once. If this function took a pointer to a
> struct with v2-specific information, then it could take a non-NULL value as an
> indication that v2 should be checked for. Then, you could populate that
struct
> with this information.
>
> That means more changes to ssl3_GatherCompleteHandshake, but I think that
would
> make the v2 code better isolated.
Done.
https://codereview.appspot.com/287280043/diff/1/lib/ssl/ssl3gthr.c#newcode125
lib/ssl/ssl3gthr.c:125: /* Check for SSLv3 handshakes. Always assume SSLv3 on
clients,
On 2016/03/02 00:12:03, mt wrote:
> "Check for SSLv2 handshakes"
Done.
https://codereview.appspot.com/287280043/diff/1/lib/ssl/ssl3gthr.c#newcode130
lib/ssl/ssl3gthr.c:130: gs->hdr[1] == MSB(SSL_LIBRARY_VERSION_3_0))) {
On 2016/03/02 00:12:03, mt wrote:
> I think that this check could be even more precise. In
> ssl3_GatherCompleteHandshake, you can check the wait state (copy it out under
> the handshake lock) and check whether you are a server or not. Then, you can
> pass a flag to this function that says that you permit detecting v2
ClientHello.
>
> That way, the v2 code is only ever engaged when you are waiting for a
> ClientHello.
Great idea, done.
https://codereview.appspot.com/287280043/diff/1/lib/ssl/ssl3gthr.c#newcode416
lib/ssl/ssl3gthr.c:416: ss->gs.inbuf.len);
On 2016/03/02 00:12:03, mt wrote:
> You can return early on error here. You won't have received a close_notify
> alert at this point so you don't have to defer.
Done.
https://codereview.appspot.com/287280043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):
https://codereview.appspot.com/287280043/diff/1/lib/ssl/sslimpl.h#newcode121
lib/ssl/sslimpl.h:121: #define SSL_CHALLENGE_BYTES 16
On 2016/03/02 17:22:07, mt wrote:
> This #define isn't needed any more.
Removed.
https://codereview.appspot.com/287280043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc (right):
https://codereview.appspot.com/287280043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc:263:
SetClientRandomLength(SSL_MIN_CHALLENGE_BYTES - 1);
On 2016/03/02 17:22:07, mt wrote:
> This #define (SSL_MIN_CHALLENGE_BYTES) is a private one. I'm a little
concerned
> that you are using private headers in these tests (outside of SSLInt_* stuff).
Removed the sslimpl.h include.
> Nothing wrong with hard-coding these values.
Done.
> Also, can you test that a length of 32 works?
Done.
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:148: gs->remainder = ((gs->hdr[0] & 0x3f) << 8) | gs->hdr[1];
On 2016/03/02 17:22:07, mt wrote:
> I was looking at this and wondering why it is that the second bit of the
length
> (i.e., 0x40) is ignored here. I realize that the spec (at least the one I
> found) says that, but you could read that bit, which would let you hoist this
> line (and any line that sets ssl2gs->isV2 to true) up into a common section.
So the spec I found [1] says the second bit denotes whether the record being
sent is a security escape (whatever that means). It says that there are no
escapes defined, and there never will be. So it *should* be always zero but do
we want to rely on that? I doubt it makes a difference but the current solution
feels more correct.
[1] https://tools.ietf.org/html/draft-hickman-netscape-ssl-00https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:178: if (ssl2gs && ssl2gs->offset) {
On 2016/03/02 17:22:07, mt wrote:
> If you do as I suggest below, you can maintain the offset local to this
function
> and avoid the double-barrelled check. i.e.,
>
> PRUint8 v2Offset = 0;
> ...
> ssl2gs->isV2 = PR_TRUE;
> v2Offset = 2;
> ...
> ssl2gs->isV2 = PR_TRUE;
> v2Offset = 3;
> ...
> if (v2Offset) {
> gs->inbuf.len = 5 - v2Offset;
Done.
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:353: isServer = ss->sec.isServer && ss->ssl3.hs.ws ==
wait_client_hello;
On 2016/03/02 17:22:07, mt wrote:
> This is a bad name. Maybe acceptV2Hello.
Yeah, you're right. Changed.
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:429: if (ssl2gs.offset) {
On 2016/03/02 17:22:07, mt wrote:
> Since your only use for .offset in this function is as a flag, change it to
> ssl2gs.isV2 and make it PRBool. That keeps the offset local to the function
> above.
Done.
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:148: gs->remainder = ((gs->hdr[0] & 0x3f) << 8) | gs->hdr[1];
On 2016/03/02 19:10:14, ttaubert wrote:
> So the spec I found [1] says the second bit denotes whether the record being
> sent is a security escape (whatever that means). It says that there are no
> escapes defined, and there never will be. So it *should* be always zero but do
> we want to rely on that? I doubt it makes a difference but the current
solution
> feels more correct.
We should abort if that bit is set. My reading of that text (and I agree it's
super-vague) is that there is no way that this going to be something we can
parse as a v2 ClientHello. My suggestion would be to add a condition to the top
section:
((gs->hdr[0] >= content_change_cipher_spec &&
gs->hdr[0] <= content_application_data) || (gs->hdr[0] & 0x40))
Which suggests a small function: ssl3_IsLikelyV3Hello(const char* buf)
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):
https://codereview.appspot.com/287280043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:148: gs->remainder = ((gs->hdr[0] & 0x3f) << 8) | gs->hdr[1];
On 2016/03/02 19:47:13, mt wrote:
> On 2016/03/02 19:10:14, ttaubert wrote:
> > So the spec I found [1] says the second bit denotes whether the record being
> > sent is a security escape (whatever that means). It says that there are no
> > escapes defined, and there never will be. So it *should* be always zero but
do
> > we want to rely on that? I doubt it makes a difference but the current
> solution
> > feels more correct.
>
> We should abort if that bit is set. My reading of that text (and I agree it's
> super-vague) is that there is no way that this going to be something we can
> parse as a v2 ClientHello. My suggestion would be to add a condition to the
top
> section:
>
> ((gs->hdr[0] >= content_change_cipher_spec &&
> gs->hdr[0] <= content_application_data) || (gs->hdr[0] & 0x40))
>
> Which suggests a small function: ssl3_IsLikelyV3Hello(const char* buf)
Sounds good, did that.
https://codereview.appspot.com/287280043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):
https://codereview.appspot.com/287280043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:53: uint8_t *msg, size_t msg_len)
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> Indent issue.
Acknowledged.
https://codereview.appspot.com/287280043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc (right):
https://codereview.appspot.com/287280043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc:216:
client_->SetPacketFilter(i1);
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> You seem to be leaking the filter. Not good.
On IRC we figured that these tests don't actually leak the filter, they're using
TEST_F with a different base class. I agree though that that's confusing and
have created a special base class that provides the same convenience functions.
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9195: ss->clientHelloVersion = version;
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> I think you should not send an alert until you establish that this is offering
> some variant of SSLv3.
At alert_loser below I could not send anything if version < SSL3 but as the
previous patch removed SSL2 support, and we only support SSL2-formatted client
hellos to initiate a v3 connection, wouldn't it be okay to respond with an
alert?
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:152: ssl3_isLikelyV3Hello((const unsigned char*)&gs->hdr)) {
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> hdr is:
> unsigned char hdr[13]; /* ssl 2 & 3 or dtls */
>
> This automatically converts to unsigned char * on being passed to a function
> which takes unsigned char *, so no cast is required. The reason you are having
> to cast is that you are inappropriately taking the address.
Oops, bad mistake. Added another test to ensure we're properly ignoring SSLv2
with the security escape bit set.
To properly test this I changed ssl3_HandleRecord() to send a decode_error when
encountering an unknown content type. Let me know if we should use a different
error code or can't do that at all. If we can't I'd change the test to check
that only the server fails as the client doesn't know about any problems.
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:163: /* Is it a 3-byte header with padding? */
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> Add a comment here about the security escape not being handled.
Done.
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:173: gs->remainder > (MAX_FRAGMENT_LENGTH + 2048 + 5)) {
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> Why the +5? The remainder is not included in remainder?
>
> I realize this is old code.
Right, that makes no sense. Removed.
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:370: acceptV2Hello = ss->sec.isServer && ss->ssl3.hs.ws ==
wait_client_hello;
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> Why not just use a pointer to ssl2gs (NULL or &ssl2gs) as the sentinel here.
Done.
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:419: ssl2gs.padding = 0;
On 2016/03/11 09:49:38, ekr-webrtc wrote:
> Why are you setting the padding field when acceptV2Hello is false?
>
> Also, why not make this block local and initialize it.
Good idea, done.
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):
https://codereview.appspot.com/287280043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9195: ss->clientHelloVersion = version;
On 2016/03/11 16:59:34, ttaubert wrote:
> On 2016/03/11 09:49:38, ekr-webrtc wrote:
> > I think you should not send an alert until you establish that this is
offering
> > some variant of SSLv3.
>
> At alert_loser below I could not send anything if version < SSL3 but as the
> previous patch removed SSL2 support, and we only support SSL2-formatted client
> hellos to initiate a v3 connection, wouldn't it be okay to respond with an
> alert?
But someone might send you a v2 ClientHello. My suggestion would be to check the
version first and if it's SSLv3, just close the connection and if it's greater
then you can send an alert.
Issue 287280043: Bug 1228555 - Re-add support for SSLv2 client hellos including tests
(Closed)
Created 8 years, 1 month ago by ttaubert
Modified 7 years, 8 months ago
Reviewers: ekr, ekr-rietveld, mt
Base URL:
Comments: 58
