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

Add authenticated encryption. #127

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Add authenticated encryption. #127

wants to merge 6 commits into from

Conversation

porcuquine
Copy link
Contributor

@porcuquine porcuquine commented Jan 21, 2022

As described in LNCS 7118 - Duplexing the Sponge: Single-Pass Authenticated Encryption and Other Applications and recommended in the Poseidon paper.

Details:

  • message is a sequence of scalar field elements (not bits, bytes, etc.)
  • key and message lengths (both max 64-bits) are encoded in the domain tag
  • no header
  • rate is always the 'arity' (i.e. one less than width)
  • if last chunk of key or message is less than rate, pad with zeros

NOTE: This needs more attention and should not be merged. I don't yet have confidence in its overall correctness yet. I do want eyes on the DST construction, which is plausibly correct and will stay as it is unless a problem is found.

UPDATE: I somehow managed not to find this document (https://drive.google.com/file/d/1EVrP3DzoGbmzkRmYnyEDcIQcXVU7GlOd/edit) initially and instead chased pointers from the main Poseidon paper to https://link.springer.com/content/pdf/10.1007%2F978-3-642-28496-0_19.pdf . I may change this PR (potentially including the DST also) once I've had a chance to look at it in more detail.

It's not all bad: this made me think and understand what's going on better than I probably would have otherwise.

TODO:

  • domain tag tests
  • support collection of intermediate authentication tags
  • circuit

@porcuquine porcuquine force-pushed the encryption branch 6 times, most recently from 5fb3ea2 to 37d9893 Compare January 21, 2022 05:57
@porcuquine porcuquine force-pushed the encryption branch 2 times, most recently from 9aba397 to c2a413a Compare January 21, 2022 08:14
src/hash_type.rs Outdated Show resolved Hide resolved
src/encryption.rs Outdated Show resolved Hide resolved

if *tag != computed_tag {
return Err(Error::TagMismatch);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If plaintext length does not not divide arity we should check that other elements are 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In essence there is implicit padding applied during encryption (by not adding anything when calling duplex with smaller number of inputs), we need to verify that padding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we do need to verify it explicitly. It is implicitly checked by the final authentication tag. If you decrypt with the 'wrong' padding values, then you won't get the correct tag, and decryption will fail.

Do you mean something different? Can you give an example where what I described fails?

Copy link
Contributor

@Kubuxu Kubuxu Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the nonce is part of a key in the implementation, not verifying the padding allows the same plaintext and key-nonce combination to encrypt into different ciphertext+tag. Which can be an issue for some applications.

Other than that I can't think off top of my head more significant negative for not verifying the padding in this case but it is recommended in both source documents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we resolved this in our conversation elsewhere, but please let me know if it still seems problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will probably switch to always using explicit padding treated uniformly, for a different reason though. That will allow for a more uniform circuit — which will make proofs simpler in most cases.

@porcuquine
Copy link
Contributor Author

Thanks for review, @Kubuxu. I decided to add variable-length messages, and will probably also extend to include multiple messages and headers in the same stream. So another commit with those changes (and a fix for the problem you noted) will be coming. Your having reviewed this one first will make following those changes simpler though.

@Kubuxu
Copy link
Contributor

Kubuxu commented Jan 26, 2022

No problem. I will review updates as well.

@porcuquine
Copy link
Contributor Author

@Kubuxu This still needs more work, but you can see generally where I'm going with it if you're interested. If you want to wait, I will also ping you when it's ready.

@porcuquine porcuquine force-pushed the encryption branch 3 times, most recently from 624354c to 1b17425 Compare January 27, 2022 10:17
@Kubuxu
Copy link
Contributor

Kubuxu commented Jan 30, 2022

@porcuquine I think we should add nonce there, I know that nonce can be considered "part of the key" here because it is no different but for ease of use we should add it explicitly.

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