Skip to content
Permalink
Browse files Browse the repository at this point in the history
Securely compare signature in derivation_endpoint
Using regular string comparison when comparing signature has different
performance depending on how much of the string matched, which can make
derivation_endpoint susceptible to timing attacks. We avoid that by
using `Rack::Utils.secure_compare` instead.

Big thanks to @esparta for reporting this.
  • Loading branch information
janko committed Oct 4, 2020
1 parent 398a27f commit 1b27090
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 1 deletion.
2 changes: 2 additions & 0 deletions CHANGELOG.md
@@ -1,5 +1,7 @@
## master

* `derivation_endpoint` – Avoid possibility of timing attacks when comparing signatures (@esparta)

* `derivatives` – Avoid downloading the attached file when calling default no-op processor (@janko)

* `derivatives` – Add `:download` processor setting for skipping downloading source file (@jrochkind, @janko)
Expand Down
2 changes: 1 addition & 1 deletion lib/shrine/plugins/derivation_endpoint.rb
Expand Up @@ -739,7 +739,7 @@ def verify_url(url)
def verify_signature(string, signature)
if signature.nil?
fail InvalidSignature, "missing \"signature\" param"
elsif signature != generate_signature(string)
elsif !Rack::Utils.secure_compare(signature, generate_signature(string))
fail InvalidSignature, "provided signature does not match the calculated signature"
end
end
Expand Down

3 comments on commit 1b27090

@esparta
Copy link

@esparta esparta commented on 1b27090 Oct 5, 2020

Choose a reason for hiding this comment

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

Nice! :)
Thanks, @janko !

@hmistry
Copy link
Member

Choose a reason for hiding this comment

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

@esparta
Copy link

Choose a reason for hiding this comment

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

that's nice, all using that particular combination of rack & OpenSSL versions will have the benefit of a better speed :)

Please sign in to comment.