Fix i2d_X509_AUX, update docs and add tests

When *pp is NULL, don't write garbage, return an unexpected pointer
or leak memory on error.

Reviewed-by: Dr. Stephen Henson <steve@openssl.org>

6 files changed, 325 insertions, 7 deletions

diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c
index 043ab07866..3eba360c47 100644
--- a/crypto/x509/x_x509.c
+++ b/crypto/x509/x_x509.c
@@ -181,12 +181,26 @@ X509 *d2i_X509_AUX(X509 **a, const unsigned char **pp, long length)
     return NULL;
 }
 
-int i2d_X509_AUX(X509 *a, unsigned char **pp)
+/*
+ * Serialize trusted certificate to *pp or just return the required buffer
+ * length if pp == NULL.  We ultimately want to avoid modifying *pp in the
+ * error path, but that depends on similar hygiene in lower-level functions.
+ * Here we avoid compounding the problem.
+ */
+static int i2d_x509_aux_internal(X509 *a, unsigned char **pp)
 {
     int length, tmplen;
     unsigned char *start = pp != NULL ? *pp : NULL;
+
+    OPENSSL_assert(pp == NULL || *pp != NULL);
+
+    /*
+     * This might perturb *pp on error, but fixing that belongs in i2d_X509()
+     * not here.  It should be that if a == NULL length is zero, but we check
+     * both just in case.
+     */
     length = i2d_X509(a, pp);
-    if (length < 0 || a == NULL)
+    if (length <= 0 || a == NULL)
         return length;
 
     tmplen = i2d_X509_CERT_AUX(a->aux, pp);
@@ -200,6 +214,42 @@ int i2d_X509_AUX(X509 *a, unsigned char **pp)
     return length;
 }
 
+/*
+ * Serialize trusted certificate to *pp, or just return the required buffer
+ * length if pp == NULL.
+ *
+ * When pp is not NULL, but *pp == NULL, we allocate the buffer, but since
+ * we're writing two ASN.1 objects back to back, we can't have i2d_X509() do
+ * the allocation, nor can we allow i2d_X509_CERT_AUX() to increment the
+ * allocated buffer.
+ */
+int i2d_X509_AUX(X509 *a, unsigned char **pp)
+{
+    int length;
+    unsigned char *tmp;
+
+    /* Buffer provided by caller */
+    if (pp == NULL || *pp != NULL)
+        return i2d_x509_aux_internal(a, pp);
+
+    /* Obtain the combined length */
+    if ((length = i2d_x509_aux_internal(a, NULL)) <= 0)
+        return length;
+
+    /* Allocate requisite combined storage */
+    *pp = tmp = OPENSSL_malloc(length);
+    if (tmp == NULL)
+        return -1; /* Push error onto error stack? */
+
+    /* Encode, but keep *pp at the originally malloced pointer */
+    length = i2d_x509_aux_internal(a, &tmp);
+    if (length <= 0) {
+        OPENSSL_free(*pp);
+        *pp = NULL;
+    }
+    return length;
+}
+
 int i2d_re_X509_tbs(X509 *x, unsigned char **pp)
 {
     x->cert_info.enc.modified = 1;
diff --git a/doc/crypto/d2i_X509.pod b/doc/crypto/d2i_X509.pod
index 3cd2509d8b..14b84f24ab 100644
--- a/doc/crypto/d2i_X509.pod
+++ b/doc/crypto/d2i_X509.pod
@@ -9,8 +9,10 @@ i2d_X509_fp - X509 encode and decode functions
 
  #include <openssl/x509.h>
 
- X509 *d2i_X509(X509 **px, const unsigned char **in, int len);
+ X509 *d2i_X509(X509 **px, const unsigned char **in, long len);
+ X509 *d2i_X509_AUX(X509 **px, const unsigned char **in, long len);
  int i2d_X509(X509 *x, unsigned char **out);
+ int i2d_X509_AUX(X509 *x, unsigned char **out);
 
  X509 *d2i_X509_bio(BIO *bp, X509 **x);
  X509 *d2i_X509_fp(FILE *fp, X509 **x);
@@ -37,6 +39,11 @@ below, and the discussion in the RETURN VALUES section).
 If the call is successful B<*in> is incremented to the byte following the
 parsed data.
 
+d2i_X509_AUX() is similar to d2i_X509() but the input is expected to consist of
+an X509 certificate followed by auxiliary trust information.
+This is used by the PEM routines to read "TRUSTED CERTIFICATE" objects.
+This function should not be called on untrusted input.
+
 i2d_X509() encodes the structure pointed to by B<x> into DER format.
 If B<out> is not B<NULL> is writes the DER encoded data to the buffer
 at B<*out>, and increments it to point after the data just written.
@@ -48,6 +55,11 @@ allocated for a buffer and the encoded data written to it. In this
 case B<*out> is not incremented and it points to the start of the
 data just written.
 
+i2d_X509_AUX() is similar to i2d_X509(), but the encoded output contains both
+the certificate and any auxiliary trust information.
+This is used by the PEM routines to write "TRUSTED CERTIFICATE" objects.
+Note, this is a non-standard OpenSSL-specific data format.
+
 d2i_X509_bio() is similar to d2i_X509() except it attempts
 to parse data from BIO B<bp>.
 
diff --git a/test/build.info b/test/build.info
index 4fd4d99ece..8bd7f62d37 100644
--- a/test/build.info
+++ b/test/build.info
@@ -16,7 +16,7 @@ IF[{- !$disabled{tests} -}]
           constant_time_test verify_extra_test clienthellotest \
           packettest asynctest secmemtest srptest memleaktest \
           dtlsv1listentest ct_test threadstest afalgtest d2i_test \
-          ssl_test_ctx_test ssl_test
+          ssl_test_ctx_test ssl_test x509aux
 
   SOURCE[aborttest]=aborttest.c
   INCLUDE[aborttest]={- rel2abs(catdir($builddir,"../include")) -} ../include
@@ -237,4 +237,8 @@ IF[{- !$disabled{tests} -}]
   INCLUDE[testutil.o]=..
   INCLUDE[ssl_test_ctx.o]={- rel2abs(catdir($builddir,"../include")) -} ../include
   INCLUDE[handshake_helper.o]={- rel2abs(catdir($builddir,"../include")) -} ../include
+
+  SOURCE[x509aux]=x509aux.c
+  INCLUDE[x509aux]={- rel2abs(catdir($builddir,"../include")) -} ../include
+  DEPEND[x509aux]=../libcrypto
 ENDIF
diff --git a/test/danetest.c b/test/danetest.c
index e89f71100a..3bcc02e504 100644
--- a/test/danetest.c
+++ b/test/danetest.c
@@ -475,7 +475,7 @@ int main(int argc, char *argv[])
     progname = argv[0];
     if (argc != 4) {
         test_usage();
-        EXIT(1);
+        EXIT(ret);
     }
     basedomain = argv[1];
     CAfile = argv[2];
@@ -492,10 +492,9 @@ int main(int argc, char *argv[])
     if (f == NULL) {
         fprintf(stderr, "%s: Error opening tlsa record file: '%s': %s\n",
                 progname, tlsafile, strerror(errno));
-        return 0;
+        EXIT(ret);
     }
 
-
     ctx = SSL_CTX_new(TLS_client_method());
     if (SSL_CTX_dane_enable(ctx) <= 0) {
         print_errors();
diff --git a/test/recipes/80-test_x509aux.t b/test/recipes/80-test_x509aux.t
new file mode 100644
index 0000000000..65ba5fcf52
--- /dev/null
+++ b/test/recipes/80-test_x509aux.t
@@ -0,0 +1,27 @@
+#! /usr/bin/env perl
+# Copyright 2015-2016 The OpenSSL Project Authors. All Rights Reserved.
+#
+# Licensed under the OpenSSL license (the "License").  You may not use
+# this file except in compliance with the License.  You can obtain a copy
+# in the file LICENSE in the source distribution or at
+# https://www.openssl.org/source/license.html
+
+
+use strict;
+use warnings;
+use OpenSSL::Test qw/:DEFAULT srctop_file/;
+use OpenSSL::Test::Utils;
+
+setup("test_x509aux");
+
+plan skip_all => "test_dane uses ec which is not supported by this OpenSSL build"
+    if disabled("ec");
+
+plan tests => 1;                # The number of tests being performed
+
+ok(run(test(["x509aux", 
+                srctop_file("test", "certs", "roots.pem"),
+                srctop_file("test", "certs", "root+anyEKU.pem"),
+                srctop_file("test", "certs", "root-anyEKU.pem"),
+                srctop_file("test", "certs", "root-cert.pem")]
+        )), "x509aux tests");
diff --git a/test/x509aux.c b/test/x509aux.c
new file mode 100644
index 0000000000..4f00196312
--- /dev/null
+++ b/test/x509aux.c
@@ -0,0 +1,226 @@
+/*
+ * 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.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+
+#include <openssl/x509.h>
+#include <openssl/pem.h>
+#include <openssl/conf.h>
+#include <openssl/err.h>
+
+#include "../e_os.h"
+
+static const char *progname;
+
+static void test_usage(void)
+{
+    fprintf(stderr, "usage: %s certfile\n", progname);
+}
+
+static void print_errors(void)
+{
+    unsigned long err;
+    char buffer[1024];
+    const char *file;
+    const char *data;
+    int line;
+    int flags;
+
+    while ((err = ERR_get_error_line_data(&file, &line, &data, &flags)) != 0) {
+        ERR_error_string_n(err, buffer, sizeof(buffer));
+        if (flags & ERR_TXT_STRING)
+            fprintf(stderr, "Error: %s:%s:%d:%s\n", buffer, file, line, data);
+        else
+            fprintf(stderr, "Error: %s:%s:%d\n", buffer, file, line);
+    }
+}
+
+static int test_certs(BIO *fp)
+{
+    int count;
+    char *name = 0;
+    char *header = 0;
+    unsigned char *data = 0;
+    long len;
+    typedef X509 *(*d2i_X509_t)(X509 **, const unsigned char **, long);
+    typedef int (*i2d_X509_t)(X509 *, unsigned char **);
+    int err = 0;
+
+    for (count = 0;
+         !err && PEM_read_bio(fp, &name, &header, &data, &len);
+	 ++count) {
+        int trusted = strcmp(name, PEM_STRING_X509_TRUSTED) == 0;
+        d2i_X509_t d2i = trusted ? d2i_X509_AUX : d2i_X509;
+        i2d_X509_t i2d = trusted ? i2d_X509_AUX : i2d_X509;
+        X509 *cert = NULL;
+	const unsigned char *p = data;
+        unsigned char *buf = NULL;
+        unsigned char *bufp;
+        long enclen;
+
+	if (!trusted
+            && strcmp(name, PEM_STRING_X509) != 0
+	    && strcmp(name, PEM_STRING_X509_OLD) != 0) {
+	    fprintf(stderr, "unexpected PEM object: %s\n", name);
+            err = 1;
+	    goto next;
+        }
+        cert = d2i(NULL, &p, len);
+
+        if (cert == NULL || (p - data) != len) {
+	    fprintf(stderr, "error parsing input %s\n", name);
+            err = 1;
+            goto next;
+        }
+
+        /* Test traditional 2-pass encoding into caller allocated buffer */
+        enclen = i2d(cert, NULL);
+        if (len != enclen) {
+	    fprintf(stderr, "encoded length %ld of %s != input length %ld\n",
+                    enclen, name, len);
+            err = 1;
+            goto next;
+        }
+        if ((buf = bufp = OPENSSL_malloc(len)) == NULL) {
+            perror("malloc");
+            err = 1;
+            goto next;
+        }
+        enclen = i2d(cert, &bufp);
+        if (len != enclen) {
+	    fprintf(stderr, "encoded length %ld of %s != input length %ld\n",
+                    enclen, name, len);
+            err = 1;
+            goto next;
+        }
+        enclen = (long) (bufp - buf);
+        if (enclen != len) {
+	    fprintf(stderr, "unexpected buffer position after encoding %s\n",
+                    name);
+            err = 1;
+            goto next;
+        }
+        if (memcmp(buf, data, len) != 0) {
+	    fprintf(stderr, "encoded content of %s does not match input\n",
+                    name);
+            err = 1;
+            goto next;
+        }
+        OPENSSL_free(buf);
+        buf = NULL;
+
+        /* Test 1-pass encoding into library allocated buffer */
+        enclen = i2d(cert, &buf);
+        if (len != enclen) {
+	    fprintf(stderr, "encoded length %ld of %s != input length %ld\n",
+                    enclen, name, len);
+            err = 1;
+            goto next;
+        }
+        if (memcmp(buf, data, len) != 0) {
+	    fprintf(stderr, "encoded content of %s does not match input\n",
+                    name);
+            err = 1;
+            goto next;
+        }
+
+        if (trusted) {
+            /* Encode just the cert and compare with initial encoding */
+            OPENSSL_free(buf);
+            buf = NULL;
+
+            /* Test 1-pass encoding into library allocated buffer */
+            enclen = i2d(cert, &buf);
+            if (enclen > len) {
+                fprintf(stderr, "encoded length %ld of %s > input length %ld\n",
+                        enclen, name, len);
+                err = 1;
+                goto next;
+            }
+            if (memcmp(buf, data, enclen) != 0) {
+                fprintf(stderr, "encoded cert content does not match input\n");
+                err = 1;
+                goto next;
+            }
+        }
+
+	/*
+	 * If any of these were null, PEM_read() would have failed.
+	 */
+    next:
+        X509_free(cert);
+        OPENSSL_free(buf);
+	OPENSSL_free(name);
+	OPENSSL_free(header);
+	OPENSSL_free(data);
+    }
+
+    if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) {
+        /* Reached end of PEM file */
+        if (count > 0) {
+            ERR_clear_error();
+            return 1;
+        }
+    }
+
+    /* Some other PEM read error */
+    print_errors();
+    return 0;
+}
+
+int main(int argc, char *argv[])
+{
+    BIO *bio_err;
+    const char *certfile;
+    const char *p;
+    int ret = 1;
+
+    progname = argv[0];
+    if (argc < 2) {
+        test_usage();
+        EXIT(ret);
+    }
+
+    bio_err = BIO_new_fp(stderr, BIO_NOCLOSE | BIO_FP_TEXT);
+
+    p = getenv("OPENSSL_DEBUG_MEMORY");
+    if (p != NULL && strcmp(p, "on") == 0)
+        CRYPTO_set_mem_debug(1);
+    CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ON);
+
+    while ((certfile = *++argv) != NULL) {
+        BIO *f = BIO_new_file(certfile, "r");
+        int ok;
+
+        if (f == NULL) {
+            fprintf(stderr, "%s: Error opening cert file: '%s': %s\n",
+                    progname, certfile, strerror(errno));
+            EXIT(ret);
+        }
+        ret = !(ok = test_certs(f));
+        BIO_free(f);
+
+        if (!ok) {
+            printf("%s ERROR\n", certfile);
+            ret = 1;
+            break;
+        }
+        printf("%s OK\n", certfile);
+    }
+
+#ifndef OPENSSL_NO_CRYPTO_MDEBUG
+    if (CRYPTO_mem_leaks(bio_err) <= 0)
+        ret = 1;
+#endif
+    BIO_free(bio_err);
+    EXIT(ret);
+}