From 1400f013e10c8ec624947d9187bebb20274385dc Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Wed, 30 Mar 2016 22:37:05 +0200 Subject: [PATCH] Fix memory leaks in ASN.1 These leaks affect 1.1.0 dev branch only; introduced around commit f93ad22f6adb00e722c130e792799467f3927b56 Found with LibFuzzer Reviewed-by: Ben Laurie --- crypto/asn1/tasn_dec.c | 9 ++- test/Makefile.in | 10 ++- test/build.info | 6 +- test/d2i-tests/bad_cert.der | Bin 0 -> 1007 bytes test/d2i-tests/bad_generalname.der | 1 + test/d2i_test.c | 117 +++++++++++++++++++++++++++++ test/recipes/25-test_d2i.t | 19 +++++ 7 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 test/d2i-tests/bad_cert.der create mode 100644 test/d2i-tests/bad_generalname.der create mode 100644 test/d2i_test.c create mode 100644 test/recipes/25-test_d2i.t diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c index b025e5809f..571592199f 100644 --- a/crypto/asn1/tasn_dec.c +++ b/crypto/asn1/tasn_dec.c @@ -273,6 +273,12 @@ static int asn1_item_embed_d2i(ASN1_VALUE **pval, const unsigned char **in, /* If field not present, try the next one */ if (ret == -1) continue; + /* + * Set the choice selector here to ensure that the value is + * correctly freed upon error. It may be partially initialized + * even if parsing failed. + */ + asn1_set_choice_selector(pval, i, it); /* If positive return, read OK, break loop */ if (ret > 0) break; @@ -294,7 +300,6 @@ static int asn1_item_embed_d2i(ASN1_VALUE **pval, const unsigned char **in, goto err; } - asn1_set_choice_selector(pval, i, it); if (asn1_cb && !asn1_cb(ASN1_OP_D2I_POST, pval, it, NULL)) goto auxerr; *in = p; @@ -617,6 +622,8 @@ static int asn1_template_noexp_d2i(ASN1_VALUE **val, ASN1_ITEM_ptr(tt->item), -1, 0, 0, ctx)) { ASN1err(ASN1_F_ASN1_TEMPLATE_NOEXP_D2I, ERR_R_NESTED_ASN1_ERROR); + /* |skfield| may be partially allocated despite failure. */ + ASN1_item_free(skfield, ASN1_ITEM_ptr(tt->item)); goto err; } len -= p - q; diff --git a/test/Makefile.in b/test/Makefile.in index e10af0bdde..ee66729f06 100644 --- a/test/Makefile.in +++ b/test/Makefile.in @@ -84,6 +84,7 @@ DTLSV1LISTENTEST = dtlsv1listentest CTTEST= ct_test THREADSTEST= threadstest AFALGTEST= afalgtest +D2ITEST = d2i_test TESTS= alltests @@ -106,7 +107,7 @@ EXE= $(NPTEST)$(EXE_EXT) $(MEMLEAKTEST)$(EXE_EXT) \ $(CONSTTIMETEST)$(EXE_EXT) $(VERIFYEXTRATEST)$(EXE_EXT) \ $(CLIENTHELLOTEST)$(EXE_EXT) $(PACKETTEST)$(EXE_EXT) $(ASYNCTEST)$(EXE_EXT) \ $(DTLSV1LISTENTEST)$(EXE_EXT) $(CTTEST)$(EXE_EXT) $(THREADSTEST)$(EXE_EXT) \ - $(AFALGTEST)$(EXE_EXT) + $(AFALGTEST)$(EXE_EXT) $(D2ITEST)$(EXE_EXT) # $(METHTEST)$(EXE_EXT) @@ -124,7 +125,7 @@ OBJ= $(NPTEST).o $(MEMLEAKTEST).o \ $(HEARTBEATTEST).o $(P5_CRPT2_TEST).o \ $(CONSTTIMETEST).o $(VERIFYEXTRATEST).o $(CLIENTHELLOTEST).o \ $(PACKETTEST).o $(ASYNCTEST).o $(DTLSV1LISTENTEST).o $(CTTEST).o \ - $(THREADSTEST).o testutil.o $(AFALGTEST).o + $(THREADSTEST).o testutil.o $(AFALGTEST).o $(D2ITEST).o SRC= $(NPTEST).c $(MEMLEAKTEST).c \ $(BNTEST).c $(ECTEST).c \ @@ -139,7 +140,7 @@ SRC= $(NPTEST).c $(MEMLEAKTEST).c \ $(HEARTBEATTEST).c $(P5_CRPT2_TEST).c \ $(CONSTTIMETEST).c $(VERIFYEXTRATEST).c $(CLIENTHELLOTEST).c \ $(PACKETTEST).c $(ASYNCTEST).c $(DTLSV1LISTENTEST).c $(CTTEST).c \ - $(THREADSTEST).c testutil.c $(AFALGTEST).c + $(THREADSTEST).c testutil.c $(AFALGTEST).c $(D2ITEST).c HEADER= testutil.h @@ -385,4 +386,7 @@ dummytest$(EXE_EXT): dummytest.o $(DLIBCRYPTO) $(AFALGTEST)$(EXE_EXT): $(AFALGTEST).o $(DLIBCRYPTO) @target=$(AFALGTEST); $(BUILD_CMD) +$(D2ITEST)$(EXE_EXT): $(D2ITEST).o $(DLIBCRYPTO) testutil.o + @target=$(D2ITEST) testutil=testutil.o; $(BUILD_CMD) + # DO NOT DELETE THIS LINE -- make depend depends on it. diff --git a/test/build.info b/test/build.info index 74f83a3817..083412cba8 100644 --- a/test/build.info +++ b/test/build.info @@ -14,7 +14,7 @@ PROGRAMS=\ danetest heartbeat_test p5_crpt2_test \ constant_time_test verify_extra_test clienthellotest \ packettest asynctest secmemtest srptest memleaktest \ - dtlsv1listentest ct_test threadstest afalgtest + dtlsv1listentest ct_test threadstest afalgtest d2i_test SOURCE[aborttest]=aborttest.c INCLUDE[aborttest]={- rel2abs(catdir($builddir,"../include")) -} ../include @@ -220,4 +220,8 @@ SOURCE[afalgtest]=afalgtest.c INCLUDE[afalgtest]={- rel2abs(catdir($builddir,"../include")) -} .. ../include DEPEND[afalgtest]=../libcrypto +SOURCE[d2i_test]=d2i_test.c testutil.c +INCLUDE[d2i_test]={- rel2abs(catdir($builddir,"../include")) -} .. ../include +DEPEND[d2i_test]=../libcrypto + INCLUDE[testutil.o]=.. diff --git a/test/d2i-tests/bad_cert.der b/test/d2i-tests/bad_cert.der new file mode 100644 index 0000000000000000000000000000000000000000..f75efad398a6192b4770339b641eb80d0d574d79 GIT binary patch literal 1007 zcmXqLV!me3#I$b#GZP~d6G!%bmO#G9lhFpeY@Awc9&O)w85y}*84NrPxeYkkm_u3E zgqcEv4TTK^K^!h&F8BQW^qf=$&%9(qRRd+9;5lYt8Mt6cYEfQliGpKkNk)EAW=W-j zyODvMIIp3pp@ETwp@pfLu|*V+Yi3|*WM%*cQ3jbWO}a!$<2Ov^9I%S<#BGY|n< zb%EWW!Jxr`0Z8!h`sSDBl_X~7DTHOJr_?#^hBO3>pv6h*Uo!NHCV}s})FXc|x`CX0j3*{^LIj?6w zL$%6LS@R7(Y7SFBf7`iew#DOyr0mx%)#u+V;xcMe(ti_sd2e;b;wHvygC@pw16iQm zvZ^c~w{vK-F|x9MBqokz3N?*SqI}=I2zPqQRyOCaUQ3;Y_M!4eSoXpg`l43(- z9r?+{1rR^*v52vV95#E_H93b-Pdh#^+VA=T!SsODR}FYTt`cTs{LjKn@v^^qy}STg$#SZrhzCwJWMo-#`(hK!HWdKmw?f3m6hCjMpleSQ!mC*x1q= zd4LJhzyPE~o<-9@-9UAL@&ZM;6LI(_2QB3?H!(6Yq*|Ptx6p5SWP;Dee>-2P2kd%& zU-IE!`8-`lHvOQ?Lnn>js=nbjSguw&|L)-|6Tu+Ae|i5kK1bC`x82{gyG`X}=QN2c zqJqUH1x2?HGJ7d(&5~<$P*^0n@W2i0O4(Zx2QE9@{L}Tq&Fj7J()CvjYb!3y(5y+m zsq^7$_Y0M0f(I8Jh<_<&DEM{3H2KW#r7g}P-!|6jDNa3^7dvy$3dZHW+d8bSy07D{ zb&0d&76T?}4h9BL!e(j^D&?8}pT}1>d>$*?kzkwX0+YTAdG?>?wC?&hQN%ZYk6XZ` z>eQ#13xwX!pY9{vI8*YwbRCaIoKl{k{GOFxtRC^Cnr$`JDM)wY{%YzTl~AO85diIr BJ4^ro literal 0 HcmV?d00001 diff --git a/test/d2i-tests/bad_generalname.der b/test/d2i-tests/bad_generalname.der new file mode 100644 index 0000000000..af45855c52 --- /dev/null +++ b/test/d2i-tests/bad_generalname.der @@ -0,0 +1 @@ +¥€0;¶!;)''ï÷!l¿(,:µ¿(*;©:§«½:“**;i)*w*ë)ã;U:'):ñ;l*!'Ò£ \ No newline at end of file diff --git a/test/d2i_test.c b/test/d2i_test.c new file mode 100644 index 0000000000..cf01012ef9 --- /dev/null +++ b/test/d2i_test.c @@ -0,0 +1,117 @@ +/* + * Copyright 2016 The OpenSSL Project Authors. All Rights Reserved. + * + * Licensed under the OpenSSL licenses, (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * https://www.openssl.org/source/license.html + * or in the file LICENSE in the source distribution. + */ + +/* Regression tests for ASN.1 parsing bugs. */ + +#include +#include + +#include "testutil.h" + +#include +#include +#include +#include +#include + +static const ASN1_ITEM *item_type; +static const char *test_file; + +typedef struct d2i_test_fixture { + const char *test_case_name; +} D2I_TEST_FIXTURE; + + +static D2I_TEST_FIXTURE set_up(const char *const test_case_name) +{ + D2I_TEST_FIXTURE fixture; + fixture.test_case_name = test_case_name; + return fixture; +} + +static int execute_test(D2I_TEST_FIXTURE fixture) +{ + BIO *bio = NULL; + ASN1_VALUE *value = NULL; + int ret = 1; + unsigned char buf[2048]; + const unsigned char *buf_ptr = buf; + int len; + + if ((bio = BIO_new_file(test_file, "r")) == NULL) + return 1; + + /* + * We don't use ASN1_item_d2i_bio because it, apparently, + * errors too early for some inputs. + */ + len = BIO_read(bio, buf, sizeof buf); + if (len < 0) + goto err; + + value = ASN1_item_d2i(NULL, &buf_ptr, len, item_type); + if (value != NULL) + goto err; + + ret = 0; + + err: + BIO_free(bio); + ASN1_item_free(value, item_type); + return ret; +} + +static void tear_down(D2I_TEST_FIXTURE fixture) +{ + ERR_print_errors_fp(stderr); +} + +#define SETUP_D2I_TEST_FIXTURE() \ + SETUP_TEST_FIXTURE(D2I_TEST_FIXTURE, set_up) + +#define EXECUTE_D2I_TEST() \ + EXECUTE_TEST(execute_test, tear_down) + +static int test_bad_asn1() +{ + SETUP_D2I_TEST_FIXTURE(); + EXECUTE_D2I_TEST(); +} + +/* + * Usage: d2i_test , e.g. + * d2i_test generalname bad_generalname.der + */ +int main(int argc, char **argv) +{ + int result = 0; + const char *test_type_name; + + if (argc != 3) + return 1; + + test_type_name = argv[1]; + test_file = argv[2]; + + if (strcmp(test_type_name, "generalname") == 0) { + item_type = ASN1_ITEM_rptr(GENERAL_NAME); + } else if (strcmp(test_type_name, "x509") == 0) { + item_type = ASN1_ITEM_rptr(X509); + } else { + fprintf(stderr, "Bad type %s\n", test_type_name); + return 1; + } + + ADD_TEST(test_bad_asn1); + + result = run_tests(argv[0]); + + return result; +} diff --git a/test/recipes/25-test_d2i.t b/test/recipes/25-test_d2i.t new file mode 100644 index 0000000000..a9c259d118 --- /dev/null +++ b/test/recipes/25-test_d2i.t @@ -0,0 +1,19 @@ +#! /usr/bin/perl + +use strict; +use warnings; + +use File::Spec; +use OpenSSL::Test qw/:DEFAULT srctop_file/; + +setup("test_d2i"); + +plan tests => 2; + +ok(run(test(["d2i_test", "x509", + srctop_file('test','d2i-tests','bad_cert.der')])), + "Running d2i_test bad_cert.der"); + +ok(run(test(["d2i_test", "generalname", + srctop_file('test','d2i-tests','bad_generalname.der')])), + "Running d2i_test bad_generalname.der"); -- 2.25.1