code review 118750043: crypto/subtle: make ConstantTimeCompare return zero for...

crypto/subtle: make ConstantTimeCompare return zero for args of different length.

This is more useful than panicking, since otherwise every caller needs
to do the length check before calling; some will forget, and have a
potential submarine crasher as a result. Other implementations of this
functionality do a length check.

This is backward compatible, except if someone has written code that
relies on this panicking with different length args. However, that was
not the case before Go 1.3 either.

Updates issue 7304.

[dsymonds, 2014-07-16 05:47:05]

Hello agl (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[minux, 2014-07-16 06:24:26]

this will leak the length information which might or might not be
acceptable.
i think the correct answer to this problem belongs to the caller (i.e.
whether the length is public information), and compare shouldn't take over
the responsibility.

the subtle package is supposed to be tricky to use correctly, so imo we
don't need to do this to simplify the client code.

[dsymonds, 2014-07-16 06:27:40]

Well, if length information is sensitive then we don't provide a way
to check that in constant time (and in general you wouldn't be able
to), so it's not like the caller can do anything better. And
comparable code bases in C++ and Java that I've seen do exactly this.
But I'm waiting to hear what agl has to say.

[hanwen-google, 2014-07-16 07:54:27]

On 2014/07/16 06:24:26, minux wrote:
> this will leak the length information which might or might not be
> acceptable.
> i think the correct answer to this problem belongs to the caller (i.e.
> whether the length is public information), and compare shouldn't take over
> the responsibility.
> 
> the subtle package is supposed to be tricky to use correctly, so imo we
> don't need to do this to simplify the client code.

A comparison of N bytes will take about N time, so you'll always leak some
information about the length.

[minux, 2014-07-16 18:39:53]

On Wed, Jul 16, 2014 at 3:54 AM, <hanwen@google.com> wrote:

> On 2014/07/16 06:24:26, minux wrote:
>
>> this will leak the length information which might or might not be
>> acceptable.
>> i think the correct answer to this problem belongs to the caller (i.e.
>> whether the length is public information), and compare shouldn't take
>>
> over
>
>> the responsibility.
>
>  the subtle package is supposed to be tricky to use correctly, so imo
>>
> we
>
>> don't need to do this to simplify the client code.
>
> A comparison of N bytes will take about N time, so you'll always leak
> some information about the length.
>
But the length should be the same every time, so the attacker won't learn
much.

early termination means it leaks whether the user provided value is of the
same length as the other value. This is different.

[agl1, 2014-07-16 22:59:47]

If you are doing a constant-time compare with different length inputs then I
think you have a bug. That's what the panic was designed to highlight.

Returning 0 isn't too unsafe, but I worry that it's just papering over a
conceptual failure in the calling code.

(I could be convinced with examples.)

[agl1, 2014-07-21 23:41:26]

I was convinced.

LGTM.

[dsymonds, 2014-07-22 00:08:30]

*** Submitted as https://code.google.com/p/go/source/detail?r=c36d3f9adce2 ***

crypto/subtle: make ConstantTimeCompare return zero for args of different
length.

This is more useful than panicking, since otherwise every caller needs
to do the length check before calling; some will forget, and have a
potential submarine crasher as a result. Other implementations of this
functionality do a length check.

This is backward compatible, except if someone has written code that
relies on this panicking with different length args. However, that was
not the case before Go 1.3 either.

Updates issue 7304.

LGTM=agl
R=agl, minux, hanwen
CC=golang-codereviews
https://codereview.appspot.com/118750043