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

Adding AEAD support as new encryption method #736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silverdaz
Copy link

  • Adding a new encryption method (with AEAD)
  • Updating the graphics with a new packet type
  • Separating the packet type because extending the session key packets with 4 more bytes makes it incompatible when using multiple session keys

@silverdaz
Copy link
Author

silverdaz commented Jul 22, 2023

PR for @daviesrob about Crypt4GH.

Note: I do think the Crypt4GH specs should be moved away from this repository, and have its own.

You can have a look at the python implementation for AEAD support. It's a branch and I have not merged it yet to master

@github-actions
Copy link

Changed PDFs as of 304afb6: crypt4gh (diff).

@silverdaz silverdaz marked this pull request as draft July 25, 2023 14:42
@silverdaz
Copy link
Author

  • Reverted the graphics to remove the sequence number packet type.
  • Each session key is now paired with a sequence number, when using the AEAD mode.
  • Encryption methods can not be mixed

@silverdaz silverdaz marked this pull request as ready for review July 26, 2023 20:32
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Changed PDFs as of 6dd276a: crypt4gh (diff).

@daviesrob
Copy link
Member

A few comments following a read-through:

This part of the "Security Considerations" could do with updating, as the new encryption method prevents block reordering. I don't think it fixes the last part where you add a new header packet though, although you'd need access to the recipient public key from elsewhere to successfully do that. It would be possible to close that last loophole by insisting that only one writer's public key is used, as the attacker will not have the corresponding private key.

The name chacha20_ietf_poly1305_with_AEAD is a bit redundant as chacha20-ietf-poly1305 is already a form of authenticated encryption (AE). chacha20_ietf_poly1305_with_AD or chacha20_ietf_poly1305_using_AD would better describe the difference with the original method.

In the header packet, sequence_sv (for sequence starting value) might be a better name than sequence_number. It would make it clearer that the value is to be combined with something else.

I think the line about the new method using AAD should be removed from the chacha20\_ietf\_poly1305 Encryption section. It's not really relevant to the original method, so may lead to confusion. It's also referencing something that's described in the next section anyway, so seems a bit redundant.

I have a few ideas for the sections about encryption and decryption, but need to work on them a bit more. I'll add some suggestions later.

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

Successfully merging this pull request may close these issues.

2 participants