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

use HMAC and timing safe comparison #883

Merged
merged 2 commits into from
Nov 7, 2016
Merged

Conversation

zt-initech
Copy link
Contributor

Instead of naive code vulnerable to length-extension attack and instead of short-circuiting string comparison.

https://en.wikipedia.org/wiki/Hash-based_message_authentication_code#Design_principles

https://docs.python.org/2/library/hmac.html#hmac.compare_digest

… use timing safe hash compare instead of short circuiting string compare
@aaugustin
Copy link
Contributor

I'm not a fan of changes that suggest that it might be feasible, let alone a workable idea, to run the debug toolbar in untrusted environments. But people will do it anyway, and defense in depth is a good thing. For this reason, I'm in favor of making these changes.

Travis CI says the PR doesn't work on Python 3.

@codecov-io
Copy link

codecov-io commented Sep 27, 2016

Current coverage is 77.03% (diff: 80.00%)

Merging #883 into master will increase coverage by 0.02%

@@             master       #883   diff @@
==========================================
  Files            30         30          
  Lines          1635       1637     +2   
  Methods           0          0          
  Messages          0          0          
  Branches        244        244          
==========================================
+ Hits           1259       1261     +2   
  Misses          302        302          
  Partials         74         74          

Powered by Codecov. Last update 637b0b7...22f88c6

@zt-initech
Copy link
Contributor Author

@aaugustin I think there are two possible states that make sense: either an authentication tag is not produced and is not checked, or it is produced securely and checked securely. The current situation in which an authentication tag is produced insecurely and then checked insecurely does not make sense. The code needs to change to be either in state 1 or 2. I prefer state 2, but state 1 is also good, because it emphasizes that debug toolbar is only for trusted environments.

@matthiask matthiask merged commit cef000a into jazzband:master Nov 7, 2016
@matthiask
Copy link
Member

Thanks!

@aaugustin
Copy link
Contributor

Yep. It's best to follow best-practices regardless of the context, since the context may change or not be what we expect. Thanks for the patch.

@zt-initech zt-initech deleted the hmac branch November 7, 2016 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants