From 881f7157f648eb8a39e5dfd3efb95951ee7ac215 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 May 2018 11:39:42 -0400 Subject: [PATCH 1/2] Return -1 from our PEM password callback Apparently, contrary to its documentation, this is how OpenSSL now wants us to report an error. Fixes bug 26116; bugfix on 0.2.5.16. --- changes/bug26116 | 7 +++++++ src/common/crypto.c | 7 ++++++- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changes/bug26116 diff --git a/changes/bug26116 b/changes/bug26116 new file mode 100644 index 0000000000..3bfde74f77 --- /dev/null +++ b/changes/bug26116 @@ -0,0 +1,7 @@ + o Minor bugfixes (compatibility, openssl): + - Work around a change in OpenSSL 1.1.1 where + return values that would previously indicate "no password" now + indicate an empty password. Without this workaround, Tor instances + running with OpenSSL 1.1.1 would accept descriptors that other Tor + instances would reject. Fixes bug 26116; bugfix on 0.2.5.16. + diff --git a/src/common/crypto.c b/src/common/crypto.c index 39c8cc2b0a..f8495bb107 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -653,7 +653,12 @@ pem_no_password_cb(char *buf, int size, int rwflag, void *u) (void)size; (void)rwflag; (void)u; - return 0; + /* The openssl documentation says that a callback "must" return 0 if an + * error occurred. But during the 1.1.1 series (commit c82c3462267afdbbaa5 + * they changed the interpretation so that 0 indicates an empty password and + * -1 indicates an error. We want to reject any encrypted PEM buffers, so we + * return -1. This will work on older OpenSSL versions and LibreSSL too. */ + return -1; } /** Read a PEM-encoded private key from the len-byte string s From aeb4be1d5a17f8ff836e370f8942c09c66b31e1d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 24 May 2018 09:36:33 -0400 Subject: [PATCH 2/2] Add a unit test for PEM-encrypted documents. --- src/test/test_crypto.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 64a46f7914..8fd9ca7671 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -1349,6 +1349,46 @@ test_crypto_pk_base64(void *arg) tor_free(encoded); } +static void +test_crypto_pk_pem_encrypted(void *arg) +{ + crypto_pk_t *pk = NULL; + (void)arg; + + pk = crypto_pk_new(); + /* we need to make sure that we won't stall if somebody gives us a key + that's encrypted with a password. */ + { + const char *s = + "-----BEGIN RSA PRIVATE KEY-----\n" + "Proc-Type: 4,ENCRYPTED\n" + "DEK-Info: AES-128-CBC,EFA86BB9D2AB11E80B4E3DCD97782B16\n" + "\n" + "Z2Je4m0cFepc6coQkVbGcvNCHxTf941N2XYEVE6kn0CqWqoUH4tlwV6for5D91np\n" + "5NiEFTkWj31EhrvrYcuiJtQ/iEbABxZULFWFeJ058rb+1izBz5rScqnEacIS/3Go\n" + "YntnROBDwiKmUnue6PJVYg==\n" + "-----END RSA PRIVATE KEY-----\n"; + tt_int_op(-1, OP_EQ, + crypto_pk_read_private_key_from_string(pk, s, strlen(s))); + } + /* For fun, make sure we aren't hit by OpenSSL issue + https://github.com/openssl/openssl/issues/6347 , where we get in trouble + if a cipher doesn't use an IV. + */ + { + const char *s = + "-----BEGIN RSA PUBLIC KEY-----\n" + "Proc-Type:4,ENCRYPTED\n" + "DEK-Info:des-ede -\n" + "\n" + "iRqK\n" + "-----END RSA PUBLIC KEY-----\n"; + tt_int_op(-1, OP_EQ, + crypto_pk_read_public_key_from_string(pk, s, strlen(s))); + } + done: + crypto_pk_free(pk); +} #ifdef HAVE_TRUNCATE #define do_truncate truncate #else @@ -2914,6 +2954,7 @@ struct testcase_t crypto_tests[] = { CRYPTO_LEGACY(pk), { "pk_fingerprints", test_crypto_pk_fingerprints, TT_FORK, NULL, NULL }, { "pk_base64", test_crypto_pk_base64, TT_FORK, NULL, NULL }, + { "pk_pem_encrypted", test_crypto_pk_pem_encrypted, TT_FORK, NULL, NULL }, CRYPTO_LEGACY(digests), { "digest_names", test_crypto_digest_names, 0, NULL, NULL }, { "sha3", test_crypto_sha3, TT_FORK, NULL, NULL},