Repository navigation
Pad to multiples of a fixed size, rather than padding randomly #54
Description
During encryption, the code in this crate currently selects a random integer between zero and the allowed record size, and adds that many bytes of padding to the plaintext.
It's my understanding that adding random padding isn't all that helpful in practice, because if your adversary collects enough samples they can statistically filter out the noise. IMHO we should either do something better here, or do what the Desktop code does and not pad at all because we don't have enough information about the usage context to do it well.
RFC 8188 Section 4.8 has some commentary on padding, and notes that "Developing a padding strategy is difficult". It lists three common strategies, all of which seem tractable and none of which are "add a random amount of padding":
- padding to a small set of fixed lengths
- padding to multiples of a value
- padding to powers of 2
(That section also says "distributing non-padding data across records is recommended to avoid leaking size information", which is something that this crate does do, and which seems worth keeping).
I propose that we change the current padding calculation to round up the plaintext size to multiples of a fixed size; for the sake of argument let's say multiples of 128 bytes. If that doesn't seem worth the effort, then I propose that we remove the padding entirely in the interests of simplicity.
I'll reach out to @martinthomson to see if he'd like to weigh in on the particulars here.
Activity
martinthomson commentedon Mar 22, 2021
Random padding isn't completely useless, depending on context. Here, it's not that much good.
It very much depends on your application when you come to choose a padding scheme. For something like push messaging, you still need to understand what information is being carried and what you want to protect. For instance, if it is price alerts for secret offers on secret stuff, you might like to pad item codes and prices individually so that they are all fixed length, but maybe you also have a variable number of alerts in each message and you want to hide that too.
As a baseline 128 is probably reasonable if you consider a maximum size of 4k. I can see cases for 32 or 64 as well.
Setting a default of 128 and letting people adjust is probably the best that this library can do.
rfk commentedon Mar 22, 2021
I also have this sneaking suspicion that the padding in
aesgcmmode doesn't work correctly. Theunpadmethod has correctly-seeming code to read the padding length as a 2-byte integer and then remove that many zero bytes, but thepadmethod seems to just prefix the plaintext with zero bytes without writing the length (which produces the correct result if you're not adding any padding, will decrypt to a corrupted plaintext otherwise).martinthomson commentedon Mar 22, 2021
Might be time to excise the
aesgcmcode then. Someone helped me do that and it made the code nicer.rfk commentedon Mar 22, 2021
Yeah :-/
I'm taking a brief look at #55 and as part of that, going to try to at least disentangle
aesgcmfromaes128gcmas a start towards one day removing it.Pad to multiples of 128 bytes, rather than to a random length.
Pad to multiples of 128 bytes, rather than to a random length.
rfk commentedon Mar 24, 2021
My attempt to disentangle to two schemes is here: #59
@martinthomson if you have a chance to look at this (probably just at the newly-standalone aes128gcm.rs and aesgcm.rs files) I'd appreciate it, but I understand it's a lot and likely not the highest-priority work. I'm just in a cleaning-up kind of mood.
Pad to multiples of 128 bytes, rather than to a random length.