aes: Avoid unwanted overflow check when using u32::MAX as the counter.

[briansmith]

The problem occurs:

• release mode with RUSTFLAGS="-C overflow-checks"
• release mode with overflow-checks = true in the Cargo.toml profile.
• debug mode.

Thanks to Mike (GitHub user MikeRomaniuk).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (a40c3a9) to head (1d35370).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2447   +/-   ##
=======================================
  Coverage   96.60%   96.60%           
=======================================
  Files         180      180           
  Lines       21748    21748           
  Branches      538      538           
=======================================
  Hits        21010    21010           
  Misses        623      623           
  Partials      115      115           

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

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[briansmith]

@cpu @ctz @djc PTAL. I intend to do a release with this fix today.

[ctz]

I think this should be a single AES block¹, so it is a surprise that the input is being treated as a counter? Though the change itself looks alright.

Footnotes

1. https://www.rfc-editor.org/rfc/rfc9001.html#section-5.4.3 ↩

[briansmith]

I think this should be a single AES block1, so it is a surprise that the input is being treated as a counter? Though the change itself looks alright.

Sometimes we use this implementation of block encryption via ctr encryption because it is faster and/or the same speed but less code.

fn encrypt_block_using_encrypt_iv_xor_block(key: &impl EncryptBlock, block: Block) -> Block {
    key.encrypt_iv_xor_block(Iv(block), ZERO_BLOCK)
}

[...]

fn encrypt_iv_xor_block_using_ctr32(key: &impl EncryptCtr32, iv: Iv, mut block: Block) -> Block {
    let mut ctr = Counter(iv.0); // This is OK because we're only encrypting one block.
    key.ctr32_encrypt_within(block.as_mut().into(), &mut ctr);
    block
}

[briansmith]

PR #2448 is the PR for the 0.17.12 release. Note that it is a minimal diff from the 0.17.11 release.

[briansmith]

OK, I have now released 0.17.12 with this fix. Please give it a go.

[djc]

@briansmith is there a denial of service issue here that deserves an advisory?

[briansmith]

@djc I know you maintain Quinn. Did you verify that the probably is reachable in Quinn and that this change fixes it? I think we should have more validation of the fix.

Does everybody agree with the math in RELEASES.md? And that this will happen on about 1/(2**32) packets sent or received in a QUIC connection? Anything else that an advisory should say?

[briansmith]

The advisory is at rustsec/advisory-db#2240. Additional tests at #2455 verify the hypothesis that the AES-GCM API is affected in that one edge case. Interestingly, only 64-bit, not 32-bit is affected, because usize::MAX is too small on smaller targets.

[ctz]

And that this will happen on about 1/(2**32) packets sent or received in a QUIC connection? Anything else that an advisory should say?

I don't know for sure, but I was thinking a malicious peer can just arrange sample to end with 0xffff_ffff? AIUI it comes directly from parts of the received packet.

[briansmith]

Yes, I think that's true. I was thinking that even in the best case scenario, it would still happen with non-negligible likelihood.

[djc]

I don't know for sure, but I was thinking a malicious peer can just arrange sample to end with 0xffff_ffff? AIUI it comes directly from parts of the received packet.

Yes, that matches my understanding.

[ahanwate]

@briansmith Do we have any CVE-ID assigned or requested for this issue?

[briansmith]

@briansmith Do we have any CVE-ID assigned or requested for this issue?

I see that Github has GHSA-4p46-pwfr-66x6 and dependabot has started sending out notices to people. My understanding is that every GitHub advisory will get a CVE assigned automatically? But, IDK for sure.

I noticed that GitHub's advisory doesn't show up in the "advisories" at https://github.com/briansmith/ring/security/advisories as of this moment, which I find curious. I would expect that if they're sending out Dependabot alerts then they'd also add their advisory to the project's own advisory list. Anway, we'll see what happens.