AndrewPaglusch / FlashPaper

One-time encrypted password/secret sharing
MIT License
377 stars 60 forks source link

Cleaner URLs #49

Closed AndrewPaglusch closed 2 years ago

AndrewPaglusch commented 2 years ago

This PR makes retreival URLs much more clean by removing special characters like #, -, and _ that might break hyperlinks in some messaging applications.

To accomplish this, we removed the base64_encode_mod() and base64_decode_mod() functions and instead relied on sha256 to generate URL-safe values -- without the need to encode them before adding them to the URL's k parameter. Where needed, the pure PHP implementation of base64_encode() and base64_decode() are used.

AndrewPaglusch commented 2 years ago

Thanks to @mattburchett for all of your help getting this put together!

AndrewPaglusch commented 2 years ago

After reviewing this change a bit more, I think we accidentally introduced a major crypto flaw that we need to discuss and fix before merging.

We're taking the output of SHA256, which is 0-1a-f (hex) and using the literal ASCII characters of that output as the encryption key and IV, which is very low entropy because of the limited character set.

What I think we need to do is replace SHA256 with a cryptographically secure keygen (maybe a KDF). We will use the keygen to create the ID, Key, and IV out of the full 0-9a-zA-Z character set, which would also be conveniently URL-safe. This would provide a lot more entropy than hex characters only.

Matthew-Jenkins commented 2 years ago

If I get what you're saying you are referring this function:

https://github.com/AndrewPaglusch/FlashPaper/blob/97d4b333745eadc7f29c55c7f2ad075e381b62ff/includes/functions.php#L129-L156

It is lower entropy. The hash function has a flag you can set to retrieve raw binary. https://www.php.net/manual/en/function.hash.php which should fix your concern.

https://github.com/AndrewPaglusch/FlashPaper/blob/97d4b333745eadc7f29c55c7f2ad075e381b62ff/includes/functions.php#L5-L21

here I see you're using aes256cbc. You may consider upgrading to chacha20 as it has a much better safety margin than aes256cbc.

AndrewPaglusch commented 2 years ago

@Matthew-Jenkins You are absolutely correct. However, the reason we are not using the raw binary from the hash() function is because we would need to encode the values before placing them in the retreival URL, which would negate the entire purpose of this PR, which is to remove special characters from the URL.

As for upgrading to ChaCha20: Good point. This has been an idea I've had for a while. I've considered using it along with AES (possibly for the encryption with the static key) so that a flaw in a single algorithm/implementation won't weaken the overall security of the final message.

Matthew-Jenkins commented 2 years ago

Then you may consider base64 encoding the binary data.

AndrewPaglusch commented 2 years ago

If you look at the code prior to this PR, that is exactily what we were doing. That caused issues with URLs breaking on people in certain messaging applications because of the special characters (we replaced URL-incompatible characters of the base64 output with #, -, and _) in the generated URLs.

Edit: See here