Fix ERR_print_errors so that it matches the documented format in doc/man3/ERR_error_s...
authorShane Lontis <shane.lontis@oracle.com>
Tue, 26 May 2020 02:44:36 +0000 (12:44 +1000)
committerShane Lontis <shane.lontis@oracle.com>
Tue, 26 May 2020 02:44:36 +0000 (12:44 +1000)
Fixes #11743

The ouput format had 2 issues that caused it not to match the expected documented format:
(1) At some point the thread id printing was changed to use the OPENSSL_hex2str method which puts ':' between hex bytes.
    An internal function that skips the seperator has been added.
(2) The error code no longer exists. So this was completely removed from the string. It is now replaced by ::

As an example:
  00:77:6E:52:14:7F:00:00:error:asn1 encoding routines:asn1_check_tlen:wrong tag:crypto/asn1/tasn_dec.c:1135:
Is now:
  00776E52147F0000:error::asn1 encoding routines:asn1_check_tlen:wrong tag:crypto/asn1/tasn_dec.c:1135:

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/11789)

crypto/err/err_prn.c
crypto/o_str.c
include/internal/cryptlib.h
test/build.info
test/errtest.c
test/hexstr_test.c [new file with mode: 0644]
test/recipes/04-test_hexstring.t [new file with mode: 0644]

index 5c4ebcb..80cc0ec 100644 (file)
@@ -35,8 +35,9 @@ void ERR_print_errors_cb(int (*cb) (const char *str, size_t len, void *u),
             func = "unknown function";
         if ((flags & ERR_TXT_STRING) == 0)
             data = "";
-        hex = OPENSSL_buf2hexstr((const unsigned char *)&tid, sizeof(tid));
-        BIO_snprintf(buf, sizeof(buf), "%s:error:%s:%s:%s:%s:%d:%s\n",
+        hex = openssl_buf2hexstr_sep((const unsigned char *)&tid, sizeof(tid),
+                                     '\0');
+        BIO_snprintf(buf, sizeof(buf), "%s:error::%s:%s:%s:%s:%d:%s\n",
                      hex == NULL ? "<null>" : hex, lib, func, reason, file,
                      line, data);
         OPENSSL_free(hex);
index 6780188..6578857 100644 (file)
@@ -12,6 +12,9 @@
 #include <openssl/crypto.h>
 #include "internal/cryptlib.h"
 
+#define DEFAULT_SEPARATOR ':'
+#define CH_ZERO '\0'
+
 char *CRYPTO_strdup(const char *str, const char* file, int line)
 {
     char *ret;
@@ -37,7 +40,7 @@ char *CRYPTO_strndup(const char *str, size_t s, const char* file, int line)
     ret = CRYPTO_malloc(maxlen + 1, file, line);
     if (ret) {
         memcpy(ret, str, maxlen);
-        ret[maxlen] = '\0';
+        ret[maxlen] = CH_ZERO;
     }
     return ret;
 }
@@ -61,7 +64,7 @@ size_t OPENSSL_strnlen(const char *str, size_t maxlen)
 {
     const char *p;
 
-    for (p = str; maxlen-- != 0 && *p != '\0'; ++p) ;
+    for (p = str; maxlen-- != 0 && *p != CH_ZERO; ++p) ;
 
     return p - str;
 }
@@ -74,7 +77,7 @@ size_t OPENSSL_strlcpy(char *dst, const char *src, size_t size)
         l++;
     }
     if (size)
-        *dst = '\0';
+        *dst = CH_ZERO;
     return l + strlen(src);
 }
 
@@ -129,11 +132,8 @@ int OPENSSL_hexchar2int(unsigned char c)
     return -1;
 }
 
-/*
- * Give a string of hex digits convert to a buffer
- */
-int OPENSSL_hexstr2buf_ex(unsigned char *buf, size_t buf_n, size_t *buflen,
-                          const char *str)
+static int hexstr2buf_sep(unsigned char *buf, size_t buf_n, size_t *buflen,
+                          const char *str, const char sep)
 {
     unsigned char *q;
     unsigned char ch, cl;
@@ -143,26 +143,24 @@ int OPENSSL_hexstr2buf_ex(unsigned char *buf, size_t buf_n, size_t *buflen,
 
     for (p = (const unsigned char *)str, q = buf, cnt = 0; *p; ) {
         ch = *p++;
-        if (ch == ':')
+        /* A separator of CH_ZERO means there is no separator */
+        if (ch == sep && sep != CH_ZERO)
             continue;
         cl = *p++;
         if (!cl) {
-            CRYPTOerr(CRYPTO_F_OPENSSL_HEXSTR2BUF_EX,
-                      CRYPTO_R_ODD_NUMBER_OF_DIGITS);
+            CRYPTOerr(0, CRYPTO_R_ODD_NUMBER_OF_DIGITS);
             return 0;
         }
         cli = OPENSSL_hexchar2int(cl);
         chi = OPENSSL_hexchar2int(ch);
         if (cli < 0 || chi < 0) {
-            CRYPTOerr(CRYPTO_F_OPENSSL_HEXSTR2BUF_EX,
-                      CRYPTO_R_ILLEGAL_HEX_DIGIT);
+            CRYPTOerr(0, CRYPTO_R_ILLEGAL_HEX_DIGIT);
             return 0;
         }
         cnt++;
         if (q != NULL) {
             if (cnt > buf_n) {
-                CRYPTOerr(CRYPTO_F_OPENSSL_HEXSTR2BUF_EX,
-                          CRYPTO_R_TOO_SMALL_BUFFER);
+                CRYPTOerr(0, CRYPTO_R_TOO_SMALL_BUFFER);
                 return 0;
             }
             *q++ = (unsigned char)((chi << 4) | cli);
@@ -174,21 +172,31 @@ int OPENSSL_hexstr2buf_ex(unsigned char *buf, size_t buf_n, size_t *buflen,
     return 1;
 }
 
-unsigned char *OPENSSL_hexstr2buf(const char *str, long *buflen)
+/*
+ * Given a string of hex digits convert to a buffer
+ */
+int OPENSSL_hexstr2buf_ex(unsigned char *buf, size_t buf_n, size_t *buflen,
+                          const char *str)
+{
+    return hexstr2buf_sep(buf, buf_n, buflen, str, DEFAULT_SEPARATOR);
+}
+
+unsigned char *openssl_hexstr2buf_sep(const char *str, long *buflen,
+                                      const char sep)
 {
     unsigned char *buf;
     size_t buf_n, tmp_buflen;
 
     buf_n = strlen(str) >> 1;
     if ((buf = OPENSSL_malloc(buf_n)) == NULL) {
-        CRYPTOerr(CRYPTO_F_OPENSSL_HEXSTR2BUF, ERR_R_MALLOC_FAILURE);
+        CRYPTOerr(0, ERR_R_MALLOC_FAILURE);
         return NULL;
     }
 
     if (buflen != NULL)
         *buflen = 0;
     tmp_buflen = 0;
-    if (OPENSSL_hexstr2buf_ex(buf, buf_n, &tmp_buflen, str)) {
+    if (hexstr2buf_sep(buf, buf_n, &tmp_buflen, str, sep)) {
         if (buflen != NULL)
             *buflen = (long)tmp_buflen;
         return buf;
@@ -197,21 +205,29 @@ unsigned char *OPENSSL_hexstr2buf(const char *str, long *buflen)
     return NULL;
 }
 
-int OPENSSL_buf2hexstr_ex(char *str, size_t str_n, size_t *strlen,
-                          const unsigned char *buf, size_t buflen)
+unsigned char *OPENSSL_hexstr2buf(const char *str, long *buflen)
+{
+    return openssl_hexstr2buf_sep(str, buflen, DEFAULT_SEPARATOR);
+}
+
+static int buf2hexstr_sep(char *str, size_t str_n, size_t *strlen,
+                          const unsigned char *buf, size_t buflen,
+                          const char sep)
 {
     static const char hexdig[] = "0123456789ABCDEF";
     const unsigned char *p;
     char *q;
     size_t i;
+    int has_sep = (sep != CH_ZERO);
+    size_t len = has_sep ? buflen * 3 : 1 + buflen * 2;
 
     if (strlen != NULL)
-        *strlen = buflen * 3;
+        *strlen = len;
     if (str == NULL)
         return 1;
 
-    if (str_n < (unsigned long)buflen * 3) {
-        CRYPTOerr(CRYPTO_F_OPENSSL_BUF2HEXSTR_EX, CRYPTO_R_TOO_SMALL_BUFFER);
+    if (str_n < (unsigned long)len) {
+        CRYPTOerr(0, CRYPTO_R_TOO_SMALL_BUFFER);
         return 0;
     }
 
@@ -219,21 +235,26 @@ int OPENSSL_buf2hexstr_ex(char *str, size_t str_n, size_t *strlen,
     for (i = 0, p = buf; i < buflen; i++, p++) {
         *q++ = hexdig[(*p >> 4) & 0xf];
         *q++ = hexdig[*p & 0xf];
-        *q++ = ':';
+        if (has_sep)
+            *q++ = sep;
     }
-    q[-1] = 0;
+    if (has_sep)
+        --q;
+    *q = CH_ZERO;
+
 #ifdef CHARSET_EBCDIC
     ebcdic2ascii(str, str, q - str - 1);
 #endif
     return 1;
 }
 
-/*
- * Given a buffer of length 'len' return a OPENSSL_malloc'ed string with its
- * hex representation @@@ (Contents of buffer are always kept in ASCII, also
- * on EBCDIC machines)
- */
-char *OPENSSL_buf2hexstr(const unsigned char *buf, long buflen)
+int OPENSSL_buf2hexstr_ex(char *str, size_t str_n, size_t *strlen,
+                          const unsigned char *buf, size_t buflen)
+{
+    return buf2hexstr_sep(str, str_n, strlen, buf, buflen, DEFAULT_SEPARATOR);
+}
+
+char *openssl_buf2hexstr_sep(const unsigned char *buf, long buflen, char sep)
 {
     char *tmp;
     size_t tmp_n;
@@ -241,18 +262,29 @@ char *OPENSSL_buf2hexstr(const unsigned char *buf, long buflen)
     if (buflen == 0)
         return OPENSSL_zalloc(1);
 
-    tmp_n = buflen * 3;
+    tmp_n = (sep != CH_ZERO) ? buflen * 3 : 1 + buflen * 2;
     if ((tmp = OPENSSL_malloc(tmp_n)) == NULL) {
-        CRYPTOerr(CRYPTO_F_OPENSSL_BUF2HEXSTR, ERR_R_MALLOC_FAILURE);
+        CRYPTOerr(0, ERR_R_MALLOC_FAILURE);
         return NULL;
     }
 
-    if (OPENSSL_buf2hexstr_ex(tmp, tmp_n, NULL, buf, buflen))
+    if (buf2hexstr_sep(tmp, tmp_n, NULL, buf, buflen, sep))
         return tmp;
     OPENSSL_free(tmp);
     return NULL;
 }
 
+
+/*
+ * Given a buffer of length 'len' return a OPENSSL_malloc'ed string with its
+ * hex representation @@@ (Contents of buffer are always kept in ASCII, also
+ * on EBCDIC machines)
+ */
+char *OPENSSL_buf2hexstr(const unsigned char *buf, long buflen)
+{
+    return openssl_buf2hexstr_sep(buf, buflen, ':');
+}
+
 int openssl_strerror_r(int errnum, char *buf, size_t buflen)
 {
 #if defined(_MSC_VER) && _MSC_VER>=1400
index b479b58..a4f18a5 100644 (file)
@@ -241,4 +241,8 @@ char *sk_ASN1_UTF8STRING2text(STACK_OF(ASN1_UTF8STRING) *text, const char *sep,
                               size_t max_len);
 char *ipaddr_to_asc(unsigned char *p, int len);
 
+char *openssl_buf2hexstr_sep(const unsigned char *buf, long buflen, char sep);
+unsigned char *openssl_hexstr2buf_sep(const char *str, long *buflen,
+                                      const char sep);
+
 #endif
index 112b68c..9697e55 100644 (file)
@@ -54,7 +54,7 @@ IF[{- !$disabled{tests} -}]
           http_test servername_test ocspapitest fatalerrtest tls13ccstest \
           sysdefaulttest errtest ssl_ctx_test gosttest \
           context_internal_test aesgcmtest params_test evp_pkey_dparams_test \
-          keymgmt_internal_test
+          keymgmt_internal_test hexstr_test
 
   SOURCE[confdump]=confdump.c
   INCLUDE[confdump]=../include ../apps/include
@@ -736,6 +736,11 @@ IF[{- !$disabled{tests} -}]
   INCLUDE[params_test]=.. ../include ../apps/include
   DEPEND[params_test]=../libcrypto.a libtestutil.a
 
+  PROGRAMS{noinst}=hexstr_test
+  SOURCE[hexstr_test]=hexstr_test.c
+  INCLUDE[hexstr_test]=.. ../include ../apps/include
+  DEPEND[hexstr_test]=../libcrypto.a libtestutil.a
+
   PROGRAMS{noinst}=namemap_internal_test
   SOURCE[namemap_internal_test]=namemap_internal_test.c
   INCLUDE[namemap_internal_test]=.. ../include ../apps/include
index 179c338..cc2f661 100644 (file)
@@ -7,6 +7,7 @@
  * https://www.openssl.org/source/license.html
  */
 
+#include <string.h>
 #include <openssl/opensslconf.h>
 #include <openssl/err.h>
 
 # include <errno.h>
 #endif
 
+#ifndef OPENSSL_NO_DEPRECATED_3_0
+# define IS_HEX(ch) ((ch >= '0' && ch <='9') || (ch >= 'A' && ch <='F'))
+
+static int test_print_error_format(void)
+{
+    static const char expected[] =
+        ":error::system library:test_print_error_format:Operation not permitted:"
+# ifndef OPENSSL_NO_FILENAMES
+        "errtest.c:30:";
+# else
+        ":0:";
+# endif
+    char *out = NULL, *p = NULL;
+    int ret = 0, len;
+    BIO *bio = NULL;
+
+    if (!TEST_ptr(bio = BIO_new(BIO_s_mem())))
+        return 0;
+
+    ERR_PUT_error(ERR_LIB_SYS, 0, 1, "errtest.c", 30);
+    ERR_print_errors(bio);
+
+    if (!TEST_int_gt(len = BIO_get_mem_data(bio, &out), 0))
+        goto err;
+    /* Skip over the variable thread id at the start of the string */
+    for (p = out; *p != ':' && *p != 0; ++p) {
+        if (!TEST_true(IS_HEX(*p)))
+            goto err;
+    }
+    if (!TEST_true(*p != 0)
+        || !TEST_strn_eq(expected, p, strlen(expected)))
+        goto err;
+
+    ret = 1;
+err:
+    BIO_free(bio);
+    return ret;
+}
+#endif
+
 /* Test that querying the error queue preserves the OS error. */
 static int preserves_system_error(void)
 {
@@ -79,5 +120,8 @@ int setup_tests(void)
     ADD_TEST(preserves_system_error);
     ADD_TEST(vdata_appends);
     ADD_TEST(raised_error);
+#ifndef OPENSSL_NO_DEPRECATED_3_0
+    ADD_TEST(test_print_error_format);
+#endif
     return 1;
 }
diff --git a/test/hexstr_test.c b/test/hexstr_test.c
new file mode 100644 (file)
index 0000000..c4f13b6
--- /dev/null
@@ -0,0 +1,133 @@
+/*
+ * Copyright 2020 The OpenSSL Project Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License 2.0 (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.
+ */
+
+/*
+ * This program tests the use of OSSL_PARAM, currently in raw form.
+ */
+
+#include "internal/nelem.h"
+#include "internal/cryptlib.h"
+#include "testutil.h"
+
+struct testdata
+{
+    const char *in;
+    const unsigned char *expected;
+    size_t expected_len;
+    const char sep;
+};
+
+static const unsigned char test_1[] = { 0xAB, 0xCD, 0xEF, 0xF1 };
+static const unsigned char test_2[] = { 0xAB, 0xCD, 0xEF, 0x76, 0x00 };
+
+static struct testdata tbl_testdata[] = {
+    {
+        "AB:CD:EF:F1",
+        test_1, sizeof(test_1),
+        ':',
+    },
+    {
+        "AB:CD:EF:76:00",
+        test_2, sizeof(test_2),
+        ':',
+    },
+    {
+        "AB_CD_EF_F1",
+        test_1, sizeof(test_1),
+        '_',
+    },
+    {
+        "AB_CD_EF_76_00",
+        test_2, sizeof(test_2),
+        '_',
+    },
+    {
+        "ABCDEFF1",
+        test_1, sizeof(test_1),
+        '\0',
+    },
+    {
+        "ABCDEF7600",
+        test_2, sizeof(test_2),
+        '\0',
+    },
+};
+
+static int test_hexstr_sep_to_from(int test_index)
+{
+    int ret = 0;
+    long len = 0;
+    unsigned char *buf = NULL;
+    char *out = NULL;
+    struct testdata *test = &tbl_testdata[test_index];
+
+    if (!TEST_ptr(buf = openssl_hexstr2buf_sep(test->in, &len, test->sep))
+        || !TEST_mem_eq(buf, len, test->expected, test->expected_len)
+        || !TEST_ptr(out = openssl_buf2hexstr_sep(buf, len, test->sep))
+        || !TEST_str_eq(out, test->in))
+       goto err;
+
+    ret = 1;
+err:
+    OPENSSL_free(buf);
+    OPENSSL_free(out);
+    return ret;
+}
+
+static int test_hexstr_to_from(int test_index)
+{
+    int ret = 0;
+    long len = 0;
+    unsigned char *buf = NULL;
+    char *out = NULL;
+    struct testdata *test = &tbl_testdata[test_index];
+
+    if (test->sep != '_') {
+        if (!TEST_ptr(buf = OPENSSL_hexstr2buf(test->in, &len))
+            || !TEST_mem_eq(buf, len, test->expected, test->expected_len)
+            || !TEST_ptr(out = OPENSSL_buf2hexstr(buf, len)))
+           goto err;
+        if (test->sep == ':') {
+            if (!TEST_str_eq(out, test->in))
+                goto err;
+        } else if (!TEST_str_ne(out, test->in)) {
+            goto err;
+        }
+    } else {
+        if (!TEST_ptr_null(buf = OPENSSL_hexstr2buf(test->in, &len)))
+            goto err;
+    }
+    ret = 1;
+err:
+    OPENSSL_free(buf);
+    OPENSSL_free(out);
+    return ret;
+}
+
+static int test_hexstr_ex_to_from(int test_index)
+{
+    size_t len = 0;
+    char out[64];
+    unsigned char buf[64];
+    struct testdata *test = &tbl_testdata[test_index];
+
+    return TEST_true(OPENSSL_hexstr2buf_ex(buf, sizeof(buf), &len, test->in))
+           && TEST_mem_eq(buf, len, test->expected, test->expected_len)
+           && TEST_true(OPENSSL_buf2hexstr_ex(out, sizeof(out), NULL, buf, len))
+           && TEST_str_eq(out, test->in);
+}
+
+int setup_tests(void)
+{
+    ADD_ALL_TESTS(test_hexstr_sep_to_from, OSSL_NELEM(tbl_testdata));
+    ADD_ALL_TESTS(test_hexstr_to_from, OSSL_NELEM(tbl_testdata));
+    ADD_ALL_TESTS(test_hexstr_ex_to_from, 2);
+    return 1;
+}
diff --git a/test/recipes/04-test_hexstring.t b/test/recipes/04-test_hexstring.t
new file mode 100644 (file)
index 0000000..664868f
--- /dev/null
@@ -0,0 +1,15 @@
+#! /usr/bin/env perl
+# Copyright 2020 The OpenSSL Project Authors. All Rights Reserved.
+#
+# Licensed under the Apache License 2.0 (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 OpenSSL::Test;
+use OpenSSL::Test::Simple;
+
+setup("test_hexstring");
+
+simple_test("test_hexstring", "hexstr_test");