Re: FW: AlgorithmIdentifier in encrypt/decrypt/sign/verify operations

On Sun, Mar 31, 2013 at 5:18 PM, Ryan Sleevi <sleevi@google.com> wrote:

> There has only been four days to review this, and besides Richard, it
> seems that no one has yet.
>

Hmm, if the bar is that more than three people have to comment on something
before we can properly engage in discussion, then this is going to take a
long long time ...


> I do not believe there is any action to take at this time beyond review
> and discussion of solutions.
>
So, could we actually do that ?

> Minimally, it requires a holistic look at *all* algorithms and the impact
> such proposed changes may have. It also may create new opportunities for
> "confused deputy" attacks through misinterpreted parameters, and thus
> requires careful security analysis before eliding otherwise security
> critical details.
>
I'm not proposing to change the inputs to any of the algorithms. I'm not
convinced what we have today has been though any such 'careful security
analysis', so why you feel such a bar should be raised for simple changes
that make the specification less confusing, I am not sure. The whole spec
needs 'careful security analysis'.

I put together a table of the things that currently go in
AlgorithmIdentifier.params for the various algorithms and methods:

https://docs.google.com/document/d/1fKtA6U0b_R-kml3qgvdnq-diu3oyDsFbEGKG2AB-tq0/edit?usp=sharing

You can see that the parameters to the Key-based operations (encrypt,
decrypt, sign, verify) are disjoint with those for the Key-creating
operations (generate, derive, import, export) with one exception (HMAC).

For HMAC, the hash must be specified both at key creation and at key usage
time, the latter being strictly redundant and serving only to create
another unnecessary failure path.

So, my proposal is that for encrypt, decrypt, sign and verify, we replace
the AlgorithmIdentifier in the method signature with OperationParameters
and derive the existing dictionaries for the operation parameters from an
(empty) OperationParameters dictionary, in place of AlgorithmParameters.

There is strictly no change in the input information to these four methods
- the algorithm name is specified once in the Key object - but I think this
would make things much clearer.

...Mark


> Given that, I think it premature to make any edits until further review
> and discussion - such as on the call following next.
> On Mar 31, 2013 2:39 PM, "GALINDO Virginie" <Virginie.GALINDO@gemalto.com>
> wrote:
>
>> Ryan,****
>>
>> Any action to take on the editors side to address Mark and Richard
>> concerns and suggestions ?****
>>
>> Regards,****
>>
>> Virginie****
>>
>> ** **
>>
>> *From:* Mark Watson [mailto:watsonm@netflix.com]
>> *Sent:* samedi 30 mars 2013 17:52
>> *To:* Richard Barnes
>> *Cc:* public-webcrypto@w3.org
>> *Subject:* Re: AlgorithmIdentifier in encrypt/decrypt/sign/verify
>> operations****
>>
>> ** **
>>
>> ** **
>>
>> On Fri, Mar 29, 2013 at 11:22 AM, Richard Barnes <rbarnes@bbn.com> wrote:
>> ****
>>
>>
>> On Mar 27, 2013, at 9:10 PM, Mark Watson <watsonm@netflix.com> wrote:
>>
>> > This may be related to ISSUE-12 and apologies again if this has been
>> discussed before - it is coming up now frequently in implementation
>> discussions.
>> >
>> > In the encrypt/decrypt/sign/verify operations, two AlgorithmIdentifier
>> objects are provided as input, one as an explicit parameter and one which
>> is associated with the Key object (and appears as the Key.algorithm
>> attribute). Presumably it is an error if the "name" member of the
>> dictionary does not match (after normalization), though I am not sure if
>> this is clearly specified.
>> >
>> > In some cases, it is specified that the params member will have
>> different types in these two places (I'm assuming that the Key.algorithm
>> attribute takes the value that was provided to generateKey). For example
>> for AES-CTR, the params in Key.algorithm contains the key length and params
>> in encrypt/decrypt contains the IV.
>> >
>> > But for other cases things are very unclear. For example, for HMAC, the
>> same AlgorithmParameters type is used, containing the hash algorithm. In
>> this case it seems completely redundant to provide the same object twice to
>> the sign/verify call (once in the method parameters and again in the
>> Key.algorithm attribute).
>> >
>> > Am I missing something ? Does anyone else find this confusing ?
>> >
>> > I think the confusion could be resolved by
>> > (i) replacing the AlgorithmIdentifier argument to
>> sign/verify/encrypt/decrypt with AlgorithmParameters.
>> > (ii) for HMAC, the params provided to sign/verify must be null, as the
>> hash algorithm should have been provided when the Key was
>> created/imported/unwrapped****
>>
>> I agree that it could be made clearer.
>>
>> When I was implementing PolyCrypt, my read of the specification was as
>> follows:
>> 1. The algorithm provided as a parameter to encrypt() specifies the
>> encryption (and parameters)
>> 2. Throw an error if the Key.algorithm.name != algorithm.name
>>
>> That is, for the algorithm in the Key, everything besides the name is
>> ignored.  This seems right to first order, but might be wrong, e.g., for
>> HMAC, where you might want to compare the hash algorithm as well.****
>>
>> ** **
>>
>> It seems ambiguous to me whether the hash algorithm is a property of the
>> key or a parameter to the operation. Another source of confusion.****
>>
>>  ****
>>
>>
>> I'm leery of removing the algorithm parameter from encrypt(), if only
>> because it seems really confusing and non-idiomatic.  ****
>>
>> ** **
>>
>> What idiom, and why do you say it's confusing ? The algorithm is a
>> property of the key, so it's confusing that I need to re-specify it as a
>> method parameter. That only seems to introduce an unnecessary failure path
>> and give the incorrect impression to developer that they have some choice
>> about the algorithm here. They don't. It's implicit in the Key.****
>>
>>  ****
>>
>> I don't think it's terrible to have the algorithm specified in two
>> places, as long as its clear how those two specifications relate to each
>> other****
>>
>> ** **
>>
>> It's not clear now. IIUC, anything in the method AlgorithmIdentifier that
>> is also a property of the Key must match. Anything else is a method
>> parameter.****
>>
>> ** **
>>
>> A particularly confusing case is when there are both algorithm and method
>> parameters. For example, suppose a create an AES-CBC key with { name :
>> "AES-CBC", params: { length: 128 } }. Am I supposed to write****
>>
>> ** **
>>
>> encrypt( { name: "AES-CBC", { "length" : 128, "iv" : iv } }, ... )****
>>
>> ** **
>>
>> ?****
>>
>> ** **
>>
>> If I'm allowed to miss out the length here, why is it that it makes sense
>> to miss out some of the Key properties and not others (the algorithm name
>> itself) ?****
>>
>> ** **
>>
>> This could be completely cleaned up by only specifying the "params"
>> member in the methods. Specifically by defining an OperationParameters for
>> that thing and derving the operation parameters objects from that. This
>> would provide a clear separation between algorithm and operation parameters.
>> ****
>>
>> ** **
>>
>> ...Mark****
>>
>>  ****
>>
>>
>>
>> > As a side note, I believe that to generate a HMAC key we need to
>> specify the key length. At least according to FIPS 198-1 the key, K, can be
>> of any length. So, either we require in WebCrypto that it is a particular
>> length (say, the same size as the hash function), or we need a length
>> parameter to generateKey for HMAC.****
>>
>> +1 on adding a length parameter.
>>
>> --Richard
>>
>> >
>> > ...Mark****
>>
>> ** **
>>
>