From: Viktor Dukhovni Date: Mon, 2 May 2016 18:46:51 +0000 (-0400) Subject: Fix i2d_X509_AUX, update docs and add tests X-Git-Tag: OpenSSL_1_1_0-pre6~873 X-Git-Url: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff_plain;h=fde2257f055f187e8e78542ea6d64ad6c206d10b 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 --- 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 - 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 into DER format. If B is not B 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. 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 +#include +#include + +#include +#include +#include +#include + +#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); +}