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

fix plaintext information leak #1

Closed
wants to merge 1 commit into from

Conversation

ryancdotorg
Copy link
Collaborator

This changes the salt for a file to be the HMAC of its contents keyed
with filename:password instead of the plain hash of its contents. Pre-
change an attacker may make guesses at the files contents and confirm
them with high probability by checking to see whether the salt matches.
This would be a concern in the case of a config file where almost
everything is known, for example everything excpet for a password is
known. This change also prevents an attacker from seeing that the
contents of two encrypted files are the same.

There may still be other security issues.

This commit doesn't change the version number or try to maintain
compatibility with older repos.

This changes the salt for a file to be the HMAC of its contents keyed
with filename:password instead of the plain hash of its contents. Pre-
change an attacker may make guesses at the files contents and confirm
them with high probability by checking to see whether the salt matches.
This would be a concern in the case of a config file where almost
everything is known, for example everything excpet for a password is
known. This change also prevents an attacker from seeing that the
contents of two encrypted files are the same.

There may still be other security issues.

This commit doesn't change the version number or try to maintain
compatibility with older repos.
@elasticdog
Copy link
Owner

I appreciate you looking through the code. This makes a lot of sense. I'll do some testing and merge later tonight.

@ryancdotorg
Copy link
Collaborator Author

I should probably point out that I'm unsure this is the safest way to ensure that files are deterministiclly encrypted. It should, to the best of my knowledge, be totally safe based on the security properties of HMAC[1], namely that it is "privacy preserving". Also note that we're using HMAC as a PRF rather than a MAC.

Also, it would be better (not sure how much) to use hmac(hmac(password, filename), file_contents), but the changed code is already tedious.

  1. http://cseweb.ucsd.edu/~mihir/papers/hmac-new.html

@elasticdog
Copy link
Owner

Thanks a ton for the contribution and information! I updated the documentation and merged in your change.

@elasticdog elasticdog closed this Feb 12, 2014
@elasticdog
Copy link
Owner

Also, since the repo is only a few days old and pre-1.0, I haven't touched the version number either. I'll be following Semantic Versioning and will try to be better at managing that. Hopefully there won't be too many more backward-incompatible changes.

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.

2 participants