Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signatures are compared with potentially unsafe == #156

Closed
drosseau opened this issue Aug 10, 2018 · 2 comments · Fixed by #209
Closed

Signatures are compared with potentially unsafe == #156

drosseau opened this issue Aug 10, 2018 · 2 comments · Fixed by #209
Assignees

Comments

@drosseau
Copy link

drosseau commented Aug 10, 2018

Signatures are compared with normal string equality as shown here: https://github.com/oauth-xx/oauth-ruby/blob/master/lib/oauth/signature/base.rb#L54. Ruby string comparisons are done with memcmp so this could potentially leak timing info although this will vary depending on the memcmp implementation.

In reality, the chance of this being exploited is very low - it should be basically nil with proper nonce and timestamp checking by the users of this library (unless they're using the PLAINTEXT signature method) - but it is probably worth while to ensure this comparison is constant time.

I saw a pretty straightforward comparison function that would work used here

def safe_compare a, b
  check = a.bytesize ^ b.bytesize
  a.bytes.zip(b.bytes) { |x, y| check |= x ^ y.to_i }
  check == 0
end
@pboling
Copy link
Member

pboling commented Aug 11, 2018

Very interesting. I will look into the change when I get a chance.

@sporkmonger
Copy link

FWIW, that change should literally just be a drop-in replacement.

@pboling pboling self-assigned this Oct 31, 2021
pboling added a commit that referenced this issue Oct 31, 2021
- Closes #156

Signed-off-by: Peter Boling <peter.boling@gmail.com>
pboling added a commit that referenced this issue Nov 1, 2021
- Closes #156

Signed-off-by: Peter Boling <peter.boling@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants