From patchwork Fri Dec 1 23:08:21 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Benjamin X-Patchwork-Id: 843780 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133; helo=bombadil.infradead.org; envelope-from=hostap-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Xe3h8HxF"; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="wPy+kGKa"; dkim-atps=neutral Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3ypVMt58F3z9sNV for ; Sat, 2 Dec 2017 10:09:30 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:To:Subject: Message-ID:Date:From:MIME-Version:Reply-To:Cc:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References:List-Owner; bh=jylPVOgv5EfmfoD/ecMLdqU97uvpsXB4qUmaimL2hx8=; b=Xe3h8HxFiW0xXR5kx7iyomLeGG HHrMfkbay+lqJBiHXlW66yWdychi26mgLTDNkuU0b9RmRpzfQyBU9nuxx26ssL9CTCICpdOO63ZGI cNeMXxXzF12iLooLxdrK+n5e62MxCXhC60TGmtm7Owj+2bFHy2bCF6VaniCSnevFU5tkMvQOGTYoW itrCxP2omjpthmcEUXEvIj90bYqsK5NlpywGyNRVUCQdl/paUNiCtM5p7rAOgDVymkuYhEZF/xD2E d5cvEh/1XXEBVPiG/0fsFeegUrqIL++BxFmcRTJq1I4HAN8uBD0awS8cCj0sSrfmEiF5oi9tkz2c/ 0XcgvWpw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1eKuQe-0002d2-01; Fri, 01 Dec 2017 23:09:08 +0000 Received: from mail-qt0-x22a.google.com ([2607:f8b0:400d:c0d::22a]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1eKuQa-0002c8-Fj for hostap@lists.infradead.org; Fri, 01 Dec 2017 23:09:07 +0000 Received: by mail-qt0-x22a.google.com with SMTP id b10so7304172qti.11 for ; Fri, 01 Dec 2017 15:08:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:from:date:message-id:subject:to; bh=1bZNbTEXiC92Vg4f5eZkR+ahHWryEO6o9gySjAM7Bls=; b=wPy+kGKaXH4tIrHBZSlKSh9sfWQVQv+W8iibydjtW5prb2sIiaxW5mf4ckUKoVYjvH bxsmLgdprhMhI5kuSXhJPNY2Gy212wTc/QHT251jcbDT6Le45d+DZg/KIQYdD0/e/jXr OtCAaODORY+5aOLmd1UUMEGJcToqusXXSjrwJeKlg8CUVApL+0g1AMQFQ/qbpgdFsIx6 cLRkBggJpaXNhwplwWNkl62DvFRhC2MZTXKBpMNuYth0wYaBEtgeibszyZ4tiYYzmgzk fd3mmyV8o/0+RLWpg6Fc62xD2CQsrIOPW8tNAwltKEaYPlOdD83SMHDcgPfrygt31V60 ODEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=1bZNbTEXiC92Vg4f5eZkR+ahHWryEO6o9gySjAM7Bls=; b=ITe80eyOw6VUFvEVgN26tlD1bsfS4cdbOPpcYkGbQfancYta7I3InLXrZ/XgbWDl8L APN1BER0/CSkoGP74RdFJlMCMNcpB9K0DRpJ6Z8AL17l4hxXTPxUBKvkoHCiSl8SwFVM KFXAQxwCsaKzHjBFg4NeA/ZX8XTI10qz6Zd4XZT+Ed/YAgYJJZXDa5mbnWgqxTC8O9YU +TZWvQdntRlbCkKqX5Sm3fYIBuu2LsALmzaCNr+rRkFwEYBIP3jJ3Uq9kPOb3GC9UwZl cYufkeMTd9xDDLmmzjr1wekUh3U1Vee0QDQewHnp0KMfqHt+w6CQ9RRGJ4gpGgwiOU/W 48vQ== X-Gm-Message-State: AKGB3mKKUqPokcLsMI1m5FyiQWi0/HXFK49+TciByQn6RI1bpNoELpp3 ApD8zkPTY4juyAmO0w6JkXsiAKLeLfyxEwuYhPpSUCw= X-Google-Smtp-Source: AGs4zMbfEheich7/rH38sI2y1TbsBpbR3Hy7pTCOmuHc9sSr5oAXOGMEBqQnrT8Rgb0ps5qwBYJBBCfdZJumlcd0flg= X-Received: by 10.237.37.177 with SMTP id x46mr11662310qtc.76.1512169722239; Fri, 01 Dec 2017 15:08:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.237.57.10 with HTTP; Fri, 1 Dec 2017 15:08:21 -0800 (PST) From: David Benjamin Date: Fri, 1 Dec 2017 18:08:21 -0500 Message-ID: Subject: Tidying up the OpenSSL private key password logic To: hostap@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20171201_150904_694439_3CC91461 X-CRM114-Status: GOOD ( 20.38 ) X-Spam-Score: -2.0 (--) X-Spam-Report: SpamAssassin version 3.4.1 on bombadil.infradead.org summary: Content analysis details: (-2.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/, no trust [2607:f8b0:400d:c0d:0:0:0:22a listed in] [list.dnswl.org] -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-BeenThere: hostap@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Hostap" Errors-To: hostap-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org Hi folks, I've attached two patches that I think tidy up the logic around OpenSSL private key loading and passwords. The first just removes an unnecessary strdup. That parameter isn't mutated or anything, it's just a generic data argument to the same callback that you pass in. The second avoids using the SSL(_CTX) default password callback altogether. Since you all use it for one-off calls anyway, it ends up being a little cumbersome as you must set and unset them. Further, you end up mutating the SSL_CTX after SSLs have been created, which isn't generally safe. Rather, I think it's cleaner to just pass the password into the PEM_read_bio_PrivateKey call yourself. The SSL-level functions are merely convenience routines on top of this. This also allows abstracting away the DER/PEM fallback code. (It also avoids a mess of OpenSSL version variability.) Note: you probably want to run tests on this. I wasn't sure how to do that, but I have checked that they compile on my system. Thoughts? David From 821e2b131340a959bdb52a5079eac4932640509a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 18 Sep 2017 11:47:47 -0400 Subject: [PATCH 2/2] OpenSSL: Avoid SSL*_use_default_passwd_cb. These functions are a bit awkward to use for one-off file loads, as suggested by the tls_clear_default_passwd_cb logic. There was also some historical mess with OpenSSL versions and either not having per-SSL settings, having per-SSL settings but ignoring them, and requiring the per-SSL settings. Instead, loading the key with the lower-level functions seems a bit tidier and also allows abstracting away trying both formats, one after another. Signed-off-by: David Benjamin --- src/crypto/tls_openssl.c | 121 ++++++++++++++++++++++------------------------- 1 file changed, 56 insertions(+), 65 deletions(-) diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c index df2ce07d2..3d32fc1f7 100644 --- a/src/crypto/tls_openssl.c +++ b/src/crypto/tls_openssl.c @@ -2686,16 +2686,6 @@ static int tls_global_client_cert(struct tls_data *data, } -static int tls_passwd_cb(char *buf, int size, int rwflag, void *password) -{ - if (password == NULL) { - return 0; - } - os_strlcpy(buf, (char *) password, size); - return os_strlen(buf); -} - - #ifdef PKCS12_FUNCS static int tls_parse_pkcs12(struct tls_data *data, SSL *ssl, PKCS12 *p12, const char *passwd) @@ -3014,21 +3004,60 @@ static int tls_connection_engine_private_key(struct tls_connection *conn) } -static void tls_clear_default_passwd_cb(SSL_CTX *ssl_ctx, SSL *ssl) +#ifndef OPENSSL_NO_STDIO +static int tls_passwd_cb(char *buf, int size, int rwflag, void *password) { -#if OPENSSL_VERSION_NUMBER >= 0x10100000L -#ifndef LIBRESSL_VERSION_NUMBER -#ifndef OPENSSL_IS_BORINGSSL - if (ssl) { - SSL_set_default_passwd_cb(ssl, NULL); - SSL_set_default_passwd_cb_userdata(ssl, NULL); - } -#endif /* !BoringSSL */ -#endif /* !LibreSSL */ -#endif /* >= 1.1.0f */ - SSL_CTX_set_default_passwd_cb(ssl_ctx, NULL); - SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx, NULL); + if (password == NULL) { + return 0; + } + os_strlcpy(buf, (const char *) password, size); + return os_strlen(buf); +} + + +static int tls_use_private_key_file(struct tls_data *data, SSL *ssl, + const char *private_key, + const char *private_key_passwd) { + BIO *bio; + EVP_PKEY *pkey; + int ret; + + /* First try ASN.1. */ + bio = BIO_new_file(private_key, "r"); + if (bio == NULL) + return -1; + pkey = d2i_PrivateKey_bio(bio, NULL); + BIO_free(bio); + + if (pkey != NULL) { + wpa_printf(MSG_DEBUG, "OpenSSL: " + "tls_use_private_key_file (DER) --> loaded"); + } else { + /* Try PEM with the provided password. */ + bio = BIO_new_file(private_key, "r"); + if (bio == NULL) + return -1; + pkey = PEM_read_bio_PrivateKey(bio, NULL, tls_passwd_cb, + (void *)private_key_passwd); + BIO_free(bio); + if (pkey == NULL) + return -1; + wpa_printf(MSG_DEBUG, "OpenSSL: " + "tls_use_private_key_file (PEM) --> loaded"); + /* Clear errors from the previous failed load. */ + ERR_clear_error(); + } + + if (ssl != NULL) { + ret = SSL_use_PrivateKey(ssl, pkey); + } else { + ret = SSL_CTX_use_PrivateKey(data->ssl, pkey); + } + + EVP_PKEY_free(pkey); + return ret == 1 ? 0 : -1; } +#endif /* OPENSSL_NO_STDIO */ static int tls_connection_private_key(struct tls_data *data, @@ -3038,30 +3067,11 @@ static int tls_connection_private_key(struct tls_data *data, const u8 *private_key_blob, size_t private_key_blob_len) { - SSL_CTX *ssl_ctx = data->ssl; int ok; if (private_key == NULL && private_key_blob == NULL) return 0; -#if OPENSSL_VERSION_NUMBER >= 0x10100000L -#ifndef LIBRESSL_VERSION_NUMBER -#ifndef OPENSSL_IS_BORINGSSL - /* - * In OpenSSL >= 1.1.0f SSL_use_PrivateKey_file() uses the callback - * from the SSL object. See OpenSSL commit d61461a75253. - */ - SSL_set_default_passwd_cb(conn->ssl, tls_passwd_cb); - SSL_set_default_passwd_cb_userdata(conn->ssl, - (void *)private_key_passwd); -#endif /* !BoringSSL */ -#endif /* !LibreSSL */ -#endif /* >= 1.1.0f && */ - /* Keep these for OpenSSL < 1.1.0f */ - SSL_CTX_set_default_passwd_cb(ssl_ctx, tls_passwd_cb); - SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx, - (void *)private_key_passwd); - ok = 0; while (private_key_blob) { if (SSL_use_PrivateKey_ASN1(EVP_PKEY_RSA, conn->ssl, @@ -3105,18 +3115,8 @@ static int tls_connection_private_key(struct tls_data *data, while (!ok && private_key) { #ifndef OPENSSL_NO_STDIO - if (SSL_use_PrivateKey_file(conn->ssl, private_key, - SSL_FILETYPE_ASN1) == 1) { - wpa_printf(MSG_DEBUG, "OpenSSL: " - "SSL_use_PrivateKey_File (DER) --> OK"); - ok = 1; - break; - } - - if (SSL_use_PrivateKey_file(conn->ssl, private_key, - SSL_FILETYPE_PEM) == 1) { - wpa_printf(MSG_DEBUG, "OpenSSL: " - "SSL_use_PrivateKey_File (PEM) --> OK"); + if (tls_use_private_key_file(data, conn->ssl, private_key, + private_key_passwd) == 0) { ok = 1; break; } @@ -3146,11 +3146,9 @@ static int tls_connection_private_key(struct tls_data *data, if (!ok) { tls_show_errors(MSG_INFO, __func__, "Failed to load private key"); - tls_clear_default_passwd_cb(ssl_ctx, conn->ssl); return -1; } ERR_clear_error(); - tls_clear_default_passwd_cb(ssl_ctx, conn->ssl); if (!SSL_check_private_key(conn->ssl)) { tls_show_errors(MSG_INFO, __func__, "Private key failed " @@ -3172,24 +3170,17 @@ static int tls_global_private_key(struct tls_data *data, if (private_key == NULL) return 0; - SSL_CTX_set_default_passwd_cb(ssl_ctx, tls_passwd_cb); - SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx, - (void *)private_key_passwd); if ( #ifndef OPENSSL_NO_STDIO - SSL_CTX_use_PrivateKey_file(ssl_ctx, private_key, - SSL_FILETYPE_ASN1) != 1 && - SSL_CTX_use_PrivateKey_file(ssl_ctx, private_key, - SSL_FILETYPE_PEM) != 1 && + tls_use_private_key_file(data, NULL, private_key, + private_key_passwd) && #endif /* OPENSSL_NO_STDIO */ tls_read_pkcs12(data, NULL, private_key, private_key_passwd)) { tls_show_errors(MSG_INFO, __func__, "Failed to load private key"); - tls_clear_default_passwd_cb(ssl_ctx, NULL); ERR_clear_error(); return -1; } - tls_clear_default_passwd_cb(ssl_ctx, NULL); ERR_clear_error(); if (!SSL_CTX_check_private_key(ssl_ctx)) { -- 2.15.0.531.g2ccb3012c9-goog