Decouple "aesgcm" and "aes128gcm" schemes, disable record chunking. #59

rfk · 2021-03-24T10:04:23Z

This is a significant refactor of the guts of the crypto code, but I think overall it
makes things easier to understand and to audit.

First, I've removed the EceWebPush trait that was previously used to share parts
of the encrypt/decrypt logic between the two schemes. The schemes are not that similar
in practice and on balance, I think the attempt to share code between them was
actually making both schemes harder to understand.

Second, I've cut all the record-chunking code out of "aesgcm". It now supports only
a single record on both encryption and decryption, in line with what the spec says
that a webpush client should support. We were already throwing errors when encountering
multiple records in "aesgcm"; this cleanup takes advantage of that fact to actually
remove the code without breaking the public API.

Finally, I've removed the record-chunking during encryption for "aes128gcm", instead
opting to support larger payloads by increasing the record size. I've also added
several layers of abstraction in the hope of making the code easier to understand -
for example there is a separate Header struct for reading/writing the header,
and a separate PlaintextRecord struct for reading/writing an individual record.

Of course "easier to understand" is subjective, but I think it's an improvement
(and I certainly understand things better as a result of having worked through it!).
Feedback and/or pushback on this is most welcome.

I'd like to try adding record chunking in back here, but as a separate PR building
atop these abstractions.

Connects to #55.

rfk · 2021-03-24T10:12:19Z

@jrconlin I'd love to get your review on this, although I understand it's a lot. I don't have much useful advice for reviewing except that it's probably easier to view the new files in isolation rather than staring at the diff, because much code has moved around or changed indentation levels.

jrconlin

Couple of comments, but looks good!

jrconlin · 2021-03-24T15:49:41Z

src/aes128gcm.rs

+    if params.rs < ECE_AES128GCM_MIN_RS {
+        return Err(Error::InvalidRecordSize);
+    }
+    if plaintext.len() + padding + ECE_TAG_LENGTH > params.rs as usize {


I kinda wonder if it might be nice to include something like dbg!(format!("Message content too long by {:?} bytes.", params.rs - (plaintext.len() + padding + ECE_TAG_LENGTH))); to provide some insight about what went wrong.

In theory we should never hit this branch, because the calling code set rs appropriate; but in practice, future-you or future-me will probably need such a debugging message at some point 😅 - thanks, I'll add one.

src/aes128gcm.rs

src/lib.rs

This is a significant refactor of the guts of the crypto code, but I think overall it makes things easier to understand and to audit. First, I've removed the `EceWebPush` trait that was previously used to share parts of the encrypt/decrypt logic between the two schemes. The schemes are not that similar in practice and on balance, I think the attempt to share code between them was actually making both schemes harder to understand. Second, I've cut all the record-chunking code out of "aesgcm". It now supports only a single record on both encryption and decryption, in line with what the spec says that a webpush client should support. We were already throwing errors when encountering multiple records in "aesgcm"; this cleanup takes advantage of that fact to actually remove the code without breaking the public API. Finally, I've removed the record-chunking during encryption for "aes128gcm", instead opting to support larger payloads by increasing the record size. I've also added several layers of abstraction in the hope of making the code easier to understand - for example there is a separate `Header` struct for reading/writing the header, and a separate `PlaintextRecord` struct for reading/writing an individual record. Of course "easier to understand" is subjective, but I think it's an improvement (and I certainly understand things better as a result of having worked through it!). Feedback and/or pushback on this is most welcome. I'd like to try adding record chunking in back here, but as a separate PR building atop these abstractions. Connects to #55.

jrconlin · 2021-03-24T23:17:16Z

oh, one other thing...
One thing we've adopted in services-engineering is to do a "squash and merge" at commit rather than a series of force pushes. This lets reviewers focus on just the changes for a given commit rather than dig around looking for what might be new.

rfk · 2021-03-24T23:19:01Z

One thing we've adopted in services-engineering is to do a "squash and merge" at commit rather than a series of force pushes.
This lets reviewers focus on just the changes for a given commit rather than dig around looking for what might be new.

I will keep this in mind, thanks!

rfk · 2021-03-25T00:17:14Z

FYI, I've pushed what I have so for for record chunking in #60. AFAICT it works, but I want to add some more tests.

martinthomson

did you consider just deleting aesgcm ?

martinthomson · 2021-03-25T01:04:58Z

src/aes128gcm.rs

+///   +-----------+             content
+///   |   data    |             any length up to rs-17 octets
+///   +-----------+
+///        |
+///        v
+///   +-----------+-----+       add a delimiter octet (0x01 or 0x02)
+///   |   data    | pad |       then 0x00-valued octets to rs-16
+///   +-----------+-----+       (or less on the last record)
+///            |
+///            v
+///   +--------------------+    encrypt with AEAD_AES_128_GCM;
+///   |    ciphertext      |    final size is rs;
+///   +--------------------+    the last record can be smaller


How does this look on rustdoc without ``` ?

Ah, good reminder, thanks! (a high chance it'll look like crap without that)

martinthomson · 2021-03-25T01:06:50Z

src/aes128gcm.rs

+        sequence_number: usize,
+        ciphertext: &[u8],
+        plaintext_buffer: &'a mut [u8],
+    ) -> Result<PlaintextRecord<'a>> {


Suggested change

) -> Result<PlaintextRecord<'a>> {

) -> Result<Self> {

I don't know if you use clippy, but this is what it always tells me to do for these.

Huh, we usually have clippy running in CI to complain about these things, but looks like it's not enabled for this repo.

martinthomson · 2021-03-25T01:08:01Z

src/aes128gcm.rs

+        let iv = generate_iv_for_record(&nonce, sequence_number);
+        let padded_plaintext = cryptographer.aes_gcm_128_decrypt(&key, &iv, &ciphertext)?;
+        // Scan backwards for the first non-zero byte from the end of the data, which delimits the padding.
+        let last_nonzero_byte = match padded_plaintext.iter().rposition(|&b| b != 0u8) {


clippy also prefers if let Some(pos) = here and I've learned to prefer it. But it recently started going with this instead:

let last_nonzero_idx = padded_plaintext.iter().rposition(|&b| b != 0u8).ok_or(Error::DecryptPadding)?

(note that this is an index and not a byte)

martinthomson · 2021-03-25T01:11:40Z

src/aes128gcm.rs

+        plaintext_buffer[0..last_nonzero_byte]
+            .copy_from_slice(&padded_plaintext[0..last_nonzero_byte]);


Do you really need to allow the caller to provide a buffer when you need to copy from a new vector anyway?

Not really no, but it worked out better for re-use of the PlaintextRecord struct between encryption and decryption.

martinthomson · 2021-03-25T01:12:45Z

src/aes128gcm.rs

+        plaintext_buffer[0..last_nonzero_byte]
+            .copy_from_slice(&padded_plaintext[0..last_nonzero_byte]);


Is the goal to always touch every byte, so that you don't leak too much timing info? Because this isn't very good timing defense and this copy call could go after you check whether the record is final or not.

I have not considered that as a goal here, and the ordering of this copy is entirely accidental. Should I by trying to do that?

(I'm interpreting your "this isn't very good timing defense" as meaning "you should not bother with trying to touch every byte, it wont provide meaningful defense" but want to check if that's correct. Naively, I would expect the fact that we've already decrypted and checked the AEAD tag to mean that we can safely do an early-return on invalid data here).

Yes, touching every byte means nothing when the next consumer down the chain won't maintain that discipline.

You can just reorder here to after the padding delimiter check.

martinthomson · 2021-03-25T01:14:16Z

src/aes128gcm.rs

+    let key = cryptographer.hkdf_sha256(
+        salt,
+        &ikm,
+        ECE_AES128GCM_KEY_INFO.as_bytes(),
+        ECE_AES_KEY_LENGTH,
+    )?;
+    let nonce = cryptographer.hkdf_sha256(
+        salt,
+        &ikm,
+        ECE_AES128GCM_NONCE_INFO.as_bytes(),
+        ECE_NONCE_LENGTH,
+    )?;


This won't be optimal; if there is an hkdf-extract function separate from hkdf-expand, you can save a few iterations of the compression function.

Agreed, but this is at least not any worse than it used to be, so we should follow up on that in a separate PR.

(I filed #61 to follow up)

rfk · 2021-03-25T01:22:57Z

did you consider just deleting aesgcm ?

Unfortunately we still see a lot of aesgcm traffic in the wild, ref this graph that JR took from a recent server snapshot.

jrconlin · 2021-03-25T04:28:38Z

Unfortunately, we see a LOT of aesgcm traffic still. I've reached out to various library authors to switch the default to aes128gcm, and most have. Much of the traffic, however, is probably coming from "legacy-built" systems that are fire and forget, and are now forgotten.

I'm not sure how to resolve this without a lot of complexity or user dissatisfaction.

rfk force-pushed the disentangle-the-trait branch from f3371b2 to 796c464 Compare March 24, 2021 10:10

rfk requested a review from jrconlin March 24, 2021 10:11

rfk mentioned this pull request Mar 24, 2021

Pad to multiples of a fixed size, rather than padding randomly #54

Closed

jrconlin approved these changes Mar 24, 2021

View reviewed changes

rfk mentioned this pull request Mar 24, 2021

Pad to multiples of 128 bytes, rather than to a random length. #58

Merged

rfk force-pushed the disentangle-the-trait branch from 796c464 to e58c0b1 Compare March 24, 2021 22:35

rfk changed the base branch from pad-to-fixed-size to main March 24, 2021 22:37

rfk force-pushed the disentangle-the-trait branch from e58c0b1 to c0995bc Compare March 24, 2021 22:38

rfk force-pushed the disentangle-the-trait branch from c0995bc to 0e286a9 Compare March 24, 2021 22:55

martinthomson reviewed Mar 25, 2021

View reviewed changes

Address Martin's feedback

430cca6

Note a potential future optimization

eaf780d

rfk merged commit aaa12b1 into main Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple "aesgcm" and "aes128gcm" schemes, disable record chunking. #59

Decouple "aesgcm" and "aes128gcm" schemes, disable record chunking. #59

rfk commented Mar 24, 2021

rfk commented Mar 24, 2021

jrconlin left a comment

jrconlin Mar 24, 2021

rfk Mar 24, 2021

jrconlin commented Mar 24, 2021

rfk commented Mar 24, 2021

rfk commented Mar 25, 2021

martinthomson left a comment

martinthomson Mar 25, 2021

rfk Mar 25, 2021

martinthomson Mar 25, 2021

rfk Mar 25, 2021

martinthomson Mar 25, 2021

martinthomson Mar 25, 2021

rfk Mar 25, 2021

martinthomson Mar 25, 2021

rfk Mar 25, 2021

martinthomson Mar 25, 2021

martinthomson Mar 25, 2021

rfk Mar 25, 2021

rfk Mar 25, 2021

rfk commented Mar 25, 2021

jrconlin commented Mar 25, 2021

		plaintext_buffer[0..last_nonzero_byte]
		.copy_from_slice(&padded_plaintext[0..last_nonzero_byte]);

Decouple "aesgcm" and "aes128gcm" schemes, disable record chunking. #59

Decouple "aesgcm" and "aes128gcm" schemes, disable record chunking. #59

Conversation

rfk commented Mar 24, 2021

rfk commented Mar 24, 2021

jrconlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrconlin commented Mar 24, 2021

rfk commented Mar 24, 2021

rfk commented Mar 25, 2021

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Mar 25, 2021

jrconlin commented Mar 25, 2021