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 <ben@openssl.org>

diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
index b025e5809f8efb62d4a69edffccb0dbe91965303..571592199f907ade360a1dcd3df6b40ab9a08176 100644 (file)
--- 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 e10af0bdde62d0ceb4a5c28e5a387a0ab0a22e18..ee66729f0619eff5d80831991d4f6f2dd02348f3 100644 (file)
--- 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 74f83a38173c3ad8d79f8052c8c1c7edc5328955..083412cba833feeac071a1e2e9e9bb998bd08c1f 100644 (file)
--- 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 (file)
index 0000000..f75efad
Binary files /dev/null and b/test/d2i-tests/bad_cert.der differ

diff --git a/test/d2i-tests/bad_generalname.der b/test/d2i-tests/bad_generalname.der
new file mode 100644 (file)
index 0000000..af45855
--- /dev/null
+++ b/test/d2i-tests/bad_generalname.der
@@ -0,0 +1 @@
+¥\80\140;¶\ 2!;)''ï÷!l¿(,:µ¿(*;©:§«½:\93*\8d*;i)*w*ë)ã;U\b:'\a):ñ;l*!'Ò£
\ No newline at end of file

diff --git a/test/d2i_test.c b/test/d2i_test.c
new file mode 100644 (file)
index 0000000..cf01012
--- /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 <stdio.h>
+#include <string.h>
+
+#include "testutil.h"
+
+#include <openssl/asn1.h>
+#include <openssl/bio.h>
+#include <openssl/err.h>
+#include <openssl/x509.h>
+#include <openssl/x509v3.h>
+
+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 <type> <file>, 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 (file)
index 0000000..a9c259d
--- /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");