Skip to content

Commit

Permalink
Deal gracefully with nil parameters in comparison
Browse files Browse the repository at this point in the history 
Regression introduced in a8f9c92
  • Loading branch information
@mdpye
mdpye committed Mar 4, 2015
1 parent a1fe7d5 commit 3d9a0bd
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions lib/signature.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ def validate_signature!(token)

# Constant time string comparison
def identical?(a, b)
return true if a.nil? && b.nil?
return false if a.nil? || b.nil?
return false unless a.bytesize == b.bytesize
a.bytes.zip(b.bytes).reduce(0) { |memo, (a, b)| memo += a ^ b } == 0
end
Expand Down

4 comments on commit 3d9a0bd

@markburns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this invalidate part of the purpose of this method?
I.e. these checks short-circuit the method invalidating its constant-time-ness. Also applying to @stevegraham's short circuit in the original commit of #a8f9c92c8ae4423733f259429e5ea8e13438df38
If so maybe it makes more sense to convert a and b into byte arrays of identical length then do the constant time comparison. Don't know if that conversion is even possible in constant time though.

@mdpye
Copy link
Contributor Author

@mdpye mdpye commented on 3d9a0bd Mar 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, it only allows an attacker to discern the difference between some signature and no signature being provided.

As for the length check, I think it's practical. The SHA256 digest is of a specific and known length. The attacker can find out how long a signature is supposed to be, but that's already documented, and so not useful.

@stevegraham
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely, neither of those values should be nil? b should simply not be nil, it's coming out of a private method. For a, I don't think this is the right place to check it is nil. Perhaps raise an argument error in the constructor if required properties are missing?

The purpose of the constant time string comparison is because String#== enumerates the strings character by character comparing the values. The method returns on the first instance of the values at a particular index being different. That SHA hash lengths are known is besides the point. An attacker could work out how much of a valid signature they have from analysing response times. This is called a timing attack.

@markburns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks guys. I was aware of the timing attack, this just kind of stood out as the comment doesn't quite match the implementation, but it makes sense that it's a comparison against a SHA so not relevant. Anyway, satisfies my curiosity.
Does seem like a test is missing though if it's fixing a regression.

Please sign in to comment.