ssh: Remove usages of and and or
#10571
Conversation
CT Test Results 2 files 29 suites 20m 0s ⏱️ Results for commit 885f27b. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
|
@Maria-12648430 I believe that functions is about comparing "password strings" in "equal time" for both the matching and the nonmatching case. I think that is is doubtfully that we can guarantee equal time in an erlang implementation, but even if we could I think that password authenticating seems like an option you should not use if you want to have a secure system. |
|
Interesting, I never thought of that. So the attack this defends against works as follows, correct? For simplicity, say passwords can only consist of lowercase letters, and in this case the password is "erlang". Assume the time taken for comparing two characters is always 1s, vastly exaggerated for illustration purposes. If you were trying to break the password, you would go about it as follows.
To find out "erlang" this way, you need only 182 tries (568s in this example), vs a naive brute-force approach ("a" .. "z", "aa" .. "zz", "aaa" ... "zzz", ..., "aaaaaa", ..., "erlang") which would need ~124 million tries (~4.2 years in this example):
Also, when you have a few characters right, you could try to guess at the rest, or at least part of the rest. In this example, when you got to "erl" in step 6, "erlang" may be worth a guess. Now, if we do not stop comparing at the first difference but always compare all characters of the attempted password, the timing attack does not work any more, and even for the brute-force method the time it takes goes up to ~23.8 years in this example. That all said, I agree that Erlang is too "erratic" to allow for the described attack to work in a real-world system where we're not talking seconds but microseconds, and probably ever will be. I also agree that password auth is in itself insecure. Then again, even if password auth is unsafe, it would be unwise to open up another hole in it, especially since we had it closed before. So @IngelaAndin WDYT, guess I'll revert this part of the changes? (But document better what it actually does and why 😉) |
|
I agree that it would not be a good idea to change the code so that it could be perceived as less secure than it could be. I think we should keep a best effort of making the comparison in equal time , although in a real system I think such analysis probably would be less reliable than if you tried it on a system based on more sequential not garbage collected language. However I have no experience in trying such analysis on Erlang systems. |
|
Yes, it is probably far fetched ;) Anyway, I will undo those changes 👍 |
Maria-12648430
force-pushed
the
ssh_remove_obsolete_boolean_operators
branch
from
f60471d to
c533017
Compare
|
@IngelaAndin I reverted the changes to |
|
@Maria-12648430 Looks good to me, I will leave final approval to @u3s that currently is our main responsible for the ssh application. |
lib/ssh/src/ssh_lib.erl
Outdated
| comp(R1, <<>>, Truth and false); | ||
| comp(<<B1, R1/binary>>, <<B2, R2/binary>>, Truth) -> | ||
| comp(R1, R2, B1 =:= B2 andalso Truth); | ||
| comp(<<B1 ,R1/binary>>, <<>>, _) -> |
There was a problem hiding this comment.
fix formatting pls. space after comma, not before if not a problem ;-)
| {{_, Reason}, client} when ((StateName =/= {connected,client}) | ||
| and (not Rengotation)) -> | ||
| {{_, Reason}, client} when StateName =/= {connected,client}, | ||
| not Rengotation -> |
There was a problem hiding this comment.
while at it, can you fix Rengotation typo pls?
lib/ssh/src/ssh_lib.erl
Outdated
| comp(R1, R2, Truth and (B1 == B2)); | ||
| comp(<<_,R1/binary>>, <<>>, Truth) -> | ||
| comp(R1, <<>>, Truth and false); | ||
| comp(<<B1, R1/binary>>, <<B2, R2/binary>>, Truth) -> |
There was a problem hiding this comment.
encouraged by #10571 (comment) i would like to complain a bit ;-) andalso used in this context does not look good as it short-circuits.
since comp/3 is not exported, we can maybe change it.
for example:
- adjust the Truth accumulator to be integer initiated with 0.
- use bxor instead of =:= (bxor resulting with 0 means number is equal)
- use bor instead andalso for accumulating result
- in the end interpret Accumulator of value 0 as "provided arguments are equal" and return true
- would above look better assuming only small integers will be processed?
"looking more secure" might reduce chances of receiving a security report for ssh, and having a lengthy discussions on how practical is the reported potential vulnerability and why we want/can't reject it. I would like to avoid this time waste if possible.
There was a problem hiding this comment.
andalso used in this context does not look good as it short-circuits.
Not sure I agree, but...
- adjust the Truth accumulator to be integer initiated with 0.
- use bxor instead of =:= (bxor resulting with 0 means number is equal)
- use bor instead andalso for accumulating result
- in the end interpret Accumulator of value 0 as "provided arguments are equal" and return true
... since the suggestion is sound (and IMO pretty elegant), I'll do it ;)
(Just for the record, the short-circuiting behavior of andalso is AFAICS of not consequence here. Or rather, for that reason, I put the comparisons on the left hand side of it (vs how it was before), so it will always be done. The andalso itself will also be always executed. The only difference we could have here is that andalso could potentially exhibit different runtime characteristic depending on whether the left hand side is true or false, I don't know. Another thing that may happen in theory is that the compiler could optimizie away ... andalso false to false if it could reason that the left hand side will always return a boolean (vs something else or failing); I don't think it does, but then again, I don't know, and it may in the future.)
- would above look better assuming only small integers will be processed?
What do you mean by that? 🤔
There was a problem hiding this comment.
would above look better assuming only small integers will be processed?
What do you mean by that? 🤔
binary operators execute differently for small integers than for large ?
There was a problem hiding this comment.
Yes, although they are "guaranteed small" in this case as it's all bitwise operations on bytes.
However, the length is leaked by the control flow diverging as soon as either list/bitstring ends. For starters it appears to crash if the provided password is longer than the correct one, and even if that were fixed, the divergent control flow is enough of a leak on its own.
I don't think it's worth trying to fix this part. Hashing is the way to go.
|
For what it's worth,
The funny thing with timing attacks is that with enough samples it doesn't matter how jittery the network is, you will see the difference anyway. |
Maria-12648430
force-pushed
the
ssh_remove_obsolete_boolean_operators
branch
from
c533017 to
885f27b
Compare
|
The hash_equals/2 in crypto was introduced here #4750 and actually it was merged by Hans N. The existing function in ssh was previously moved from crypto code to ssh. I do not know why he did not uses the new crypto function in ssh, maybe to support really old OpenSSL versions?! That should no longer be an issue. I think that for avoiding vulnerability reports on this functionality ( which I think is the goal) crypto:hash_equals makes the most sense. @u3s what do you think? |
lib/ssh/src/ssh_lib.erl
Outdated
| comp(R1, R2, Truth and (B1 == B2)); | ||
| comp(<<_,R1/binary>>, <<>>, Truth) -> | ||
| comp(R1, <<>>, Truth and false); | ||
| comp(<<B1, R1/binary>>, <<B2, R2/binary>>, Truth) -> |
There was a problem hiding this comment.
would above look better assuming only small integers will be processed?
What do you mean by that? 🤔
binary operators execute differently for small integers than for large ?
|
I agree using I accepted current proposal, but it would be great if we could refine it to use crypto:hash_equals/2. |
That leaks the length of the password, and is the main reason it's called It doesn't look like a huge modification, but then again this isn't my turf. |
|
we should do crypto:hash_equals/2 with using hashed passwords. @Maria-12648430 let me know if you have will and time to extend this PR. |
|
@u3s I'll see what I can do 👍 |
4 participants
This PR removes remaining usages of
andandorfromssh, as well as the then unneedednowarn_obsolete_bool_opdirectives.I have not the least idea what this
ssh_lib:comp/2function was about. It was moved over fromcryptowhere it was calledequal_const_time, as far as I could ferret out 🤷♀️ Both the original function incryptoas well as the move tossh_liband rename were done by @HansN.As far as I can tell, my revised implementation is semantically the same, except that it in the case of improper lists, my implementation will return
truefor any two equal improper lists, while the old implementation would returntrueonly for two equal improper lists if their last elements were equal binaries.