PKCS7 padding

[alex]

• docs for padder() and unpadder()
• optimize inner loop of update()
• API review
• figure out a name for PKCS7(128), calling it padder is confusing

[alex]

Docs are missing for padder() and unpadder()

[alex]

For reference, work is almost exclusively donalds :)

[alex]

Also the inner loops of update() can be rewritten as some math and a single slice, that should block the merge.

[dstufft]

Do we think these helper functions should be left out similarly to encrypt() and decrypt() ?

[alex]

I think they make sense here, there's not really any other alternative
parameters or whatever you'd have.

On Tue, Oct 29, 2013 at 11:00 AM, Donald Stufft notifications@github.comwrote:

In cryptography/hazmat/primitives/padding.py:

•    super(PKCS7, self).**init**()
  

•    if not (0 <= block_size < 256):
  

•        raise ValueError("block_size must be in range(0, 256)")
  

•    if block_size % 8 != 0:
  

•        raise ValueError("block_size must be a multiple of 8")
  

•    self.block_size = block_size
  
• def padder(self):

•    return _PaddingContext(self.block_size)
  
• def unpadder(self):

•    return _UnpaddingContext(self.block_size)
  
• def pad(self, data):

Do we think these helper functions should be left out similarly to
encrypt() and decrypt() ?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/192/files#r7288249
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

[dreid]

This should probably be documented as returning an object that conforms to some ABC? Maybe even just CipherContext?

[public]

raising early like this leads to a timing side-channel in the padding validation?

[alex]

We looked at the OpenSSL implementation, it also bails immediately.

On Tue, Oct 29, 2013 at 11:58 AM, Alex Stapleton
notifications@github.comwrote:

In cryptography/hazmat/primitives/padding.py:

•    return result
  
• def finalize(self):

•    if self._buffer is None:
  

•        raise ValueError("Context was already finalized")
  

•    if not self._buffer:
  

•        raise ValueError("Invalid padding bytes")
  

•    pad_size = six.indexbytes(self._buffer, -1)
  

•    if pad_size > self.block_size // 8:
  

•        raise ValueError("Invalid padding bytes")
  

•    for b in six.iterbytes(self._buffer[-pad_size:]):
  

•        if b != pad_size:
  

raising early like this leads to a timing side-channel in the padding
validation?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/192/files#r7290507
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

[public]

Which implementation? tls1_cbc_remove_padding (which removes the almost but not quite PKCS5 padding in TLS) was specifically updated with constant time fixes to mitigate Lucky 13. Maybe they didn't copy this over to the PKCS5/7 verification too?

[dstufft]

PKCS7 is easily turned into a padding oracle if the encryption is malleable. TLS' problem, if I recall correctly, is that it doesn't verify the MAC before unpadding the message. Exploiting this, in a properly designed system, would require being able to forge the MAC.

[public]

That exposes the flaw in the padding verification yes.

Assuming that all users will be implementing properly designed systems seems optimistic to me and avoiding known timing channels is in the list of things we are meant to be avoiding :)

The constant time version isn't terrifyingly complicated. I'm happy to port it over unless the consensus is that it's not worth it?

[alex]

Does http://bpaste.net/show/144990/ address your concern?

[public]

Yep.

Sent with AquaMail for Android
http://www.aqua-mail.com

On October 29, 2013 9:34:44 PM Alex Gaynor notifications@github.com wrote:

•    return result
  
• def finalize(self):

•    if self._buffer is None:
  

•        raise ValueError("Context was already finalized")
  

•    if not self._buffer:
  

•        raise ValueError("Invalid padding bytes")
  

•    pad_size = six.indexbytes(self._buffer, -1)
  

•    if pad_size > self.block_size // 8:
  

•        raise ValueError("Invalid padding bytes")
  

•    for b in six.iterbytes(self._buffer[-pad_size:]):
  

•        if b != pad_size:
  

Does http://bpaste.net/show/144990/ address your concern?

---

Reply to this email directly or view it on GitHub:
https://github.com/pyca/cryptography/pull/192/files#r7295565