Use a constant time comparison, without a length check short-circuit

I don't think it's possible to constant-time compare two arrays of different lengths.

cc @agl @FiloSottile to decide if this issue is actionable.

@weingart

It's true that hashing the passphrase will create a bitstring comparable in constant time, but the suggestion of bcrypt sounds rather expensive. Is there a particular reason you would choose this function over another?

Note, this could make the implementation also more safe
from casual memory snooping.

I would prefer to step away from such corollaries, it creates one more location in memory associated with the passphrase.

I don't think it's possible to constant-time compare two arrays of different lengths.

Would something like [0], but written in Go assembly, not be constant-time? It is supposed to be constant time with respect to length of one string (the attacker controlled one).
[0] https://nachtimwald.com/2017/04/02/constant-time-string-comparison-in-c/

/cc @hanwen too

the bug here is that we're checking the lengths at all, as ConstantTimeCompare already does so

https://golang.org/src/crypto/subtle/constant_time.go?s=501:542#L2

see also https://go.googlesource.com/go/+/446bfffcd6cfcaca141988a9d844aac61a704866 and issue
#7304 (which coincidentally, I filed.)

Change https://golang.org/cl/110068 mentions this issue: ssh/agent: remove len check in Unlock

the bug here is that we're checking the lengths at all, as ConstantTimeCompare already does so

I've sent a fix for this while we wait for a decision regarding making Unlock constant-time even if the arrays have different length.

I've submitted the aforementioned change.

I don't think there is anything left to be done here.

But the submitted fix does not address the issue: that comparing two strings of different lengths with subtle.ConstantTimeCompare takes less time than comparing two strings of equal length. Is the code I linked to not correct?

strings in Go are not null terminated, so the code you link to can't work.

you could make subtle.ConstantTimeCompare constant time for differently sized input strings, eg. by doing

l :=  len(x) - len(y)
var bs [4]byte
binary.PutInt(l, bs[:])
byte v = bs[0] | bs[1] | bs[2] | bs[3]
for i := 0; i < len(x) && i < len(y); i++ {
  v |= x[i]^y[i]
}

but it will still leak the size of the smallest array, and it takes work to do this portably.

I am not a cryptographer, but it seems to me that the linked code could be ported straightforwardly to Go by replacing "byte is NULL" by "current index >= length of string".

I am not a cryptographer, but it seems to me that the linked code could be ported straightforwardly to Go by replacing "byte is NULL" by "current index >= length of string".

It is not that simple, just doing that would result in a read out of slice bounds in the first line of the loop.