Open
Bug 853674
Opened 12 years ago
Updated 2 years ago
The output buffer size checks in AES_Encrypt and AES_Decrypt are too lax or too strict for AES GCM
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
NEW
3.15.4
People
(Reporter: wtc, Unassigned)
Details
Both AES_Encrypt and AES_Decrypt check the output buffer size as follows: if (maxOutputLen < inputLen) { PORT_SetError(SEC_ERROR_OUTPUT_LEN); return SECFailure; } http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/rijndael.c&rev=1.30&mark=1218,1233-1236,1249,1264-1267#1211 For AES GCM, this check is too lax in AES_Encrypt and too strict in AES_Decrypt because the check fails to take into account the length of the authentication tag. I tried to fix this by adding a |mode| member to AESContext to store the mode of operation, but realized that I would also need to store the ulTagBits value from CK_GCM_PARAMS. So it seems better to have cx->worker() check the output buffer size. This is more tedious because all cx->worker() need to be modified, but cx->worker() has all the info it needs in cx->worker_cx.
Reporter | ||
Comment 1•12 years ago
|
||
Bob: I verified AES_Encrypt won't overflow the output buffer when using GCM. Details follow. I called AES_Encrypt with an output buffer that has no room for the GCM authentication tag and single-stepped in the debugger. Although AES_Encrypt itself allows that output buffer size: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/rijndael.c&rev=1.30&mark=1218,1233-1236#1211 when AES_Encrypt passes the output buffer to cx->worker(), GCM_EncryptUpdate rejects that output buffer size: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/gcm.c&rev=1.2&mark=752,766-770#746 So we will not overflow the output buffer. Also, the output buffer size check of AES_Encrypt is correct: it first does a more lax check that is fine for all modes of operation, and then each cx->worker() may perform a more strict check if necessary. This design may have been unintentional but it works out just fine. Unfortunately AES_Decrypt still has a problem because the output buffer size check in AES_Decrypt is too strict and cx->worker() doesn't get a chance to relax that check. But the caller can easily work around this bug by allocating a larger output buffer.
Priority: -- → P2
Comment 2•12 years ago
|
||
Ah, I was expecting AES_Encrypt and AES_Decrypt to call the worker thread to get the length, I think.
Reporter | ||
Updated•12 years ago
|
Target Milestone: 3.15 → 3.15.1
Reporter | ||
Updated•11 years ago
|
Target Milestone: 3.15.1 → 3.15.2
Reporter | ||
Updated•11 years ago
|
Target Milestone: 3.15.2 → 3.15.3
Comment 4•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: wtc → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(bbeurdouche)
Updated•2 years ago
|
Severity: normal → S3
Comment 5•2 years ago
|
||
We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.
Flags: needinfo?(bbeurdouche)
You need to log in
before you can comment on or make changes to this bug.
Description
•