Closed
Bug 1025463
Opened 10 years ago
Closed 10 years ago
AES-GCM should accept a zero tagLength parameter
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
RESOLVED
INVALID
People
(Reporter: ttaubert, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
rbarnes
:
review-
|
Details | Diff | Splinter Review |
// 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.
Assignee | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
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?
Attachment #8440260 -
Flags: review?(rlb) → review-
Comment 3•10 years ago
|
||
In other words, the comment is wrong (0-128), and the conditional is correct.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Assignee | ||
Comment 4•10 years ago
|
||
You're absolutely right. Sorry for the extra work...
You need to log in
before you can comment on or make changes to this bug.
Description
•