AppendAVA may read beyond the end of the avaValue->data buffer if useHex is false and truncateValue is true

RESOLVED FIXED in 3.15

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 746158 [details] [diff] [review]
Patch 1: pass the minimum of avaValue->len and valueLen

(The code in this bug was added in bug 487487 by Nelson.)

AddressSanitizer reported that the AppendAVA function in nss/nss/lib/certdb/alg1485.c
may read beyond the end of the avaValue->data buffer if useHex is false and
truncateValue is true.  Unfortunately I don't have an example of the certificate
that causes this problem, but my code inspection shows this is plausible.

ASan said AppendAVA allocated the avaValue->data buffer on line 936:

 935     if (!useHex) {
 936         avaValue = CERT_DecodeAVAValue(&ava->value);
 937         if (!avaValue) {
 938             useHex = PR_TRUE;
 939             if (strict != CERT_N2A_READABLE) {
 940                 tagName = NULL;  /* must use OID.N form */
 941             }
 942         }
 943     }

This implies useHex is false.

ASan reported a heap-buffer-overflow inside the escapeAndQuote call on line 1040:

1022     /* escape and quote as necessary - don't quote hex strings */
1023     if (useHex) {
1024         char * end = encodedAVA + nameLen + valueLen;
1025         memcpy(encodedAVA + nameLen, (char *)avaValue->data, valueLen);
1026         end[0] = '\0';
1027         if (truncateValue) {
1028             end[-1] = '.';
1029             end[-2] = '.';
1030             end[-3] = '.';
1031         }
1032         rv = SECSuccess;
1033     } else if (!truncateValue) {
1034         rv = escapeAndQuote(encodedAVA + nameLen, len - nameLen,
1035                             (char *)avaValue->data, avaValue->len, &mode);
1036     } else {
1037         /* must truncate the escaped and quoted value */
1038         char bigTmpBuf[TMPBUF_LEN * 3 + 3];
1039         rv = escapeAndQuote(bigTmpBuf, sizeof bigTmpBuf,
1040                             (char *)avaValue->data, valueLen, &mode);
1041 
1042         bigTmpBuf[valueLen--] = '\0'; /* hard stop here */

This implies useHex is false (consistent with the above) and truncateValue
is true.

The heap-buffer-overflow occurred inside escapeAndQuote on line 641:

 640     /* space for terminal null */
 641     reqLen = cert_RFC1485_GetRequiredLen(src, srclen, &mode) + 1;
 642     if (reqLen > dstlen) {
 643         PORT_SetError(SEC_ERROR_OUTPUT_LEN);
 644         return SECFailure;
 645     }

and ultimately inside cert_RFC1485_GetRequiredLen on line 602:

 600     /* need to make an initial pass to determine if quoting is needed */
 601     for (i = 0; i < srclen; i++) {
 602         char c = src[i];

This means the value of the 'srclen' argument (valueLen) that we pass
to the escapeAndQuote call on line 1040 is bigger than the size of the
avaValue->data buffer (avaValue->len).

Patch 1 is the first way to fix the bug. Like the original code, I think it
has the problem that escapeAndQuote may escape/quote a truncated avaValue->data
buffer in a different way than a full avaValue->data buffer.
Attachment #746158 - Flags: superreview?(bsmith)
Attachment #746158 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 1

5 years ago
Created attachment 746159 [details] [diff] [review]
(Correct patch) Patch 1: pass the minimum of avaValue->len and valueLen

I attached the wrong patch. This is the correct one.
Attachment #746158 - Attachment is obsolete: true
Attachment #746158 - Flags: superreview?(bsmith)
Attachment #746158 - Flags: review?(ryan.sleevi)
Attachment #746159 - Flags: superreview?(bsmith)
Attachment #746159 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 2

5 years ago
Created attachment 746167 [details] [diff] [review]
Patch 2: escape/quote the full avaValue->data buffer, then truncate

The second way to fix this bug requires an extra heap allocation.
But it is easier to see it's correct.
Attachment #746167 - Flags: superreview?(bsmith)
Attachment #746167 - Flags: review?(ryan.sleevi)

Comment 3

5 years ago
Comment on attachment 746159 [details] [diff] [review]
(Correct patch) Patch 1: pass the minimum of avaValue->len and valueLen

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

r+ rsleevi
Attachment #746159 - Flags: review?(ryan.sleevi) → review+
(Assignee)

Comment 4

5 years ago
Comment on attachment 746159 [details] [diff] [review]
(Correct patch) Patch 1: pass the minimum of avaValue->len and valueLen

https://hg.mozilla.org/projects/nss/rev/edfcd816acaf
Attachment #746159 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: 3.15.1 → 3.15
Attachment #746159 - Flags: superreview?(bsmith) → superreview+
Attachment #746167 - Flags: superreview?(bsmith)

Comment 5

5 years ago
Comment on attachment 746167 [details] [diff] [review]
Patch 2: escape/quote the full avaValue->data buffer, then truncate

Removing the r? for me, because Patch 1 was the checked in patch.

It's not clear to me if you're still proposing we do the heap allocation here as the solution "going forward". If so, please feel free to set the r?
Attachment #746167 - Flags: review?(ryan.sleevi)
You need to log in before you can comment on or make changes to this bug.