superreview requested: [Bug 869262] AppendAVA may read beyond the end of the avaValue->data buffer if useHex is false and truncateValue is true : [Attachment 746158] Patch 1: pass the minimum of avaValue->len and valueLen

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has asked Brian Smith (:bsmith)
<bsm...@mozilla.com> for superreview:
Bug 869262: AppendAVA may read beyond the end of the avaValue->data buffer if
useHex is false and truncateValue is true
https://bugzilla.mozilla.org/show_bug.cgi?id=869262

Attachment 746158: Patch 1: pass the minimum of avaValue->len and valueLen
https://bugzilla.mozilla.org/attachment.cgi?id=746158&action=edit


------- Additional Comments from Wan-Teh Chang <w...@google.com>
(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.

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has asked Brian Smith (:bsmith)
<bsm...@mozilla.com> for superreview:
Bug 869262: AppendAVA may read beyond the end of the avaValue->data buffer if
useHex is false and truncateValue is true
https://bugzilla.mozilla.org/show_bug.cgi?id=869262

Attachment 746159: (Correct patch) Patch 1: pass the minimum of avaValue->len
and valueLen
https://bugzilla.mozilla.org/attachment.cgi?id=746159&action=edit

------- Additional Comments from Wan-Teh Chang <w...@google.com>

I attached the wrong patch. This is the correct one.

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has canceled Wan-Teh Chang <w...@google.com>'s
request for superreview:

Bug 869262: AppendAVA may read beyond the end of the avaValue->data buffer if
useHex is false and truncateValue is true
https://bugzilla.mozilla.org/show_bug.cgi?id=869262

Attachment 746158: Patch 1: pass the minimum of avaValue->len and valueLen
https://bugzilla.mozilla.org/attachment.cgi?id=746158&action=edit

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has asked Brian Smith (:bsmith)

<bsm...@mozilla.com> for superreview:

Bug 869262: AppendAVA may read beyond the end of the avaValue->data buffer if
useHex is false and truncateValue is true
https://bugzilla.mozilla.org/show_bug.cgi?id=869262

Attachment 746167: Patch 2: escape/quote the full avaValue->data buffer, then
truncate
https://bugzilla.mozilla.org/attachment.cgi?id=746167&action=edit

------- Additional Comments from Wan-Teh Chang <w...@google.com>

The second way to fix this bug requires an extra heap allocation.
But it is easier to see it's correct.

[bugzill...@mozilla.org]

Brian Smith (:bsmith) <bsm...@mozilla.com> has granted Wan-Teh Chang
<w...@google.com>'s request for superreview:

Bug 869262: AppendAVA may read beyond the end of the avaValue->data buffer if
useHex is false and truncateValue is true
https://bugzilla.mozilla.org/show_bug.cgi?id=869262

Attachment 746159: (Correct patch) Patch 1: pass the minimum of avaValue->len
and valueLen
https://bugzilla.mozilla.org/attachment.cgi?id=746159&action=edit

[bugzill...@mozilla.org]

Brian Smith (:bsmith) <bsm...@mozilla.com> has canceled Wan-Teh Chang

<w...@google.com>'s request for superreview:
Bug 869262: AppendAVA may read beyond the end of the avaValue->data buffer if
useHex is false and truncateValue is true
https://bugzilla.mozilla.org/show_bug.cgi?id=869262