Bug1179338

Bug1179338

[mt, 2016-06-01 15:45:19]

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

https://codereview.appspot.com/300860043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:12376: }
Here's a crazy idea.  What if computing hashes for a hashType of
handshake_hash_record only grabbed a snapshot of the transcript?  Then, when you
"read" the hashes, you could do the hash of the type that you actually wanted.

I'm trying to find a way to encapsulate the damage here and this doesn't really
work very well for me.

More on this when it's not 1:30.

[kaie, 2016-06-01 15:58:22]

It seem you don't like the idea to pass along the msgLenForCertVerify parameter.
I personally don't understand why you're trying to avoid it, because the
existing code already does special things, such as passing the hashes result.

I believe your suggestion "grab snapshot" means, you want to perform a full copy
of the accumulated buffer of handshake messages.

If yes, isn't that a rather expensive runtime cost?

[mt, 2016-06-02 09:55:55]

On 2016/06/01 15:58:22, kaie wrote:
> It seem you don't like the idea to pass along the msgLenForCertVerify
parameter.
> I personally don't understand why you're trying to avoid it, because the
> existing code already does special things, such as passing the hashes result.
> 
> I believe your suggestion "grab snapshot" means, you want to perform a full
copy
> of the accumulated buffer of handshake messages.
> 
> If yes, isn't that a rather expensive runtime cost?

You don't need to copy it, you could just point at the requisite parts of the
handshake message buffer, since you know that is going to stick around.

Something like:

typedef struct {
    unsigned int len;
    SSLHashType hashAlg;
    union {
        PRUint8 raw[64];
        SSL3HashesIndividually s;
        SECItem record;  /* or struct { const unsigned char *data, unsigned int
len } record; */
    } u;
} SSL3Hashes;

When computing the hashes, if the hash type is _record, just have `record` point
to the part of the sslBuffer that contains the messages.

You can perform the hash computation when the data is needed.

The reason I suggested this is that it contains the complexity of dealing with
hash types to the functions that need to know about it anyway.

[mt, 2016-06-02 09:56:07]

> It seem you don't like the idea to pass along the msgLenForCertVerify
parameter.
> I personally don't understand why you're trying to avoid it, because the
> existing code already does special things, such as passing the hashes result.
> 
> I believe your suggestion "grab snapshot" means, you want to perform a full
copy
> of the accumulated buffer of handshake messages.
> 
> If yes, isn't that a rather expensive runtime cost?

You don't need to copy it, you could just point at the requisite parts of the
handshake message buffer, since you know that is going to stick around.

Something like:

typedef struct {
    unsigned int len;
    SSLHashType hashAlg;
    union {
        PRUint8 raw[64];
        SSL3HashesIndividually s;
        SECItem record;  /* or struct { const unsigned char *data, unsigned int
len } record; */
    } u;
} SSL3Hashes;

When computing the hashes, if the hash type is _record, just have `record` point
to the part of the sslBuffer that contains the messages.

You can perform the hash computation when the data is needed.

The reason I suggested this is that it contains the complexity of dealing with
hash types to the functions that need to know about it anyway.

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

https://codereview.appspot.com/300860043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4382: ssl3_clearSSLBuffer(sslBuffer *b)
Should this be moved to sslsecur.c and named sslBuffer_Clear()?

https://codereview.appspot.com/300860043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4495: if (ss->version != SSL_LIBRARY_VERSION_TLS_1_2 &&
ss->ssl3.hs.messages.len > 0) {
ss->ssl3.hs.hashType == handshake_hash_record

[kaie, 2016-06-02 14:43:38]

As of today, SSL3Hashes is purely the result of a hash.

IIUC, you want to redefine its meaning, so that it can be both input and output.

With TLS 1.2, you want it to transport the input information.

Because SSL3Hashes itself doesn't carry information about its current state
(still input, or already computed output), as a result, we must depend on the
context, and the timing of execution, to know whether it's still input, or
already computed output.

That's doable, but IMHO it's more complicated to understand for future readers
of the code.

[kaie, 2016-06-02 14:44:50]

And adding the state "is input" or "is output" to SSL3Hashes probably isn't a
good idea, because it would require that we touch all existing code that deals
with the struct.

[kaie, 2016-06-02 16:23:22]

I've implemented the suggestion, and I must confess, it looks better than I
thought.
New patch forthcoming.

[kaie, 2016-06-02 16:33:07]

Patch v13 addresses all your comments.

[mt, 2016-06-02 21:35:17]

I was thinking that the SSL3Hashes thing would be a little more extensive.

We currently consume the raw hashes in only a small number of places:

ssl3_SignHashes and ssl3_VerifySignedHashes
ssl3_ComputeCommonKeyHash
tls_ComputeExtendedMasterSecretInt
ssl3_ComputeTLSFinished

You could change ssl3_ComputeHandshakeHashes() so that it saved
pointer_to_hash_input only.  Then, in each of the above places, you could add a
call to a function that hashed the handshake and updated the `raw` value.

Currently, you call your new functions for hashing the entire transcript in two
places, this would change that to 5 places where you call out to the new
function, but would encapsulate the logic better, I think.

The only wrinkle is that for ssl3_SignHashes, ssl3_VerifySignedHashes you would
need to be passed the hash function that it would use for the signature.  For
certificate verify, it would need to to look at the tls12CertVerifyHash rather
than the cipher suite hash.

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

https://codereview.appspot.com/300860043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6797: */
IMPORTANT: We have a problem for 1.3 here.  The code now (and maybe even
before), was correctly updating the handshake hash with the hash that is in the
cipher suite.  But we still need to be looking at the supported signature
algorithms before we decide on the signature.

That means changing ssl3_SignHashes so that looks at the value of
tls12CertVerifyHash (or it can be passed in) rather than the hashes->hashAlg.  I
assume that also means the same change to ssl3_VerifySignedHashes.

https://codereview.appspot.com/300860043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6818: }
If you changed ssl3_HandleHandshakeMessage so that it computed hashes for
certificate_verify, then you don't need to run this code.  See also the top
comment.

https://codereview.appspot.com/300860043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:10370: errCode = SSL_ERROR_DIGEST_FAILURE;
This could be moved to the new function that I propose, which you could call
above.

https://codereview.appspot.com/300860043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:12336: *
Check for trailing whitespace here.

[kaie, 2016-06-02 22:14:56]

On 2016/06/02 21:35:17, mt wrote:
> You could change ssl3_ComputeHandshakeHashes() so that it saved
> pointer_to_hash_input only.  Then, in each of the above places, you could add
a
> call to a function that hashed the handshake and updated the `raw` value.

You didn't make it clear, if you suggest to change ssl3_ComputeHandshakeHashes
for handshake_hash_record, only, or for everything.

I'm guessing you want it for handshake_hash_record, only,
because for handshake_hash_combo the pointer_to_hash_input wouldn't suffice.

Now, when ssl3_SignHashes() is called, how can it decide whether
hashes->u already contains the final hashes, or if if must consume
pointer_to_hash_input to calculate the hash, first?

[kaie, 2016-06-02 22:23:24]

> Now, when ssl3_SignHashes() is called, how can it decide whether
> hashes->u already contains the final hashes, or if if must consume
> pointer_to_hash_input to calculate the hash, first?

I believe the answer is, we either must pass that information (already raw or
not) to those functions, too, or alternatively, add the conversion to the final
raw hash just before calling functions like ssl3_SignHashes.

Also, ssl3_SignHashes doesn't know whether it needs to use the bypass or
non-bypass version, in addition to passing the hash function.

It seems that a lot of new things must be passed around. I don't see yet how the
idea would make things better.

[kaie, 2016-06-02 22:27:43]

In case of ssl3_ComputeCommonKeyHash, learning if hashes is raw or input-only,
would require to pass through that information through the intermediate callers
ssl3_ComputeExportRSAKeyHash and ssl3_ComputeDHKeyHash, none of which currently
knows SSL3HandshakeHashType we're using.

[kaie, 2016-06-02 22:38:59]

I think for clarity, your design would require a new data structure like
SSL3HashInstructions, that carries all knowledge about the input, and all flags
that must be considered when calculating, such as buffering mode, input
location, hash function, whether bypass is used or not, etc., or a reference to
the already pre-computed hash.

Then, all functions that you'd expect to do calculations potentially at the last
moment, would pass that on.

But frankly, I'm still not convinced why that would be better. I'd prefer a
justification why that redesign of the code would actually be worth the effort.

[kaie, 2016-06-03 01:22:32]

Patch v14 calls ssl3_DecideTls12CertVerifyHash for TLS 1.3

It adds an assertion requested by EKR for TLS 1.3, that he offered to fix later,
after merging with his WIP.

I found that SHA-1 is forbidden with cert_verify in TLS 1.3, so I've added a
check for that.

[mt, 2016-06-03 01:57:48]

I think that I found a bug and I think that special-casing ssl_hash_none isn't
necessary.

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

https://codereview.appspot.com/300860043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:4454: ss->ssl3.hs.hashType = handshake_hash_record;
Nit: this code appears twice.  Why not put this check above the #ifdef so that
it runs first?

https://codereview.appspot.com/300860043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6800: ss->ssl3.hs.tls12CertVerifyHash != ssl_hash_none) {
The second check can be tls12CertVerifyHash != ssl3_GetSuitePrfHash()

(See below for more, but in short, I don't think that we should special case
ssl_hash_none.)

I need to double-check my understanding here.  If the hash IS the same as the
PRF hash, we just rely on ssl3_ComputeHandshakeHashes below, right?

https://codereview.appspot.com/300860043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6850: PORT_Assert(hashes.hashAlg ==
ss->ssl3.hs.tls12CertVerifyHash);
hashes.hashAlg should be the PRF hash, right?  You could reduce this check to:

if (hashes.hashAlg != ss->ssl3.hs.tls12CertVerifyHash) {
  PORT_Assert(0);
  return SECFailure;
}

That is, if you take my suggestions below.

https://codereview.appspot.com/300860043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10043: } else if (hashAlg == ss->ssl3.hs.suite_def->prf_hash)
{
BUG: Use ssl3_GetSuitePrfHash() here or you will incorrectly accept
ssl_hash_none for older cipher suites.  You get away with this currently because
you special case ssl_hash_none.

https://codereview.appspot.com/300860043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10045: } else if (otherHashAlg == ssl_hash_none) {
BUG: Remove the "else" from this second clause.  Otherwise we won't settle on
SHA-1 unless we also prefer it (not that that is strictly bad, but if it's the
only option we're given we might as well use it).

The first else could also be removed if you like.

https://codereview.appspot.com/300860043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10054: ss->ssl3.hs.tls12CertVerifyHash = ssl_hash_none; /* Use
suite PRF hash. */
I think that you should just set the value to ssl3_GetSuitePrfHash() here.  We
already have a special case for ssl_hash_none in the code, this creates a second
one.  I don't think that we need the extra information either.

[kaie, 2016-06-03 02:37:42]

Thanks for your suggestions. I've made all changes, except the assertion for TLS
1.3, which I'm still wrapping in != none, because the TLS 1.3 server code also
uses send-cert-verify, and in that case, we haven't gone through the decide
function, and our cert-verify-hash is therefore none.

Patch tested locally and still works.

[mt, 2016-06-03 02:52:50]

LGTM, r+

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

https://codereview.appspot.com/300860043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6813: }
Nit, you could combine this block into a ssl3_ComputeHandshakeHashFromRecord(). 
I see this appears in a few places.

[kaie, 2016-06-03 02:59:50]

On 2016/06/03 02:52:50, mt wrote:
>
https://codereview.appspot.com/300860043/diff/140001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:6813: }
> Nit, you could combine this block into a
ssl3_ComputeHandshakeHashFromRecord(). 
> I see this appears in a few places.

will land that as a follow up today, if the commit sticks