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

Security Patch: Fixing Loopholes for Timing Attacks #3

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

marcoonroad
Copy link
Owner

This PR is open to track changes and further discussion. This patch introduces the eqaf library dependence and some refactorings. I need to make both execution flows in case of success and failure to match in their own approximate execution times. In this sense:

  • I compare strings/bytes in constant time (see eqaf);
  • I decrypt the ciphertext even if the MAC tags don't match (obviously the result plaintext is ignored and computation fails);
  • Many recursive calls which would trigger a Garbage Collection (either minor-to-major heap promotion or major heap compaction) are avoided by using imperative loops provided by OCaml syntax.

More discussion is still needed here.

@marcoonroad
Copy link
Owner Author

marcoonroad commented Dec 16, 2020

The eqaf dependence for constant-time string comparison is not really needed. I can compare two strings in constant-time by hashing them with a random HMAC-key and then comparing their images, e.g:

let hmac_compare (a : string) (b : string) : bool =
  let random = random_bytes 32 in
  hmac_sha256 a ~key:random = hmac_sha256 b ~key:random

@marcoonroad
Copy link
Owner Author

Dunno if mixing eqaf with hmac-string-comparison would improve security and timings on benchs, but nevertheless I will try both here.

… base64 encoding on input message anymore

Signed-off-by: Marco Aurélio da Silva <[email protected]>
…ovided by now 'cause it will need a csprng from mirage-crypto dependency)

Signed-off-by: Marco Aurélio da Silva <[email protected]>
@marcoonroad
Copy link
Owner Author

I should as well test the timings by using AES-CTR mode instead AES-CBC. 'Cause I generate a random key, there's no such key+IV reuse problem for stream ciphers (AES-CTR in the case). AES-CTR is faster than AES-CBC, but harder to make it right due counter/IV management (it requires a stateful tracking of that). Nevertheless, AES-CTR makes padding useless (more timings saved), but both encrypt and decrypt operations are the same due xor-nature of AES-CTR's generated key-stream.

Another really important question is how secure is AES-CTR for long-lived data 'cause it's just a xor-ed key-stream, could it resist all major cipher attacks (assuming AEAD-support with Encrypt-then-MAC in the same sense of configured AES-CBC)?

Would it be a commiting encryption / lockable box as well?

Discussion open for future tracking.

@marcoonroad
Copy link
Owner Author

Things seem to fail no matter how I try to find a good eqaf dependency version. That is, GH Actions would still break for some OCaml compiler versions. Anyways, I'll ignore that by now and give focus on make this portable on Windows too by replacing the internal CSPRNG from nocrypto by the mirage-crypto provided one.

…ad mirage-crypto - blake2b mac replaced by sha256 for broader compatibility

Signed-off-by: Marco Aurélio da Silva <[email protected]>
@marcoonroad marcoonroad added this to the 2.0.0 release milestone Dec 22, 2020
@marcoonroad marcoonroad self-assigned this Dec 22, 2020
@marcoonroad marcoonroad added the enhancement New feature or request label Dec 22, 2020
@marcoonroad
Copy link
Owner Author

AES-CTR encryption is discarded 'cause we will provide support for the encryption of opening key too (see #5), possibly reusing an end-user password from $NOCOINER_PASSWORD environment variable - still needs to check Windows support and test this CLI program on all platform workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant