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

Improve encryption strength #506

Closed
wants to merge 1 commit into from
Closed

Improve encryption strength #506

wants to merge 1 commit into from

Conversation

john-shaffer
Copy link
Contributor

The encryption uses AES256, but the implementation silently discards half of the available entropy. That is, we take a sha256 hash of $secret_key, which is 64 hex chars, but the implementation only uses the first 32 chars, which is 256 bits of memory but only 128 bits of entropy. I instead call hex2bin to decode the 64 hex chars into 32 bytes of memory. There is a similar issue with the initial variate which I also patched.

There is an explanation at https://stackoverflow.com/questions/55062897/decrypt-aes256-on-python-vs-php/55063033#55063033. The root problem here is that openssl_decrypt and openssl_encrypt silently truncate the key, discarding half of the entropy without warning.

…mentation was using only half the entropy we should have (128 bits for the key instead of 256 and 8 bits for the IV rather than 16).
@leonstafford
Copy link
Contributor

Oh, brilliant! Security's not my strong point, but will try to grok it!

Aside, have started using https://github.com/phpseclib/phpseclib for upcoming sFTP add-on. Would that offer any advantage here?

@leonstafford
Copy link
Contributor

Thanks again, @john-shaffer!

I cast the results of hex2bin to string to pass type-checking. Please let me know if there's any issues: b5c3202

@john-shaffer
Copy link
Contributor Author

Thank you, @leonstafford. That works perfectly.

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.

None yet

2 participants