1025318 - AesCbcParams should enforce a 16 byte IV length

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

dictionary AesCbcParams : Algorithm {
  // The initialization vector. MUST be 16 bytes.
  CryptoOperationData iv;
};

The length of 16 bytes for "iv" is currently unchecked.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1025318-AesCbcParams-should-enforce-a-16-byte-IV.patch

Added tests for AES-CTR as well while I was at it.

Comment 2

[Richard Barnes [:rbarnes]]

Comment on attachment 8440139 [details] [diff] [review]
0002-Bug-1025318-AesCbcParams-should-enforce-a-16-byte-IV.patch

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

::: dom/crypto/WebCryptoTask.cpp
@@ +265,5 @@
>          return;
>        }
>  
>        ATTEMPT_BUFFER_INIT(mIv, params.mIv.Value())
> +      if (mIv.Length() != 16) {

It would be marginally more efficient to do this check before assigning to mIv.  That is, to check params.mIv.Value().Length().  That way you avoid the copy if you're not going to do anything with it.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Richard Barnes [:rbarnes] from comment #2)
> >        ATTEMPT_BUFFER_INIT(mIv, params.mIv.Value())
> > +      if (mIv.Length() != 16) {
> 
> It would be marginally more efficient to do this check before assigning to
> mIv.  That is, to check params.mIv.Value().Length().  That way you avoid the
> copy if you're not going to do anything with it.

I'll leave this as is as discussed over IRC because we'd have to call GetAsArrayBufferView() or GetAsArrayBuffer() to access the length.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcea87ef8d6c

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bcea87ef8d6c