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

Fix padding calculation for large AADs in xchacha20poly1305 secretstream push/pull #73

Merged
merged 7 commits into from
Dec 7, 2024

Conversation

httpjamesm
Copy link
Contributor

@httpjamesm httpjamesm commented Dec 5, 2024

  • Uses the correct modulo calculation when updating the mac:

mac.update(&_pad0[..((0x10 - (associated_data.len() % 0x10)) & 0xf)]);

Previous code did not include the modulus, thus failing on AADs larger than 16 bytes. The XChaCha20 specification allows much more than that.

The original libsodium implementation in C does a similar operation:

    crypto_onetimeauth_poly1305_update(&poly1305_state, _pad0,
                                       (0x10 - adlen) & 0xf);
  • Adds a unit test to ensure large AAD works
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.32s
     Running unittests src/lib.rs (/home/james/Develop/dryoc/target/debug/deps/dryoc-fc575e74639062ec)

running 1 test
test classic::crypto_secretstream_xchacha20poly1305::tests::test_secretstream_large_aad ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 87 filtered out; finished in 0.00s

Without the fix, it fails:

test classic::crypto_secretstream_xchacha20poly1305::tests::test_secretstream_large_aad ... FAILED

failures:

---- classic::crypto_secretstream_xchacha20poly1305::tests::test_secretstream_large_aad stdout ----
thread 'classic::crypto_secretstream_xchacha20poly1305::tests::test_secretstream_large_aad' panicked at src/classic/crypto_secretstream_xchacha20poly1305.rs:394:26:
attempt to subtract with overflow
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    classic::crypto_secretstream_xchacha20poly1305::tests::test_secretstream_large_aad

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 87 filtered out; finished in 0.00s

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for dryoc-docs ready!

Name Link
🔨 Latest commit fafdab3
🔍 Latest deploy log https://app.netlify.com/sites/dryoc-docs/deploys/6754503686b6430008e02e31
😎 Deploy Preview https://deploy-preview-73--dryoc-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

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

A couple minor suggestions, but otherwise it looks good.

@brndnmtthws
Copy link
Owner

For anyone reading, here's more detail on the bug: this would only fail in debug mode, but not release mode. In release mode, it will silently succeed. Here's a simple demonstration of the problem: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=af06abf7079edb5b16fc860693102d71

While libsodium's implementation is technically wrong, it works fine because C isn't fussy about the overflow, but Rust is strict.

For reference, the padding is described in RFC 7539 here: https://datatracker.ietf.org/doc/html/rfc7539#section-2.8.1

In terms of security risk, this is pretty innocuous but still worth fixing.

@brndnmtthws
Copy link
Owner

It's not related to this PR, but I should also implement the missing AEAD functions (crypto_aead_*) for ChaCha20-Poly1305 and XChaCha20-Poly1305 (from https://doc.libsodium.org/secret-key_cryptography/aead/chacha20-poly1305).

@brndnmtthws
Copy link
Owner

I did some housekeeping, you'll need to rebase against main.

@httpjamesm
Copy link
Contributor Author

Changes addressed 👍

Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.60%. Comparing base (8fa96ff) to head (fafdab3).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   65.55%   65.60%   +0.05%     
==========================================
  Files          43       43              
  Lines        3170     3172       +2     
==========================================
+ Hits         2078     2081       +3     
+ Misses       1092     1091       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

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

Very nice, thank you kindly!

@brndnmtthws brndnmtthws merged commit e0e0ef0 into brndnmtthws:main Dec 7, 2024
27 checks passed
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