From b9557296cb988c6007ed17f182a03c8205d5dffc Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Mon, 7 Aug 2017 12:49:59 +0200 Subject: Fix crash when reading a PKCS12 file with no private key The only reason our code wants PKCS12 files is for a private key, but a valid file needn't contain one; and reading a file without lead to a crash in QSslKeyPrivate::fromEVP_PKEY(). So check for missing key and fail the load, since the file is useless to us. Also ensure the caller's pkey is initialized, as we aren't promised that PKCS12_parse() will set it when there is no private key. Add a test for this case (it crashes without the fix) and update the instructions for how to generate test data to cover it also. (Corrected the wording there, too; at the interactive prompt, "providing no password" really provides an empty password.) Task-number: QTBUG-62335 Change-Id: I617508b903f6d9dee40d539b7136b0be8bc2c747 Reviewed-by: Timur Pocheptsov --- src/network/ssl/qsslkey_openssl.cpp | 3 +++ src/network/ssl/qsslsocket_openssl.cpp | 2 +- .../auto/network/ssl/qsslcertificate/pkcs12/README | 21 ++++++++++++++++----- .../ssl/qsslcertificate/pkcs12/leaf-nokey.p12 | Bin 0 -> 2216 bytes .../ssl/qsslcertificate/tst_qsslcertificate.cpp | 10 ++++++++++ 5 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 tests/auto/network/ssl/qsslcertificate/pkcs12/leaf-nokey.p12 diff --git a/src/network/ssl/qsslkey_openssl.cpp b/src/network/ssl/qsslkey_openssl.cpp index 79df33ecca..26119023d1 100644 --- a/src/network/ssl/qsslkey_openssl.cpp +++ b/src/network/ssl/qsslkey_openssl.cpp @@ -84,6 +84,9 @@ void QSslKeyPrivate::clear(bool deep) bool QSslKeyPrivate::fromEVP_PKEY(EVP_PKEY *pkey) { + if (pkey == nullptr) + return false; + if (pkey->type == EVP_PKEY_RSA) { isNull = false; algorithm = QSsl::Rsa; diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp index 644dfdb6a8..ab82cdcfc9 100644 --- a/src/network/ssl/qsslsocket_openssl.cpp +++ b/src/network/ssl/qsslsocket_openssl.cpp @@ -1805,7 +1805,7 @@ bool QSslSocketBackendPrivate::importPkcs12(QIODevice *device, } // Extract the data - EVP_PKEY *pkey; + EVP_PKEY *pkey = nullptr; X509 *x509; STACK_OF(X509) *ca = 0; diff --git a/tests/auto/network/ssl/qsslcertificate/pkcs12/README b/tests/auto/network/ssl/qsslcertificate/pkcs12/README index 1828d089c1..231567f586 100644 --- a/tests/auto/network/ssl/qsslcertificate/pkcs12/README +++ b/tests/auto/network/ssl/qsslcertificate/pkcs12/README @@ -1,8 +1,19 @@ -The PKCS#12 bundle was created by running the following on -in the qsslsocket/certs directory: +The PKCS#12 bundle was created by running the following in an +interactive shell in ../../qsslsocket/certs/: -openssl pkcs12 -export -in leaf.crt -inkey leaf.key \ - -out leaf.p12 \ +openssl pkcs12 -export -in leaf.crt \ + -inkey leaf.key -out leaf.p12 \ -certfile inter.crt -CAfile ca.crt -No password was provided. +An empty password was provided (twice). The pkcs.crt and pkcs.key +files were then copied here and leaf.p12 was moved here. + + +The test-case with no private key (in a valid PKCS12 file) was created +similarly but with the command adjusted to: + +openssl pkcs12 -export -in leaf.crt \ + -nokeys -out leaf-nokey.p12 \ + -certfile inter.crt -CAfile ca.crt + +The file leaf-nokey.p12 was then moved here. diff --git a/tests/auto/network/ssl/qsslcertificate/pkcs12/leaf-nokey.p12 b/tests/auto/network/ssl/qsslcertificate/pkcs12/leaf-nokey.p12 new file mode 100644 index 0000000000..032bf97b1b Binary files /dev/null and b/tests/auto/network/ssl/qsslcertificate/pkcs12/leaf-nokey.p12 differ diff --git a/tests/auto/network/ssl/qsslcertificate/tst_qsslcertificate.cpp b/tests/auto/network/ssl/qsslcertificate/tst_qsslcertificate.cpp index fced638ecb..064efc120b 100644 --- a/tests/auto/network/ssl/qsslcertificate/tst_qsslcertificate.cpp +++ b/tests/auto/network/ssl/qsslcertificate/tst_qsslcertificate.cpp @@ -1308,6 +1308,7 @@ void tst_QSslCertificate::version() void tst_QSslCertificate::pkcs12() { + // See pkcs12/README for how to generate the PKCS12 files used here. if (!QSslSocket::supportsSsl()) { qWarning("SSL not supported, skipping test"); return; @@ -1349,6 +1350,15 @@ void tst_QSslCertificate::pkcs12() QVERIFY(!caCerts.isEmpty()); QCOMPARE(caCerts.first(), caCert.first()); QCOMPARE(caCerts, caCert); + + // QTBUG-62335 - Fail (found no private key) but don't crash: + QFile nocert(testDataDir + QLatin1String("/pkcs12/leaf-nokey.p12")); + ok = nocert.open(QIODevice::ReadOnly); + QVERIFY(ok); + QTest::ignoreMessage(QtWarningMsg, "Unable to convert private key"); + ok = QSslCertificate::importPkcs12(&nocert, &key, &cert, &caCerts); + QVERIFY(!ok); + nocert.close(); } #endif // QT_NO_SSL -- cgit v1.2.3