1025463 - AES-GCM should accept a zero tagLength parameter

Status: RESOLVED INVALID

(Core :: Security, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

// The desired length of the authentication tag. May be 0 - 128.
[EnforceRange] octet? tagLength;


If the tagLength member of normalizedAlgorithm is not present or is null:
  Let tagLength be 128.

If the tagLength member of normalizedAlgorithm is one of 32, 64, 96, 104, 112, 120 or 128:
  Let tagLength be equal to the tagLength member of normalizedAlgorithm

Otherwise:
  Return an error named DataError.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1025463-AES-GCM-should-accept-a-zero-tagLength-p.patch

Comment 2

[Richard Barnes [:rbarnes]]

Comment on attachment 8440260 [details] [diff] [review]
0003-Bug-1025463-AES-GCM-should-accept-a-zero-tagLength-p.patch

Review of attachment 8440260 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/WebCryptoTask.cpp
@@ +305,5 @@
>        }
>  
>        // 32, 64, 96, 104, 112, 120 or 128
>        mTagLength = 128;
> +      if (params.mTagLength.WasPassed() && params.mTagLength.Value() > 0) {

The conditional you've added here means that if params.mTagLength == 0, then mTagLength will default to 128.  That is not what the spec says.  Here's the conditional in the spec that this branch needs to reflect:

> If the tagLength member of normalizedAlgorithm is not present or is null:
>   Let tagLength be 128.
> If the tagLength member of normalizedAlgorithm is one of 32, 64, 96, 104, 112, 120 or 128:
>   Let tagLength be equal to the tagLength member of normalizedAlgorithm
> Otherwise:
>   Return an error named DataError.

So if tagLength (params.mTagLength) is zero, then it is present and it's not in the specified set, so we should return DataError.

@@ +310,5 @@
>          mTagLength = params.mTagLength.Value();
>          if ((mTagLength > 128) ||
>              !(mTagLength == 32 || mTagLength == 64 ||
>                (mTagLength >= 96 && mTagLength % 8 == 0))) {
> +          mEarlyRv = NS_ERROR_DOM_DATA_ERR;

This is correct, but shouldn't be in this bug.  Could you take a look through to verify that the other error types are aligned with the spec, then file a bug to correct them all?

Comment 3

[Richard Barnes [:rbarnes]]

In other words, the comment is wrong (0-128), and the conditional is correct.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

You're absolutely right. Sorry for the extra work...