From f0feaab93868decfe645f2190a281f26a2a03c0d Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Wed, 30 Nov 2022 11:22:22 -0600 Subject: [PATCH] asn1: Template CHOICE element 0 bug While we no longer have a decoder CHOICE element 0 bug, we did still have one encode and copy and free that was leading to a memory leak (and _save trashing) prior to the fix for asn1: Fix 1-byte leaks in der_copy_octet_string() This commit fixes that. --- lib/asn1/check-gen.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ lib/asn1/template.c | 39 ++++++++++++++++++++++++----- 2 files changed, 92 insertions(+), 6 deletions(-) diff --git a/lib/asn1/check-gen.c b/lib/asn1/check-gen.c index 6b5c71c39..bba5b9db2 100644 --- a/lib/asn1/check-gen.c +++ b/lib/asn1/check-gen.c @@ -655,6 +655,12 @@ test_cert(void) size_t i; int ret; + memset(&c, 0, sizeof(c)); + ret = copy_Certificate(&c, &c2); + if (ret) + return ret; + free_Certificate(&c2); + for (i = 0; i < sizeof(certs)/sizeof(certs[0]); i++) { ret = decode_Certificate((unsigned char *)certs[i].cert, @@ -1114,6 +1120,57 @@ test_decorated(void) return 0; } +static int +test_extensible_choice(void) +{ + PA_FX_FAST_REQUEST r, r2; + size_t len, size; + void *ptr; + int ret; + + memset(&r, 0, sizeof(r)); + + ret = copy_PA_FX_FAST_REQUEST(&r, &r2); + if (ret) + return ret; + free_PA_FX_FAST_REQUEST(&r2); + + r.element = 0; + r.u.asn1_ellipsis.data = "hello"; + r.u.asn1_ellipsis.length = sizeof("hello") - 1; + ret = copy_PA_FX_FAST_REQUEST(&r, &r2); + if (ret) + errx(1, "Out of memory"); + if (r2.element != 0) + errx(1, "Extensible CHOICE copy failure to set discriminant to 0"); + if (r2.u.asn1_ellipsis.length != r.u.asn1_ellipsis.length) + errx(1, "Extensible CHOICE copy failure to copy extension"); + if (memcmp(r.u.asn1_ellipsis.data, r2.u.asn1_ellipsis.data, + r.u.asn1_ellipsis.length) != 0) + errx(1, "Extensible CHOICE copy failure to copy extension (2)"); + free_PA_FX_FAST_REQUEST(&r2); + + ASN1_MALLOC_ENCODE(PA_FX_FAST_REQUEST, ptr, len, &r, &size, ret); + if (ret || len != size) + errx(1, "Extensible CHOICE encoding failure"); + + ret = decode_PA_FX_FAST_REQUEST(ptr, len, &r2, &size); + if (ret || len != size) + errx(1, "Extensible CHOICE decoding failure"); + + if (r2.element != 0) + errx(1, "Extensible CHOICE decode failure to set discriminant to 0"); + if (r2.u.asn1_ellipsis.length != r.u.asn1_ellipsis.length) + errx(1, "Extensible CHOICE decode failure to copy extension"); + if (memcmp(r.u.asn1_ellipsis.data, r2.u.asn1_ellipsis.data, + r.u.asn1_ellipsis.length) != 0) + errx(1, "Extensible CHOICE decode failure to copy extension (2)"); + + free_PA_FX_FAST_REQUEST(&r2); + free(ptr); + return 0; +} + static int test_decorated_choice(void) { @@ -2674,6 +2731,8 @@ main(int argc, char **argv) DO_ONE(test_default); + DO_ONE(test_extensible_choice); + DO_ONE(test_decorated_choice); DO_ONE(test_decorated); diff --git a/lib/asn1/template.c b/lib/asn1/template.c index 31eb66004..c2112174b 100644 --- a/lib/asn1/template.c +++ b/lib/asn1/template.c @@ -1176,6 +1176,13 @@ _asn1_decode(const struct asn1_template *t, unsigned flags, return ret; } if (i >= A1_HEADER_LEN(choice) + 1 || !choice[i].tt) { + /* + * If this is an extensible CHOICE, then choice->tt will be the + * offset to u.ellipsis. If it's not, then this "extension" is + * an error and must stop parsing it. (We could be permissive + * and throw away the extension, though one might as well just + * mark such a CHOICE as extensible.) + */ if (choice->tt == 0) return ASN1_BAD_ID; @@ -1786,16 +1793,20 @@ _asn1_encode(const struct asn1_template *t, unsigned char *p, size_t len, const } if (*element == 0) { - ret += der_put_octet_string(p, len, - DPOC(data, choice->tt), &datalen); + if (choice->tt) { + /* This is an extensible CHOICE */ + ret += der_put_octet_string(p, len, + DPOC(data, choice->tt), &datalen); + len -= datalen; p -= datalen; + } /* else this is really an error -- XXX what to do? */ } else { choice += *element; el = DPOC(data, choice->offset); ret = _asn1_encode(choice->ptr, p, len, el, &datalen); if (ret) return ret; + len -= datalen; p -= datalen; } - len -= datalen; p -= datalen; break; } @@ -2175,7 +2186,8 @@ _asn1_length(const struct asn1_template *t, const void *data) break; if (*element == 0) { - ret += der_length_octet_string(DPOC(data, choice->tt)); + if (choice->tt) + ret += der_length_octet_string(DPOC(data, choice->tt)); } else { choice += *element; ret += _asn1_length(choice->ptr, DPOC(data, choice->offset)); @@ -2350,7 +2362,16 @@ _asn1_free(const struct asn1_template *t, void *data) break; if (*element == 0) { - der_free_octet_string(DPO(data, choice->tt)); + /* + * If choice->tt != 0 then this is an extensible choice, and + * the offset choice->tt is the offset to u.ellipsis. + */ + if (choice->tt != 0) + der_free_octet_string(DPO(data, choice->tt)); + /* + * Else this was a not-fully initialized CHOICE. We could + * stand to memset clear the rest of it though... + */ } else { choice += *element; _asn1_free(choice->ptr, DPO(data, choice->offset)); @@ -2727,6 +2748,7 @@ _asn1_print(const struct asn1_template *t, if (*element > A1_HEADER_LEN(choice)) { r = rk_strpoolprintf(r, "null"); } else if (*element == 0) { + /* XXX If choice->tt then we should print the u.ellipsis */ r = rk_strpoolprintf(r, "null"); } else { choice += *element; @@ -3024,7 +3046,12 @@ _asn1_copy(const struct asn1_template *t, const void *from, void *to) *telement = *felement; if (*felement == 0) { - ret = der_copy_octet_string(DPOC(from, choice->tt), DPO(to, choice->tt)); + if (choice->tt) + ret = der_copy_octet_string(DPOC(from, choice->tt), DPO(to, choice->tt)); + /* + * Else we should really memset clear the rest of this choice, + * but we don't really know its size. + */ } else { choice += *felement; ret = _asn1_copy(choice->ptr,