oysteikt/heimdal
0
0
Fork 0
Code Issues 5 Pull Requests Releases Activity

asn1: Fix UB and incorrect codec for unconstrained INTEGER values of -1

Browse Source
This commit is contained in:
Nicolas Williams
2022-10-26 01:53:10 -05:00
parent 476d216f89
commit e4311f3a82
5 changed files with 206 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
lib/asn1/check-der.c
View File

@@ -67,11 +67,12 @@ test_integer (void)
{NULL, 1, "\xff", NULL }, {NULL, 1, "\xff", NULL },
{NULL, 2, "\xff\x01", NULL }, {NULL, 2, "\xff\x01", NULL },
{NULL, 2, "\x00\xff", NULL }, {NULL, 2, "\x00\xff", NULL },
{NULL, 2, "\xfe\x01", NULL },
{NULL, 4, "\x7f\xff\xff\xff", NULL } {NULL, 4, "\x7f\xff\xff\xff", NULL }
}; };
int values[] = {0, 127, 128, 256, -128, -129, -1, -255, 255, int values[] = {0, 127, 128, 256, -128, -129, -1, -255, 255,
0x7fffffff}; -511, 0x7fffffff};
int i, ret; int i, ret;
int ntests = sizeof(tests) / sizeof(*tests); int ntests = sizeof(tests) / sizeof(*tests);
@@ -153,7 +154,7 @@ test_one_int(int val)
static int static int
test_integer_more (void) test_integer_more (void)
{ {
int i, n1, n2, n3, n4, n5, n6; int64_t i, n1, n2, n3, n4, n5, n6;
n2 = 0; n2 = 0;
for (i = 0; i < (sizeof(int) * 8); i++) { for (i = 0; i < (sizeof(int) * 8); i++) {
@@ -522,23 +523,27 @@ static int
test_heim_integer (void) test_heim_integer (void)
{ {
struct test_case tests[] = { struct test_case tests[] = {
{NULL, 1, "\xff", NULL },
{NULL, 2, "\xff\x01", NULL },
{NULL, 2, "\xfe\x01", NULL }, {NULL, 2, "\xfe\x01", NULL },
{NULL, 2, "\xef\x01", NULL }, {NULL, 2, "\xef\x01", NULL },
{NULL, 3, "\xff\x00\xff", NULL }, {NULL, 3, "\xff\x00\xff", NULL },
{NULL, 3, "\xff\x01\x00", NULL }, {NULL, 3, "\xff\x01\x00", NULL },
{NULL, 1, "\x00", NULL }, {NULL, 1, "\x00", NULL },
{NULL, 1, "\x01", NULL }, {NULL, 1, "\x01", NULL },
{NULL, 2, "\x00\x80", NULL } {NULL, 2, "\x00\x80", NULL },
}; };
heim_integer values[] = { heim_integer values[] = {
{ 1, "\x01", 1 },
{ 1, "\xff", 1 },
{ 2, "\x01\xff", 1 }, { 2, "\x01\xff", 1 },
{ 2, "\x10\xff", 1 }, { 2, "\x10\xff", 1 },
{ 2, "\xff\x01", 1 }, { 2, "\xff\x01", 1 },
{ 2, "\xff\x00", 1 }, { 2, "\xff\x00", 1 },
{ 0, "", 0 }, { 0, "", 0 },
{ 1, "\x01", 0 }, { 1, "\x01", 0 },
{ 1, "\x80", 0 } { 1, "\x80", 0 },
}; };
int i, ret; int i, ret;
int ntests = sizeof(tests) / sizeof(tests[0]); int ntests = sizeof(tests) / sizeof(tests[0]);

156
lib/asn1/der_cmp.c
View File

@@ -38,22 +38,25 @@ der_heim_oid_cmp(const heim_oid *p, const heim_oid *q)
{ {
int c; int c;
if (p->length == q->length) if (p->length == q->length) {
if (p->length == 0)
return 0;
return memcmp(p->components, return memcmp(p->components,
q->components, q->components,
p->length * sizeof(*p->components)); p->length * sizeof(*p->components));
}
if (p->length < q->length) { if (p->length < q->length) {
c = memcmp(p->components, if (p->length == 0 ||
q->components, (c = memcmp(p->components,
p->length * sizeof(*p->components)); q->components,
if (c == 0) p->length * sizeof(*p->components))) == 0)
return -1; return -1;
return c; return c;
} }
c = memcmp(p->components, if (q->length == 0 ||
q->components, (c = memcmp(p->components,
q->length * sizeof(*p->components)); q->components,
if (c == 0) q->length * sizeof(*p->components))) == 0)
return 1; return 1;
return c; return c;
} }
@@ -64,14 +67,19 @@ der_heim_octet_string_cmp(const heim_octet_string *p,
{ {
int c; int c;
if (p->length == q->length) if (p->length == q->length) {
if (p->length == 0)
return 0;
return memcmp(p->data, q->data, p->length); return memcmp(p->data, q->data, p->length);
}
if (p->length < q->length) { if (p->length < q->length) {
if ((c = memcmp(p->data, q->data, p->length)) == 0) if (p->length == 0 ||
(c = memcmp(p->data, q->data, p->length)) == 0)
return -1; return -1;
return c; return c;
} }
if ((c = memcmp(p->data, q->data, q->length)) == 0) if (q->length == 0 ||
(c = memcmp(p->data, q->data, q->length)) == 0)
return 1; return 1;
return c; return c;
} }
@@ -94,22 +102,92 @@ int ASN1CALL
der_heim_bit_string_cmp(const heim_bit_string *p, der_heim_bit_string_cmp(const heim_bit_string *p,
const heim_bit_string *q) const heim_bit_string *q)
{ {
int r1, r2; unsigned char pc, qc;
size_t i; size_t bits;
if (p->length != q->length) int c = 0;
return (int)(p->length - q->length);
i = memcmp(p->data, q->data, p->length / 8); /* Compare prefix */
if (i) if (p->length == 0 && q->length == 0)
return (int)i; return 0;
if ((p->length % 8) == 0) if (p->length > 7 && q->length > 7) {
return 0; if (p->length < q->length)
i = (p->length / 8); c = memcmp(p->data, q->data, p->length / 8);
r1 = ((unsigned char *)p->data)[i]; else
r2 = ((unsigned char *)q->data)[i]; c = memcmp(p->data, q->data, q->length / 8);
i = 8 - (p->length % 8); }
r1 = r1 >> i; if (c)
r2 = r2 >> i; return c;
return r1 - r2;
/* Prefixes are equal, c == 0 */
if (p->length == q->length && p->length % 8 == 0)
return 0;
if (p->length == 0 && q->length)
return -1; /* No trailing bits of p to compare to corresponding bits of q */
if (q->length == 0 && p->length)
return 1; /* No trailing bits of q to compare to corresponding bits of p */
/* c == 0, lengths are not equal, both are at least 1 bit */
pc = ((unsigned char *)p->data)[p->length / 8];
qc = ((unsigned char *)q->data)[q->length / 8];
if (p->length < q->length)
bits = p->length % 8;
else
bits = q->length % 8;
if (bits > 0) {
if ((pc & 0x80) == 0 && (qc & 0x80) != 0)
return -1;
if ((pc & 0x80) != 0 && (qc & 0x80) == 0)
return 1;
}
if (bits > 1) {
if ((pc & 0x40) == 0 && (qc & 0x40) != 0)
return -1;
if ((pc & 0x40) != 0 && (qc & 0x40) == 0)
return 1;
}
if (bits > 2) {
if ((pc & 0x20) == 0 && (qc & 0x20) != 0)
return -1;
if ((pc & 0x20) != 0 && (qc & 0x20) == 0)
return 1;
}
if (bits > 3) {
if ((pc & 0x10) == 0 && (qc & 0x10) != 0)
return -1;
if ((pc & 0x10) != 0 && (qc & 0x10) == 0)
return 1;
}
if (bits > 4) {
if ((pc & 0x08) == 0 && (qc & 0x08) != 0)
return -1;
if ((pc & 0x08) != 0 && (qc & 0x08) == 0)
return 1;
}
if (bits > 5) {
if ((pc & 0x04) == 0 && (qc & 0x04) != 0)
return -1;
if ((pc & 0x04) != 0 && (qc & 0x04) == 0)
return 1;
}
if (bits > 6) {
if ((pc & 0x02) == 0 && (qc & 0x02) != 0)
return -1;
if ((pc & 0x02) != 0 && (qc & 0x02) == 0)
return 1;
}
/*
* `bits' can't be 8.
*
* All leading `bits' bits of the tail of the shorter of `p' or `q' are
* equal.
*/
if (p->length < q->length)
return -1;
if (q->length < p->length)
return 1;
return 0;
} }
int ASN1CALL int ASN1CALL
@@ -128,14 +206,19 @@ der_heim_bmp_string_cmp(const heim_bmp_string *p, const heim_bmp_string *q)
{ {
int c; int c;
if (p->length == q->length) if (p->length == q->length) {
if (p->length == 0)
return 0;
return memcmp(p->data, q->data, p->length * sizeof(q->data[0])); return memcmp(p->data, q->data, p->length * sizeof(q->data[0]));
}
if (p->length < q->length) { if (p->length < q->length) {
if ((c = memcmp(p->data, q->data, p->length * sizeof(q->data[0]))) == 0) if (p->length == 0 ||
(c = memcmp(p->data, q->data, p->length * sizeof(q->data[0]))) == 0)
return -1; return -1;
return c; return c;
} }
if ((c = memcmp(p->data, q->data, q->length * sizeof(q->data[0]))) == 0) if (q->length == 0 ||
(c = memcmp(p->data, q->data, q->length * sizeof(q->data[0]))) == 0)
return 1; return 1;
return c; return c;
} }
@@ -146,14 +229,19 @@ der_heim_universal_string_cmp(const heim_universal_string *p,
{ {
int c; int c;
if (p->length == q->length) if (p->length == q->length) {
if (p->length == 0)
return 0;
return memcmp(p->data, q->data, p->length * sizeof(q->data[0])); return memcmp(p->data, q->data, p->length * sizeof(q->data[0]));
}
if (p->length < q->length) { if (p->length < q->length) {
if ((c = memcmp(p->data, q->data, p->length * sizeof(q->data[0]))) == 0) if (p->length == 0 ||
(c = memcmp(p->data, q->data, p->length * sizeof(q->data[0]))) == 0)
return -1; return -1;
return c; return c;
} }
if ((c = memcmp(p->data, q->data, q->length * sizeof(q->data[0]))) == 0) if (q->length == 0 ||
(c = memcmp(p->data, q->data, q->length * sizeof(q->data[0]))) == 0)
return 1; return 1;
return c; return c;
} }

48
lib/asn1/der_get.c
View File

@@ -86,9 +86,12 @@ der_get_integer (const unsigned char *p, size_t len,
int val = 0; int val = 0;
size_t oldlen = len; size_t oldlen = len;
if (len > sizeof(val)) if (len == sizeof(val) + 1 && (p[0] == 0 || p[0] == 0xff))
;
else if (len > sizeof(val))
return ASN1_OVERRUN; return ASN1_OVERRUN;
/* We assume we're on a twos-complement platform */
if (len > 0) { if (len > 0) {
val = (signed char)*p++; val = (signed char)*p++;
while (--len) while (--len)
@@ -109,6 +112,7 @@ der_get_integer64 (const unsigned char *p, size_t len,
if (len > sizeof(val)) if (len > sizeof(val))
return ASN1_OVERRUN; return ASN1_OVERRUN;
/* We assume we're on a twos-complement platform */
if (len > 0) { if (len > 0) {
val = (signed char)*p++; val = (signed char)*p++;
while (--len) while (--len)
@@ -456,13 +460,45 @@ der_get_heim_integer (const unsigned char *p, size_t len,
if (p[0] & 0x80) { if (p[0] & 0x80) {
unsigned char *q; unsigned char *q;
int carry = 1; int carry = 1;
data->negative = 1;
/*
* A negative number. It's going to be a twos complement byte array.
* We're going to leave the positive value in `data->data', but set the
* `data->negative' flag. That means we need to negate the
* twos-complement integer received.
*/
data->negative = 1;
data->length = len; data->length = len;
if (p[0] == 0xff) { if (p[0] == 0xff) {
if (data->length == 1) {
/* One byte of all ones == -1 */
q = data->data = malloc(1);
*q = 1;
data->length = 1;
if (size)
*size = 1;
return 0;
}
p++; p++;
data->length--; data->length--;
/*
* We could check if the next byte's high bit is set, which would
* be an error ("illegal padding" in OpenSSL). However, this would
* mean failing to accept certificates made by certain CAs that
* would read 8 bytes of RNG into a buffer, slap on length 8, then
* slap on the tag [UNIVERSAL INTEGER], and make that the
* serialNumber field's encoding, which then fails to parse in
* around 1 in 256 certificates.
*
* So let's not.
*
* if (p[0] & 0x80)
* return ASN1_PARSE_ERROR; // or a new error code
*/
} }
data->data = malloc(data->length); data->data = malloc(data->length);
if (data->data == NULL) { if (data->data == NULL) {
@@ -471,9 +507,17 @@ der_get_heim_integer (const unsigned char *p, size_t len,
*size = 0; *size = 0;
return ENOMEM; return ENOMEM;
} }
/*
* Note that if `data->length' were zero, this would be UB because we
* underflow if data->length is zero even though we wouldn't actually
* dereference the byte before data->data. Thus we check above for
* that.
*/
q = &((unsigned char*)data->data)[data->length - 1]; q = &((unsigned char*)data->data)[data->length - 1];
p += data->length - 1; p += data->length - 1;
while (q >= (unsigned char*)data->data) { while (q >= (unsigned char*)data->data) {
/* *p XOR 0xff -> ~*p; we're dealing with twos complement */
*q = *p ^ 0xff; *q = *p ^ 0xff;
if (carry) if (carry)
carry = !++*q; carry = !++*q;

4
lib/asn1/der_length.c
View File

@@ -256,7 +256,9 @@ der_length_heim_integer (const heim_integer *k)
{ {
if (k->length == 0) if (k->length == 0)
return 1; return 1;
if (k->negative) if (k->negative && k->length == 1 && ((unsigned char *)k->data)[0] == 1)
return 1;
else if (k->negative)
return k->length + (((~(((unsigned char *)k->data)[0])) & 0x80) ? 0 : 1); return k->length + (((~(((unsigned char *)k->data)[0])) & 0x80) ? 0 : 1);
else else
return k->length + ((((unsigned char *)k->data)[0] & 0x80) ? 1 : 0); return k->length + ((((unsigned char *)k->data)[0] & 0x80) ? 1 : 0);

40
lib/asn1/der_put.c
View File

@@ -343,7 +343,8 @@ der_put_octet_string (unsigned char *p, size_t len,
if (len < data->length) if (len < data->length)
return ASN1_OVERFLOW; return ASN1_OVERFLOW;
p -= data->length; p -= data->length;
memcpy (p+1, data->data, data->length); if (data->length)
memcpy(p+1, data->data, data->length);
*size = data->length; *size = data->length;
return 0; return 0;
} }
@@ -378,19 +379,30 @@ der_put_heim_integer (unsigned char *p, size_t len,
if (data->negative) { if (data->negative) {
ssize_t i; ssize_t i;
int carry; int carry;
for (i = data->length - 1, carry = 1; i >= 0; i--) {
*p = buf[i] ^ 0xff; /*
if (carry) * We represent the parsed integer as a positive value with a
carry = !++*p; * negativity flag. But we need to put it on the wire as the shortest
p--; * twos-complement byte sequence possible. So we're going to negate
} * the number as go.
if (p[1] < 128) { */
if (len < 1) if (data->length == 1 && *(unsigned char *)data->data == 1) {
return ASN1_OVERFLOW; *(p--) = 0xff;
*p-- = 0xff; } else {
len--; for (i = data->length - 1, carry = 1; i >= 0; i--) {
hibitset = 1; *p = buf[i] ^ 0xff;
} if (carry)
carry = !++*p;
p--;
}
if (p[1] < 128) {
if (len < 1)
return ASN1_OVERFLOW;
*p-- = 0xff;
len--;
hibitset = 1;
}
}
} else { } else {
p -= data->length; p -= data->length;
memcpy(p + 1, buf, data->length); memcpy(p + 1, buf, data->length);