diff --git a/crypto/ec/ec_asn1.c b/crypto/ec/ec_asn1.c index c7ed01ef..33c4c230 100644 --- a/crypto/ec/ec_asn1.c +++ b/crypto/ec/ec_asn1.c @@ -367,10 +367,8 @@ static int ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve) { int ok = 0, nid; BIGNUM *tmp_1 = NULL, *tmp_2 = NULL; - unsigned char *buffer_1 = NULL, *buffer_2 = NULL, - *a_buf = NULL, *b_buf = NULL; - size_t len_1, len_2; - unsigned char char_zero = 0; + unsigned char *a_buf = NULL, *b_buf = NULL; + size_t len; if (!group || !curve || !curve->a || !curve->b) return 0; @@ -398,44 +396,26 @@ static int ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve) } } #endif - len_1 = (size_t)BN_num_bytes(tmp_1); - len_2 = (size_t)BN_num_bytes(tmp_2); - - if (len_1 == 0) { - /* len_1 == 0 => a == 0 */ - a_buf = &char_zero; - len_1 = 1; - } else { - if ((buffer_1 = OPENSSL_malloc(len_1)) == NULL) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_MALLOC_FAILURE); - goto err; - } - if ((len_1 = BN_bn2bin(tmp_1, buffer_1)) == 0) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_BN_LIB); - goto err; - } - a_buf = buffer_1; + /* + * Per SEC 1, the curve coefficients must be padded up to size. See C.2's + * definition of Curve, C.1's definition of FieldElement, and 2.3.5's + * definition of how to encode the field elements. + */ + len = ((size_t)EC_GROUP_get_degree(group) + 7) / 8; + if ((a_buf = OPENSSL_malloc(len)) == NULL + || (b_buf = OPENSSL_malloc(len)) == NULL) { + ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_MALLOC_FAILURE); + goto err; } - - if (len_2 == 0) { - /* len_2 == 0 => b == 0 */ - b_buf = &char_zero; - len_2 = 1; - } else { - if ((buffer_2 = OPENSSL_malloc(len_2)) == NULL) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_MALLOC_FAILURE); - goto err; - } - if ((len_2 = BN_bn2bin(tmp_2, buffer_2)) == 0) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_BN_LIB); - goto err; - } - b_buf = buffer_2; + if (BN_bn2binpad(tmp_1, a_buf, len) < 0 + || BN_bn2binpad(tmp_2, b_buf, len) < 0) { + ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_BN_LIB); + goto err; } /* set a and b */ - if (!ASN1_OCTET_STRING_set(curve->a, a_buf, len_1) || - !ASN1_OCTET_STRING_set(curve->b, b_buf, len_2)) { + if (!ASN1_OCTET_STRING_set(curve->a, a_buf, len) + || !ASN1_OCTET_STRING_set(curve->b, b_buf, len)) { ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_ASN1_LIB); goto err; } @@ -462,8 +442,8 @@ static int ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve) ok = 1; err: - OPENSSL_free(buffer_1); - OPENSSL_free(buffer_2); + OPENSSL_free(a_buf); + OPENSSL_free(b_buf); BN_free(tmp_1); BN_free(tmp_2); return ok; @@ -611,7 +591,12 @@ EC_GROUP *EC_GROUP_new_from_ecparameters(const ECPARAMETERS *params) goto err; } - /* now extract the curve parameters a and b */ + /* + * Now extract the curve parameters a and b. Note that, although SEC 1 + * specifies the length of their encodings, historical versions of OpenSSL + * encoded them incorrectly, so we must accept any length for backwards + * compatibility. + */ if (!params->curve || !params->curve->a || !params->curve->a->data || !params->curve->b || !params->curve->b->data) { @@ -856,7 +841,7 @@ EC_GROUP *EC_GROUP_new_from_ecpkparameters(const ECPKPARAMETERS *params) ECerr(EC_F_EC_GROUP_NEW_FROM_ECPKPARAMETERS, ERR_R_EC_LIB); return NULL; } - EC_GROUP_set_asn1_flag(ret, 0x0); + EC_GROUP_set_asn1_flag(ret, OPENSSL_EC_EXPLICIT_CURVE); } else if (params->type == 2) { /* implicitlyCA */ return NULL; } else { diff --git a/crypto/threads_win.c b/crypto/threads_win.c index 1e5cf820..7fdbc1f6 100644 --- a/crypto/threads_win.c +++ b/crypto/threads_win.c @@ -101,7 +101,26 @@ int CRYPTO_THREAD_init_local(CRYPTO_THREAD_LOCAL *key, void (*cleanup)(void *)) void *CRYPTO_THREAD_get_local(CRYPTO_THREAD_LOCAL *key) { - return TlsGetValue(*key); + DWORD last_error; + void *ret; + + /* + * TlsGetValue clears the last error even on success, so that callers may + * distinguish it successfully returning NULL or failing. It is documented + * to never fail if the argument is a valid index from TlsAlloc, so we do + * not need to handle this. + * + * However, this error-mangling behavior interferes with the caller's use of + * GetLastError. In particular SSL_get_error queries the error queue to + * determine whether the caller should look at the OS's errors. To avoid + * destroying state, save and restore the Windows error. + * + * https://msdn.microsoft.com/en-us/library/windows/desktop/ms686812(v=vs.85).aspx + */ + last_error = GetLastError(); + ret = TlsGetValue(*key); + SetLastError(last_error); + return ret; } int CRYPTO_THREAD_set_local(CRYPTO_THREAD_LOCAL *key, void *val) diff --git a/test/build.info b/test/build.info index c3a09048..000153d5 100644 --- a/test/build.info +++ b/test/build.info @@ -51,7 +51,7 @@ INCLUDE_MAIN___test_libtestutil_OLB = /INCLUDE=MAIN recordlentest drbgtest drbg_cavs_test sslbuffertest \ time_offset_test pemtest ssl_cert_table_internal_test ciphername_test \ servername_test ocspapitest rsa_mp_test fatalerrtest tls13ccstest \ - sysdefaulttest + sysdefaulttest errtest SOURCE[versions]=versions.c INCLUDE[versions]=../include @@ -535,6 +535,10 @@ INCLUDE_MAIN___test_libtestutil_OLB = /INCLUDE=MAIN SOURCE[sysdefaulttest]=sysdefaulttest.c INCLUDE[sysdefaulttest]=../include DEPEND[sysdefaulttest]=../libcrypto ../libssl libtestutil.a + + SOURCE[errtest]=errtest.c + INCLUDE[errtest]=../include + DEPEND[errtest]=../libcrypto libtestutil.a ENDIF {- diff --git a/test/ectest.c b/test/ectest.c index 1c31cce8..73e8aa87 100644 --- a/test/ectest.c +++ b/test/ectest.c @@ -1407,20 +1407,91 @@ err: } # endif +static const unsigned char p521_named[] = { + 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x23, +}; + +static const unsigned char p521_explicit[] = { + 0x30, 0x82, 0x01, 0xc3, 0x02, 0x01, 0x01, 0x30, 0x4d, 0x06, 0x07, 0x2a, + 0x86, 0x48, 0xce, 0x3d, 0x01, 0x01, 0x02, 0x42, 0x01, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0x30, 0x81, 0x9f, 0x04, 0x42, 0x01, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xfc, 0x04, 0x42, 0x00, 0x51, 0x95, 0x3e, 0xb9, 0x61, 0x8e, 0x1c, 0x9a, + 0x1f, 0x92, 0x9a, 0x21, 0xa0, 0xb6, 0x85, 0x40, 0xee, 0xa2, 0xda, 0x72, + 0x5b, 0x99, 0xb3, 0x15, 0xf3, 0xb8, 0xb4, 0x89, 0x91, 0x8e, 0xf1, 0x09, + 0xe1, 0x56, 0x19, 0x39, 0x51, 0xec, 0x7e, 0x93, 0x7b, 0x16, 0x52, 0xc0, + 0xbd, 0x3b, 0xb1, 0xbf, 0x07, 0x35, 0x73, 0xdf, 0x88, 0x3d, 0x2c, 0x34, + 0xf1, 0xef, 0x45, 0x1f, 0xd4, 0x6b, 0x50, 0x3f, 0x00, 0x03, 0x15, 0x00, + 0xd0, 0x9e, 0x88, 0x00, 0x29, 0x1c, 0xb8, 0x53, 0x96, 0xcc, 0x67, 0x17, + 0x39, 0x32, 0x84, 0xaa, 0xa0, 0xda, 0x64, 0xba, 0x04, 0x81, 0x85, 0x04, + 0x00, 0xc6, 0x85, 0x8e, 0x06, 0xb7, 0x04, 0x04, 0xe9, 0xcd, 0x9e, 0x3e, + 0xcb, 0x66, 0x23, 0x95, 0xb4, 0x42, 0x9c, 0x64, 0x81, 0x39, 0x05, 0x3f, + 0xb5, 0x21, 0xf8, 0x28, 0xaf, 0x60, 0x6b, 0x4d, 0x3d, 0xba, 0xa1, 0x4b, + 0x5e, 0x77, 0xef, 0xe7, 0x59, 0x28, 0xfe, 0x1d, 0xc1, 0x27, 0xa2, 0xff, + 0xa8, 0xde, 0x33, 0x48, 0xb3, 0xc1, 0x85, 0x6a, 0x42, 0x9b, 0xf9, 0x7e, + 0x7e, 0x31, 0xc2, 0xe5, 0xbd, 0x66, 0x01, 0x18, 0x39, 0x29, 0x6a, 0x78, + 0x9a, 0x3b, 0xc0, 0x04, 0x5c, 0x8a, 0x5f, 0xb4, 0x2c, 0x7d, 0x1b, 0xd9, + 0x98, 0xf5, 0x44, 0x49, 0x57, 0x9b, 0x44, 0x68, 0x17, 0xaf, 0xbd, 0x17, + 0x27, 0x3e, 0x66, 0x2c, 0x97, 0xee, 0x72, 0x99, 0x5e, 0xf4, 0x26, 0x40, + 0xc5, 0x50, 0xb9, 0x01, 0x3f, 0xad, 0x07, 0x61, 0x35, 0x3c, 0x70, 0x86, + 0xa2, 0x72, 0xc2, 0x40, 0x88, 0xbe, 0x94, 0x76, 0x9f, 0xd1, 0x66, 0x50, + 0x02, 0x42, 0x01, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfa, + 0x51, 0x86, 0x87, 0x83, 0xbf, 0x2f, 0x96, 0x6b, 0x7f, 0xcc, 0x01, 0x48, + 0xf7, 0x09, 0xa5, 0xd0, 0x3b, 0xb5, 0xc9, 0xb8, 0x89, 0x9c, 0x47, 0xae, + 0xbb, 0x6f, 0xb7, 0x1e, 0x91, 0x38, 0x64, 0x09, 0x02, 0x01, 0x01, +}; + static int parameter_test(void) { EC_GROUP *group = NULL, *group2 = NULL; ECPARAMETERS *ecparameters = NULL; - int r; + unsigned char *buf = NULL; + int r = 0, len; - r = TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp112r1)) - && TEST_ptr(ecparameters = EC_GROUP_get_ecparameters(group, NULL)) - && TEST_ptr(group2 = EC_GROUP_new_from_ecparameters(ecparameters)) - && TEST_int_eq(EC_GROUP_cmp(group, group2, NULL), 0); + if (!TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp112r1)) + || !TEST_ptr(ecparameters = EC_GROUP_get_ecparameters(group, NULL)) + || !TEST_ptr(group2 = EC_GROUP_new_from_ecparameters(ecparameters)) + || !TEST_int_eq(EC_GROUP_cmp(group, group2, NULL), 0)) + goto err; + EC_GROUP_free(group); + group = NULL; + + /* Test the named curve encoding, which should be default. */ + if (!TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp521r1)) + || !TEST_true((len = i2d_ECPKParameters(group, &buf)) >= 0) + || !TEST_mem_eq(buf, len, p521_named, sizeof(p521_named))) + goto err; + + OPENSSL_free(buf); + buf = NULL; + + /* + * Test the explicit encoding. P-521 requires correctly zero-padding the + * curve coefficients. + */ + EC_GROUP_set_asn1_flag(group, OPENSSL_EC_EXPLICIT_CURVE); + if (!TEST_true((len = i2d_ECPKParameters(group, &buf)) >= 0) + || !TEST_mem_eq(buf, len, p521_explicit, sizeof(p521_explicit))) + goto err; + + r = 1; +err: EC_GROUP_free(group); EC_GROUP_free(group2); ECPARAMETERS_free(ecparameters); + OPENSSL_free(buf); return r; } #endif diff --git a/test/errtest.c b/test/errtest.c new file mode 100644 index 00000000..e464d08b --- /dev/null +++ b/test/errtest.c @@ -0,0 +1,39 @@ +/* + * Copyright 2018 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 + */ + +#include +#include + +#include "testutil.h" + +#if defined(OPENSSL_SYS_WINDOWS) +# include +#else +# include +#endif + +/* Test that querying the error queue preserves the OS error. */ +static int preserves_system_error(void) +{ +#if defined(OPENSSL_SYS_WINDOWS) + SetLastError(ERROR_INVALID_FUNCTION); + ERR_get_error(); + return TEST_int_eq(GetLastError(), ERROR_INVALID_FUNCTION); +#else + errno = EINVAL; + ERR_get_error(); + return TEST_int_eq(errno, EINVAL); +#endif +} + +int setup_tests(void) +{ + ADD_TEST(preserves_system_error); + return 1; +} diff --git a/test/recipes/04-test_err.t b/test/recipes/04-test_err.t new file mode 100644 index 00000000..dd7681af --- /dev/null +++ b/test/recipes/04-test_err.t @@ -0,0 +1,12 @@ +#! /usr/bin/env perl +# Copyright 2018 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 OpenSSL::Test::Simple; + +simple_test("test_err", "errtest");