fix CCM, as discovered when porting TLS to string #242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The issue were missing offsets, both if multiple blocks were encrypted/decrypted and when unaligned data (i.e. not a multiple of block_size) was encrypted and decrypted.
reynir
reviewed
Aug 20, 2024
| Bytes.unsafe_blit buf 0 dst dst_off len ; | ||
| unsafe_xor_into src ~src_off dst ~dst_off len ; | ||
| Bytes.unsafe_blit_string cbcblock cbc_off buf 0 len ; | ||
| Bytes.unsafe_fill buf len (block_size - len) '\x00'; | ||
| cbc (Bytes.unsafe_to_string buf) cbc_off iv 0 | ||
| cbc (Bytes.unsafe_to_string buf) 0 iv 0 |
There was a problem hiding this comment.
Hmm is it safe to switch cbc_off with 0?
There was a problem hiding this comment.
Ah, it is. buf is the counter stuff and unrelated to src/dst.
There was a problem hiding this comment.
No, it's not the counter stuff, I am confusing myself. But this looks good.
There was a problem hiding this comment.
buf is the locally created 16 byte buffer that holds the last block (which is then partially blit into dst). the cbc is for the tag, and here the full buf is used -- where previously the cbcblock (depending on encrypt or decrypt, the src or dst) has been blit into, followed by clearing the remaining bytes with 0.
reynir
approved these changes
Aug 20, 2024
|
CI failures are:
|
hannesm
added a commit
to hannesm/opam-repository
that referenced
this pull request
Aug 20, 2024
CHANGES: * mirage-crypto: fix CCM implementation, discovered while porting TLS to mirage-crypto 1.0.0 (@hannesm @dinosaure @reynir mirage/mirage-crypto#242)
hannesm
added a commit
to hannesm/opam-repository
that referenced
this pull request
Aug 20, 2024
CHANGES: * FEATURE mirage-crypto-ec: provide Dh.secret_to_octets (requested in mirage/mirage-crypto#243 by @palainp, implemented by @hannesm mirage/mirage-crypto#244) * mirage-crypto: fix CCM implementation, discovered while porting TLS to mirage-crypto 1.0.0 (@hannesm @dinosaure @reynir mirage/mirage-crypto#242)
avsm
pushed a commit
to avsm/opam-repository
that referenced
this pull request
Sep 5, 2024
CHANGES: * FEATURE mirage-crypto-ec: provide Dh.secret_to_octets (requested in mirage/mirage-crypto#243 by @palainp, implemented by @hannesm mirage/mirage-crypto#244) * mirage-crypto: fix CCM implementation, discovered while porting TLS to mirage-crypto 1.0.0 (@hannesm @dinosaure @reynir mirage/mirage-crypto#242)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue were missing offsets, both if multiple blocks were encrypted/decrypted and when unaligned data (i.e. not a multiple of block_size) was encrypted and decrypted.
//cc @reynir @dinosaure