New tls channel id version for OpenSSL

New tls channel id version for OpenSSL

New tls channel id version extracted from patch 0015-channelid.patch attached to http://crbug.com/366961.

BUG=366961

[haavardm, 2014-04-28 19:27:18]

Wtc please review.

I just you assigned the bug you yourself just now, however I had the patch more
or less ready.

I've removed all server code and also removed the code keeping the server
backward compatible with the initial version.

[haavardm, 2014-04-28 19:28:21]

Wtc please review.

I just you assigned the bug you yourself just now, however I had the patch more
or less ready.

I've removed all server code and also removed the code keeping the server
backward compatible with the initial version.

This version does not contain the patch text files. I figure it's best to
generate those when the patch has been approved  to avoid too much overhead.

[haavardm, 2014-04-28 20:26:15]

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c
File openssl/ssl/s3_clnt.c (right):

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c#ne...
openssl/ssl/s3_clnt.c:594: ret = tls1_record_handshake_hashes_for_channel_id(s);
Before I knew about the server patch, I calculated the original hash using

s->session->original_handshake_hash = 
 SSL_get_finished(s, s->session->original_handshake_hash,							
                  sizeof(s->session->original_handshake_hash));

To me this seems a bit simpler as we don't need all the changes in
openssl/ssl/t1_lib.c The finish message is calculated in ssl3_send_finished(),
so the result should be the same?

[wtc, 2014-04-30 14:11:50]

Patch set 2 LGTM. Thanks.

Adam: I have a question for you in openssl/ssl/s3_clnt.c.

Haavard: do we not have time generate a patch file now?

Did you include the server-side changes?

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c
File openssl/ssl/s3_clnt.c (right):

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c#ne...
openssl/ssl/s3_clnt.c:586: if (s->s3->tlsext_channel_id_valid)

Adam: This seems to mean that if the original full handshake doesn't involve
ChannelID, then future handshakes that resume this sesssion won't be able to use
ChannelID. Is this correct?

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c#ne...
openssl/ssl/s3_clnt.c:594: ret = tls1_record_handshake_hashes_for_channel_id(s);

On 2014/04/28 20:26:15, haavardm wrote:
> Before I knew about the server patch, I calculated the original hash using
> 
> s->session->original_handshake_hash = 
>  SSL_get_finished(s, s->session->original_handshake_hash,							
>                   sizeof(s->session->original_handshake_hash));

If SSL_get_finished returns the contents of the Finished message, then it is not
the handshake hash:

      struct {
          opaque verify_data[verify_data_length];
      } Finished;

      verify_data
         PRF(master_secret, finished_label, Hash(handshake_messages))
            [0..verify_data_length-1];

What ChannelID uses is just Hash(handshake_messages).

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl.h
File openssl/ssl/ssl.h (right):

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl.h#newcod...
openssl/ssl/ssl.h:556: unsigned int original_handshake_hash_len;

Do we need to initialize original_handshake_hash_len to 0 explicitly, or is the
entire ssl_session_st struct zeroed at5 creation time?

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h
File openssl/ssl/ssl_locl.h (right):

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h#n...
openssl/ssl/ssl_locl.h:1105: int tls1_handshake_digest(SSL *s, unsigned char
*out, size_t out_len);

Nit: tls1_handshake_digest => tls1_handshake_hash ?

Unless handshake digest is the term commonly used in OpenSSL.

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h#n...
openssl/ssl/ssl_locl.h:1162: int tls1_record_handshake_hashes_for_channel_id(SSL
*s);

Nit: handshake_hashes => handshake_hash (singular)?

[haavardm, 2014-04-30 17:08:59]

On 2014/04/30 14:11:50, wtc wrote:
> Patch set 2 LGTM. Thanks.
> 
> Adam: I have a question for you in openssl/ssl/s3_clnt.c.
> 
> Haavard: do we not have time generate a patch file now?
> 
I just didn't want to generate the patch files until all the review changes in
the code
was done, as the patch itself adds little value to the review and is a pain to
generate. 

So I'll do the changes you propose and add the text file in the next CL upload.

Due to Mai 1 is a day off here in Norway, and my regular  20% University
position at 
Fridays I'm not back to Opera until Monday. Is it a hurry to get this in? 



> Did you include the server-side changes?

I removed all server changes along with backward compatibility with first
version.

They method I used was to create this patch, was to apply the existing chromium
patch 
to a clean openssl checkout and the server patch to another clean  openssl
checkout. 
Then I diffed the results to find out what the difference was between existing 
client and server, and finally removed all server code and channel id version
code.

> 
> https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c
> File openssl/ssl/s3_clnt.c (right):
> 
>
https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c#ne...
> openssl/ssl/s3_clnt.c:586: if (s->s3->tlsext_channel_id_valid)
> 
> Adam: This seems to mean that if the original full handshake doesn't involve
> ChannelID, then future handshakes that resume this sesssion won't be able to
use
> ChannelID. Is this correct?
> 
>
https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c#ne...
> openssl/ssl/s3_clnt.c:594: ret =
tls1_record_handshake_hashes_for_channel_id(s);
> 
> On 2014/04/28 20:26:15, haavardm wrote:
> > Before I knew about the server patch, I calculated the original hash using
> > 
> > s->session->original_handshake_hash = 
> >  SSL_get_finished(s, s->session->original_handshake_hash,							
> >                   sizeof(s->session->original_handshake_hash));
> 
> If SSL_get_finished returns the contents of the Finished message, then it is
not
> the handshake hash:
> 
>       struct {
>           opaque verify_data[verify_data_length];
>       } Finished;
> 
>       verify_data
>          PRF(master_secret, finished_label, Hash(handshake_messages))
>             [0..verify_data_length-1];
> 
> What ChannelID uses is just Hash(handshake_messages).
> 
> https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl.h
> File openssl/ssl/ssl.h (right):
> 
>
https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl.h#newcod...
> openssl/ssl/ssl.h:556: unsigned int original_handshake_hash_len;
> 
> Do we need to initialize original_handshake_hash_len to 0 explicitly, or is
the
> entire ssl_session_st struct zeroed at5 creation time?
> 
> https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h
> File openssl/ssl/ssl_locl.h (right):
> 
>
https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h#n...
> openssl/ssl/ssl_locl.h:1105: int tls1_handshake_digest(SSL *s, unsigned char
> *out, size_t out_len);
> 
> Nit: tls1_handshake_digest => tls1_handshake_hash ?
> 
> Unless handshake digest is the term commonly used in OpenSSL.
> 
>
https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h#n...
> openssl/ssl/ssl_locl.h:1162: int
tls1_record_handshake_hashes_for_channel_id(SSL
> *s);
> 
> Nit: handshake_hashes => handshake_hash (singular)?

[haavardm, 2014-04-30 17:18:50]

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl.h
File openssl/ssl/ssl.h (right):

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl.h#newcod...
openssl/ssl/ssl.h:556: unsigned int original_handshake_hash_len;
On 2014/04/30 14:11:50, wtc wrote:
> 
> Do we need to initialize original_handshake_hash_len to 0 explicitly, or is
the
> entire ssl_session_st struct zeroed at5 creation time?

memset(ss,0,sizeof(SSL_SESSION)) happens in SSL_SESSION_new()

[wtc, 2014-04-30 18:30:12]

On 2014/04/30 17:08:59, haavardm wrote:
> Is it a hurry to get this in?

No. This can wait until next Monday.

> They method I used was to create this patch, was to apply the existing
chromium
> patch 
> to a clean openssl checkout and the server patch to another clean  openssl
> checkout. 
> Then I diffed the results to find out what the difference was between existing

> client and server, and finally removed all server code and channel id version
> code.

That's a good method. Thanks. It would be nice to keep the server code, but it's
not required.

[agl, 2014-05-02 17:58:53]

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c
File openssl/ssl/s3_clnt.c (right):

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/s3_clnt.c#ne...
openssl/ssl/s3_clnt.c:586: if (s->s3->tlsext_channel_id_valid)
On 2014/04/30 14:11:50, wtc wrote:
> 
> Adam: This seems to mean that if the original full handshake doesn't involve
> ChannelID, then future handshakes that resume this sesssion won't be able to
use
> ChannelID. Is this correct?

Yes.

[haavardm, 2014-05-05 14:12:45]

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h
File openssl/ssl/ssl_locl.h (right):

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h#n...
openssl/ssl/ssl_locl.h:1105: int tls1_handshake_digest(SSL *s, unsigned char
*out, size_t out_len);
On 2014/04/30 14:11:50, wtc wrote:
> 
> Nit: tls1_handshake_digest => tls1_handshake_hash ?
> 
> Unless handshake digest is the term commonly used in OpenSSL.

Handshake digest seems to be the term OpenSSL uses.

https://codereview.chromium.org/259963009/diff/40001/openssl/ssl/ssl_locl.h#n...
openssl/ssl/ssl_locl.h:1162: int tls1_record_handshake_hashes_for_channel_id(SSL
*s);
On 2014/04/30 14:11:50, wtc wrote:
> 
> Nit: handshake_hashes => handshake_hash (singular)?

The naming here seems to be a bit inconsistent. I would argue that plural is
correct and that session->original_handshake_hash should be renamed to
original_hashes, since session->original_handshake_hash contains a concatenation
of all hashes in the handshake.

[haavardm, 2014-05-05 15:26:53]

wtc: Uploaded patch files without other changes. PTAL

[wtc, 2014-05-05 22:09:46]

Patch set 3 LGTM.

https://codereview.chromium.org/259963009/diff/80001/patches.chromium/0014-ne...
File patches.chromium/0014-new_channelid.patch (right):

https://codereview.chromium.org/259963009/diff/80001/patches.chromium/0014-ne...
patches.chromium/0014-new_channelid.patch:1: diff -burN
android-openssl.orig/include/openssl/ssl.h android-openssl/include/openssl/ssl.h

Do we need to add the patch file name "0014-new_channelid.patch" to some file?
Or is it enough to just add the patch file to the patches.chromium/ directory?

[haavardm, 2014-05-06 07:22:53]

https://codereview.chromium.org/259963009/diff/80001/patches.chromium/0014-ne...
File patches.chromium/0014-new_channelid.patch (right):

https://codereview.chromium.org/259963009/diff/80001/patches.chromium/0014-ne...
patches.chromium/0014-new_channelid.patch:1: diff -burN
android-openssl.orig/include/openssl/ssl.h android-openssl/include/openssl/ssl.h
On 2014/05/05 22:09:47, wtc wrote:
> 
> Do we need to add the patch file name "0014-new_channelid.patch" to some file?
> Or is it enough to just add the patch file to the patches.chromium/ directory?

It is enough to add the patch file. The numbering in front makes sure they are
applied in correct order.

[haavardm, 2014-05-06 07:23:44]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-05-06 07:23:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haavardm@opera.com/259963009/80001

[commit-bot: I haz the power, 2014-05-06 07:24:12]

Change committed as 268457

[haavardm, 2014-05-06 09:00:42]

A revert of this CL has been created in
https://codereview.chromium.org/270103002/ by haavardm@opera.com.

The reason for reverting is: I was a little quick committing this. If someone
adds another OpenSSL patch and rolls OpenSSL version forward in DEPS the current
channel id test will fail on Android, since tls lite has not yet been updated.
I'll revert and sync commit and DEPS update with commit of TLS lite..

[haavardm, 2014-05-08 15:45:50]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-05-08 15:47:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haavardm@opera.com/259963009/80001

[commit-bot: I haz the power, 2014-05-08 15:48:34]

Change committed as 269063