853674 - The output buffer size checks in AES_Encrypt and AES_Decrypt are too lax or too strict for AES GCM

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

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.

Comment 1

[Wan-Teh Chang]

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.

Comment 2

[Robert Relyea]

Ah, I was expecting AES_Encrypt and AES_Decrypt to call the worker thread to get the length, I think.

Comment 3

[Kai Engert [:KaiE:]]

changing target milestone to 3.15.4

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 5

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.