This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 25659 - AES-CBC mode does not describe concretely how padding is added
Summary: AES-CBC mode does not describe concretely how padding is added
Status: RESOLVED FIXED
Alias: None
Product: Web Cryptography
Classification: Unclassified
Component: Web Cryptography API Document (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal
Target Milestone: ---
Assignee: Mark Watson
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-12 05:20 UTC by Ryan Sleevi
Modified: 2014-11-30 20:53 UTC (History)
3 users (show)

See Also:


Attachments

Description Ryan Sleevi 2014-05-12 05:20:29 UTC
The definition of AES-CBC mode defines a variable, "padded-plaintext", that is the result of "adding padding octets to ciphertext"

However, ciphertext is an ArrayBuffer/ArrayBufferView, and thus the length of the underlying buffer is fixed.

While possible to specify that an implementation must create a copy of ciphertext that is aligned to blocksize, and THEN add the padding octets, a better definition would provide more prosaic guidance and flexibility to implementors.

For example, if the underlying implementation supported streaming operations, a conforming user agent might only have to create a copy of the final block of "ciphertext". Likewise, if the underlying implementation supports adding the padding itself for unaligned inputs, the specification should permit conforming user agents to not having to modify the data.

That is, rather than normatively specifying that "paddedPlaintext" is equal to the result of the UA adding padding, the inputs should be specified "as if" plaintext was padded according to the padding rules, thus allowing flexibility and avoiding confusion about whether or not it's necessary to invoke the ArrayBuffer constructor during the asynchronous processing.
Comment 1 Mark Watson 2014-05-12 16:12:47 UTC
First, the "behave as if ..." applies to all our algorithm descriptions. Implementors are free to do whatever they want as long as the externally visible behavior is the same as that which would result from following the algorithm description to the letter. I think we say this somewhere explicitly. Anyway, we should be able to take it as read for all algorithm descriptions.

Second, I thought that by the time we get to the algorithm descriptions we are dealing with abstract types. 'plaintext' and 'padded plaintext' are just strings of octets, not a language-specific concrete type that contains such a string. HOw they are stored / manipulated in memory is implementation-specific.

At least, this was my intention when drafting this text. The method descriptions which call into the operation descriptions should say '... passing the contents of X as *plaintext*' where X is the ArrayBufferView and so plaintext is just the octet string.
Comment 2 Ryan Sleevi 2014-05-12 17:05:03 UTC
(In reply to Mark Watson from comment #1)
> First, the "behave as if ..." applies to all our algorithm descriptions.
> Implementors are free to do whatever they want as long as the externally
> visible behavior is the same as that which would result from following the
> algorithm description to the letter. I think we say this somewhere
> explicitly. Anyway, we should be able to take it as read for all algorithm
> descriptions.
> 
> Second, I thought that by the time we get to the algorithm descriptions we
> are dealing with abstract types. 'plaintext' and 'padded plaintext' are just
> strings of octets, not a language-specific concrete type that contains such
> a string. HOw they are stored / manipulated in memory is
> implementation-specific.

This is not actually what the spec says, which is why I filed the bug. They are language-specific, concrete types - and that needs to be resolved.

> 
> At least, this was my intention when drafting this text. The method
> descriptions which call into the operation descriptions should say '...
> passing the contents of X as *plaintext*' where X is the ArrayBufferView and
> so plaintext is just the octet string.

Except it doesn't work like that.

This issue came up when working trough Bug 23728. It's actually quite clear from the spec that, at the time the phrase occurs, "ciphertext" still refers to a concrete ArrayBuffer type.

As such, from the perspective of an external user, "adding padding octets to ciphertext" has a defined meaning - an illegal one. It's also underspecified what it means by "Let X be the result of adding padding octets" - that implies a concrete, mutative step, but the intent is certainly not.

I agree that the 'intent' is understood, but as it's written, it's underspecified, which is why I filed this bug - as I'm having to change the language of these algorithms to make it clear what their external effects are, so that we *could* include such a conformance requirement.

Conformance requirements and the language related to them should be tracked as a separate bug.
Comment 3 Mark Watson 2014-05-12 17:10:31 UTC
Ok, so if we change the text in the method descriptions to be as I suggested, passing abstract types, does this solve the problem ?
Comment 4 Ryan Sleevi 2014-05-12 17:11:29 UTC
(In reply to Mark Watson from comment #3)
> Ok, so if we change the text in the method descriptions to be as I
> suggested, passing abstract types, does this solve the problem ?

No, not really.

I'm already working on resolving this, I merely filed this to track the issue so that the change motivation would be clear.
Comment 5 Mark Watson 2014-09-23 22:57:00 UTC
This appears to be mainly already resolved in the specification with the introduction of the concepts of 'cloning the data' and 'contents of an ArrayBuffer'.

For AES-CBC the following remains to be done: Encrypt: step 2 should refer to plaintext (not ciphertext) and the padding should be added to the 'contents of plaintext' such that there are no ArrayBuffer side-effects as a result of adding the padding (i.e. paddedPlaintext is an octet string, not a slice of an ArrayBuffer)
Comment 7 Ryan Sleevi 2014-10-07 02:51:44 UTC
(In reply to Mark Watson from comment #6)
> https://dvcs.w3.org/hg/webcrypto-api/rev/54a835eaad42

Note that a typo was introduced in this commit

"methods are supported" -> "methods areA supported"
Comment 8 Harry Halpin 2014-11-30 20:53:53 UTC
Typo fixed in CR.