Closed
Bug 869262
Opened 11 years ago
Closed 11 years ago
AppendAVA may read beyond the end of the avaValue->data buffer if useHex is false and truncateValue is true
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.15
People
(Reporter: wtc, Assigned: wtc)
Details
Attachments
(2 files, 1 obsolete file)
977 bytes,
patch
|
ryan.sleevi
:
review+
briansmith
:
superreview+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
Details | Diff | Splinter Review |
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 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•11 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•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: 3.15.1 → 3.15
Updated•11 years ago
|
Attachment #746159 -
Flags: superreview?(bsmith) → superreview+
Updated•11 years ago
|
Attachment #746167 -
Flags: superreview?(bsmith)
Comment 5•11 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.
Description
•