diff --git a/lib/asn1/check-der.c b/lib/asn1/check-der.c index a8956a74b..58c1446fd 100644 --- a/lib/asn1/check-der.c +++ b/lib/asn1/check-der.c @@ -67,11 +67,12 @@ test_integer (void) {NULL, 1, "\xff", NULL }, {NULL, 2, "\xff\x01", NULL }, {NULL, 2, "\x00\xff", NULL }, + {NULL, 2, "\xfe\x01", NULL }, {NULL, 4, "\x7f\xff\xff\xff", NULL } }; int values[] = {0, 127, 128, 256, -128, -129, -1, -255, 255, - 0x7fffffff}; + -511, 0x7fffffff}; int i, ret; int ntests = sizeof(tests) / sizeof(*tests); @@ -153,7 +154,7 @@ test_one_int(int val) static int test_integer_more (void) { - int i, n1, n2, n3, n4, n5, n6; + int64_t i, n1, n2, n3, n4, n5, n6; n2 = 0; for (i = 0; i < (sizeof(int) * 8); i++) { @@ -522,23 +523,27 @@ static int test_heim_integer (void) { struct test_case tests[] = { + {NULL, 1, "\xff", NULL }, + {NULL, 2, "\xff\x01", NULL }, {NULL, 2, "\xfe\x01", NULL }, {NULL, 2, "\xef\x01", NULL }, {NULL, 3, "\xff\x00\xff", NULL }, {NULL, 3, "\xff\x01\x00", NULL }, {NULL, 1, "\x00", NULL }, {NULL, 1, "\x01", NULL }, - {NULL, 2, "\x00\x80", NULL } + {NULL, 2, "\x00\x80", NULL }, }; heim_integer values[] = { + { 1, "\x01", 1 }, + { 1, "\xff", 1 }, { 2, "\x01\xff", 1 }, { 2, "\x10\xff", 1 }, { 2, "\xff\x01", 1 }, { 2, "\xff\x00", 1 }, { 0, "", 0 }, { 1, "\x01", 0 }, - { 1, "\x80", 0 } + { 1, "\x80", 0 }, }; int i, ret; int ntests = sizeof(tests) / sizeof(tests[0]); diff --git a/lib/asn1/der_cmp.c b/lib/asn1/der_cmp.c index ccaf6fd0f..f745270f3 100644 --- a/lib/asn1/der_cmp.c +++ b/lib/asn1/der_cmp.c @@ -38,22 +38,25 @@ der_heim_oid_cmp(const heim_oid *p, const heim_oid *q) { int c; - if (p->length == q->length) + if (p->length == q->length) { + if (p->length == 0) + return 0; return memcmp(p->components, q->components, p->length * sizeof(*p->components)); + } if (p->length < q->length) { - c = memcmp(p->components, - q->components, - p->length * sizeof(*p->components)); - if (c == 0) + if (p->length == 0 || + (c = memcmp(p->components, + q->components, + p->length * sizeof(*p->components))) == 0) return -1; return c; } - c = memcmp(p->components, - q->components, - q->length * sizeof(*p->components)); - if (c == 0) + if (q->length == 0 || + (c = memcmp(p->components, + q->components, + q->length * sizeof(*p->components))) == 0) return 1; return c; } @@ -64,14 +67,19 @@ der_heim_octet_string_cmp(const heim_octet_string *p, { 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); + } 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 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 c; } @@ -94,22 +102,92 @@ int ASN1CALL der_heim_bit_string_cmp(const heim_bit_string *p, const heim_bit_string *q) { - int r1, r2; - size_t i; - if (p->length != q->length) - return (int)(p->length - q->length); - i = memcmp(p->data, q->data, p->length / 8); - if (i) - return (int)i; - if ((p->length % 8) == 0) - return 0; - i = (p->length / 8); - r1 = ((unsigned char *)p->data)[i]; - r2 = ((unsigned char *)q->data)[i]; - i = 8 - (p->length % 8); - r1 = r1 >> i; - r2 = r2 >> i; - return r1 - r2; + unsigned char pc, qc; + size_t bits; + int c = 0; + + /* Compare prefix */ + if (p->length == 0 && q->length == 0) + return 0; + if (p->length > 7 && q->length > 7) { + if (p->length < q->length) + c = memcmp(p->data, q->data, p->length / 8); + else + c = memcmp(p->data, q->data, q->length / 8); + } + if (c) + return c; + + /* 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 @@ -128,14 +206,19 @@ der_heim_bmp_string_cmp(const heim_bmp_string *p, const heim_bmp_string *q) { 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])); + } 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 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 c; } @@ -146,14 +229,19 @@ der_heim_universal_string_cmp(const heim_universal_string *p, { 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])); + } 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 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 c; } diff --git a/lib/asn1/der_get.c b/lib/asn1/der_get.c index c12f81700..06564b7ca 100644 --- a/lib/asn1/der_get.c +++ b/lib/asn1/der_get.c @@ -86,9 +86,12 @@ der_get_integer (const unsigned char *p, size_t len, int val = 0; 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; + /* We assume we're on a twos-complement platform */ if (len > 0) { val = (signed char)*p++; while (--len) @@ -109,6 +112,7 @@ der_get_integer64 (const unsigned char *p, size_t len, if (len > sizeof(val)) return ASN1_OVERRUN; + /* We assume we're on a twos-complement platform */ if (len > 0) { val = (signed char)*p++; while (--len) @@ -456,13 +460,45 @@ der_get_heim_integer (const unsigned char *p, size_t len, if (p[0] & 0x80) { unsigned char *q; 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; 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++; 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); if (data->data == NULL) { @@ -471,9 +507,17 @@ der_get_heim_integer (const unsigned char *p, size_t len, *size = 0; 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]; p += data->length - 1; while (q >= (unsigned char*)data->data) { + /* *p XOR 0xff -> ~*p; we're dealing with twos complement */ *q = *p ^ 0xff; if (carry) carry = !++*q; diff --git a/lib/asn1/der_length.c b/lib/asn1/der_length.c index 9a9913311..cd50df846 100644 --- a/lib/asn1/der_length.c +++ b/lib/asn1/der_length.c @@ -256,7 +256,9 @@ der_length_heim_integer (const heim_integer *k) { if (k->length == 0) 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); else return k->length + ((((unsigned char *)k->data)[0] & 0x80) ? 1 : 0); diff --git a/lib/asn1/der_put.c b/lib/asn1/der_put.c index 8fbd6f3da..106d45602 100644 --- a/lib/asn1/der_put.c +++ b/lib/asn1/der_put.c @@ -343,7 +343,8 @@ der_put_octet_string (unsigned char *p, size_t len, if (len < data->length) return ASN1_OVERFLOW; p -= data->length; - memcpy (p+1, data->data, data->length); + if (data->length) + memcpy(p+1, data->data, data->length); *size = data->length; return 0; } @@ -378,19 +379,30 @@ der_put_heim_integer (unsigned char *p, size_t len, if (data->negative) { ssize_t i; int carry; - for (i = data->length - 1, carry = 1; i >= 0; i--) { - *p = buf[i] ^ 0xff; - if (carry) - carry = !++*p; - p--; - } - if (p[1] < 128) { - if (len < 1) - return ASN1_OVERFLOW; - *p-- = 0xff; - len--; - hibitset = 1; - } + + /* + * We represent the parsed integer as a positive value with a + * negativity flag. But we need to put it on the wire as the shortest + * twos-complement byte sequence possible. So we're going to negate + * the number as go. + */ + if (data->length == 1 && *(unsigned char *)data->data == 1) { + *(p--) = 0xff; + } else { + for (i = data->length - 1, carry = 1; i >= 0; i--) { + *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 { p -= data->length; memcpy(p + 1, buf, data->length);