Skip to content

Commit

Permalink
Bug 1289186 - wait for the server certificate to verify successfully …
Browse files Browse the repository at this point in the history 
…before asking for a client auth certificate r=jschanck

If a TLS server asks for a client authentication certificate, no dialog asking
the user to select one should be shown until the server's certificate verifies
successfully.

Differential Revision: https://phabricator.services.mozilla.com/D175170
  • Loading branch information
@mozkeeler
mozkeeler committed Apr 13, 2023
1 parent 2099d82 commit 9db5d24
Show file tree
Hide file tree
Showing 10 changed files with 187 additions and 105 deletions.
Binary file modified build/pgo/certs/cert9.db
View file
Binary file not shown.
Binary file modified build/pgo/certs/key4.db
View file
Binary file not shown.
Binary file modified build/pgo/certs/mochitest.client
View file
Binary file not shown.
1 change: 1 addition & 0 deletions build/pgo/server-locations.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ https://expired.example.com:443 privileged,cert=expired
https://requestclientcert.example.com:443 privileged,clientauth=request
https://requireclientcert.example.com:443 privileged,clientauth=require
https://requireclientcert-2.example.com:443 privileged,clientauth=require
https://requireclientcert-untrusted.example.com:443 privileged,clientauth=require,cert=untrusted
https://mismatch.expired.example.com:443 privileged,cert=expired
https://mismatch.untrusted.example.com:443 privileged,cert=untrusted
https://untrusted-expired.example.com:443 privileged,cert=untrustedandexpired
Expand Down
24 changes: 14 additions & 10 deletions security/manager/ssl/NSSSocketControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ NSSSocketControl::NSSSocketControl(const nsCString& aHostName, int32_t aPort,
uint32_t providerTlsFlags)
: CommonSocketControl(aHostName, aPort, providerFlags),
mFd(nullptr),
mCertVerificationState(before_cert_verification),
mCertVerificationState(BeforeCertVerification),
mSharedState(aState),
mForSTARTTLS(false),
mTLSVersionRange{0, 0},
Expand All @@ -45,7 +45,8 @@ NSSSocketControl::NSSSocketControl(const nsCString& aHostName, int32_t aPort,
mMACAlgorithmUsed(nsITLSSocketControl::SSL_MAC_UNKNOWN),
mProviderTlsFlags(providerTlsFlags),
mSocketCreationTimestamp(TimeStamp::Now()),
mPlaintextBytesRead(0) {}
mPlaintextBytesRead(0),
mPendingSelectClientAuthCertificate(nullptr) {}

NS_IMETHODIMP
NSSSocketControl::GetKEAUsed(int16_t* aKea) {
Expand Down Expand Up @@ -336,12 +337,12 @@ nsresult NSSSocketControl::SetFileDescPtr(PRFileDesc* aFilePtr) {

void NSSSocketControl::SetCertVerificationWaiting() {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
// mCertVerificationState may be before_cert_verification for the first
// handshake on the connection, or after_cert_verification for subsequent
// mCertVerificationState may be BeforeCertVerification for the first
// handshake on the connection, or AfterCertVerification for subsequent
// renegotiation handshakes.
MOZ_ASSERT(mCertVerificationState != waiting_for_cert_verification,
"Invalid state transition to waiting_for_cert_verification");
mCertVerificationState = waiting_for_cert_verification;
MOZ_ASSERT(mCertVerificationState != WaitingForCertVerification,
"Invalid state transition to WaitingForCertVerification");
mCertVerificationState = WaitingForCertVerification;
}

// Be careful that SetCertVerificationResult does NOT get called while we are
Expand All @@ -351,8 +352,8 @@ void NSSSocketControl::SetCertVerificationWaiting() {
void NSSSocketControl::SetCertVerificationResult(PRErrorCode errorCode) {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
SetUsedPrivateDNS(GetProviderFlags() & nsISocketProvider::USED_PRIVATE_DNS);
MOZ_ASSERT(mCertVerificationState == waiting_for_cert_verification,
"Invalid state transition to cert_verification_finished");
MOZ_ASSERT(mCertVerificationState == WaitingForCertVerification,
"Invalid state transition to AfterCertVerification");

if (mFd) {
SECStatus rv = SSL_AuthCertificateComplete(mFd, errorCode);
Expand Down Expand Up @@ -380,7 +381,7 @@ void NSSSocketControl::SetCertVerificationResult(PRErrorCode errorCode) {
AssertedCast<uint32_t>(mPlaintextBytesRead));
}

mCertVerificationState = after_cert_verification;
mCertVerificationState = AfterCertVerification;
}

void NSSSocketControl::ClientAuthCertificateSelected(
Expand Down Expand Up @@ -470,6 +471,9 @@ NSSSocketControl::SetHandshakeCallbackListener(

PRStatus NSSSocketControl::CloseSocketAndDestroy() {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();

mPendingSelectClientAuthCertificate = nullptr;

PRFileDesc* popped = PR_PopIOLayer(mFd, PR_TOP_IO_LAYER);
MOZ_ASSERT(
popped && popped->identity == nsSSLIOLayerHelpers::nsSSLIOLayerIdentity,
Expand Down
46 changes: 39 additions & 7 deletions security/manager/ssl/NSSSocketControl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@

#include "CommonSocketControl.h"
#include "SharedSSLState.h"
#include "TLSClientAuthCertSelection.h"
#include "nsThreadUtils.h"

extern mozilla::LazyLogModule gPIPNSSLog;

class SelectClientAuthCertificate;

class NSSSocketControl final : public CommonSocketControl {
public:
Expand Down Expand Up @@ -118,24 +124,24 @@ class NSSSocketControl final : public CommonSocketControl {

mozilla::psm::SharedSSLState& SharedState();

// XXX: These are only used on for diagnostic purposes
enum CertVerificationState {
before_cert_verification,
waiting_for_cert_verification,
after_cert_verification
BeforeCertVerification,
WaitingForCertVerification,
AfterCertVerification
};

void SetCertVerificationWaiting();

// Use errorCode == 0 to indicate success;
void SetCertVerificationResult(PRErrorCode errorCode) override;

void ClientAuthCertificateSelected(
nsTArray<uint8_t>& certBytes,
nsTArray<nsTArray<uint8_t>>& certChainBytes);

// for logging only
PRBool IsWaitingForCertVerification() const {
bool IsWaitingForCertVerification() const {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
return mCertVerificationState == waiting_for_cert_verification;
return mCertVerificationState == WaitingForCertVerification;
}
void AddPlaintextBytesRead(uint64_t val) {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
Expand Down Expand Up @@ -218,8 +224,32 @@ class NSSSocketControl final : public CommonSocketControl {
void SetPreliminaryHandshakeInfo(const SSLChannelInfo& channelInfo,
const SSLCipherSuiteInfo& cipherInfo);

void SetPendingSelectClientAuthCertificate(
nsCOMPtr<nsIRunnable>&& selectClientAuthCertificate) {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
MOZ_LOG(
gPIPNSSLog, mozilla::LogLevel::Debug,
("[%p] setting pending select client auth certificate", (void*)mFd));
mPendingSelectClientAuthCertificate =
std::move(selectClientAuthCertificate);
}

void MaybeDispatchSelectClientAuthCertificate() {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
if (!IsWaitingForCertVerification() &&
mPendingSelectClientAuthCertificate) {
MOZ_LOG(gPIPNSSLog, mozilla::LogLevel::Debug,
("[%p] dispatching pending select client auth certificate",
(void*)mFd));
mozilla::Unused << NS_DispatchToMainThread(
mPendingSelectClientAuthCertificate);
mPendingSelectClientAuthCertificate = nullptr;
}
}

private:
~NSSSocketControl() = default;

PRFileDesc* mFd;

CertVerificationState mCertVerificationState;
Expand Down Expand Up @@ -270,6 +300,8 @@ class NSSSocketControl final : public CommonSocketControl {
mozilla::TimeStamp mSocketCreationTimestamp;
uint64_t mPlaintextBytesRead;

nsCOMPtr<nsIRunnable> mPendingSelectClientAuthCertificate;

// Regarding the client certificate message in the TLS handshake, RFC 5246
// (TLS 1.2) says:
// If the certificate_authorities list in the certificate request
Expand Down
110 changes: 28 additions & 82 deletions security/manager/ssl/TLSClientAuthCertSelection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,34 +98,6 @@ static bool hasExplicitKeyUsageNonRepudiation(CERTCertificate* cert) {
return !!(keyUsage & KU_NON_REPUDIATION);
}

// This class is used to store the needed information for invoking the client
// cert selection UI.
class ClientAuthInfo final {
public:
explicit ClientAuthInfo(const nsACString& hostName,
const mozilla::OriginAttributes& originAttributes,
int32_t port, uint32_t providerFlags,
uint32_t providerTlsFlags);
~ClientAuthInfo() = default;
ClientAuthInfo(ClientAuthInfo&& aOther) noexcept;

const nsACString& HostName() const;
const mozilla::OriginAttributes& OriginAttributesRef() const;
int32_t Port() const;
uint32_t ProviderFlags() const;
uint32_t ProviderTlsFlags() const;

ClientAuthInfo(const ClientAuthInfo&) = delete;
void operator=(const ClientAuthInfo&) = delete;

private:
nsCString mHostName;
mozilla::OriginAttributes mOriginAttributes;
int32_t mPort;
uint32_t mProviderFlags;
uint32_t mProviderTlsFlags;
};

ClientAuthInfo::ClientAuthInfo(const nsACString& hostName,
const OriginAttributes& originAttributes,
int32_t port, uint32_t providerFlags,
Expand Down Expand Up @@ -435,41 +407,6 @@ ClientAuthCertificateSelected::Run() {
return NS_OK;
}

// Helper runnable to select a client authentication certificate. Gets created
// on the socket thread or an IPC thread, runs on the main thread, and then runs
// its continuation on the socket thread.
class SelectClientAuthCertificate : public Runnable {
public:
SelectClientAuthCertificate(ClientAuthInfo&& info,
UniqueCERTCertificate&& serverCert,
nsTArray<nsTArray<uint8_t>>&& caNames,
UniqueCERTCertList&& potentialClientCertificates,
ClientAuthCertificateSelectedBase* continuation)
: Runnable("SelectClientAuthCertificate"),
mInfo(std::move(info)),
mServerCert(std::move(serverCert)),
mCANames(std::move(caNames)),
mPotentialClientCertificates(std::move(potentialClientCertificates)),
mContinuation(continuation) {}

NS_IMETHOD Run() override;

private:
mozilla::pkix::Result BuildChainForCertificate(
nsTArray<uint8_t>& certBytes,
nsTArray<nsTArray<uint8_t>>& certChainBytes);
void DoSelectClientAuthCertificate();

ClientAuthInfo mInfo;
UniqueCERTCertificate mServerCert;
nsTArray<nsTArray<uint8_t>> mCANames;
UniqueCERTCertList mPotentialClientCertificates;
RefPtr<ClientAuthCertificateSelectedBase> mContinuation;

nsTArray<nsTArray<uint8_t>> mEnterpriseCertificates;
nsTArray<uint8_t> mSelectedCertBytes;
};

NS_IMETHODIMP
SelectClientAuthCertificate::Run() {
DoSelectClientAuthCertificate();
Expand Down Expand Up @@ -793,12 +730,6 @@ SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket,
// appropriate information to the NSSSocketControl, which then calls
// SSL_ClientCertCallbackComplete to continue the connection.
if (XRE_IsSocketProcess()) {
mozilla::ipc::PBackgroundChild* actorChild = mozilla::ipc::BackgroundChild::
GetOrCreateForSocketParentBridgeForCurrentThread();
if (!actorChild) {
PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
return SECFailure;
}
RefPtr<SelectTLSClientAuthCertChild> selectClientAuthCertificate(
new SelectTLSClientAuthCertChild(continuation));
nsAutoCString hostname(info->GetHostName());
Expand All @@ -809,26 +740,41 @@ SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket,
}
serverCertBytes.AppendElements(serverCert->derCert.data,
serverCert->derCert.len);
if (!actorChild->SendPSelectTLSClientAuthCertConstructor(
selectClientAuthCertificate, hostname, info->GetOriginAttributes(),
info->GetPort(), info->GetProviderFlags(),
info->GetProviderTlsFlags(), ByteArray(serverCertBytes),
caNamesBytes)) {
PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
return SECFailure;
}
OriginAttributes originAttributes(info->GetOriginAttributes());
int32_t port(info->GetPort());
uint32_t providerFlags(info->GetProviderFlags());
uint32_t providerTlsFlags(info->GetProviderTlsFlags());
nsCOMPtr<nsIRunnable> remoteSelectClientAuthCertificate(
NS_NewRunnableFunction(
"RemoteSelectClientAuthCertificate",
[selectClientAuthCertificate(
std::move(selectClientAuthCertificate)),
hostname(std::move(hostname)),
originAttributes(std::move(originAttributes)), port, providerFlags,
providerTlsFlags, serverCertBytes(std::move(serverCertBytes)),
caNamesBytes(std::move(caNamesBytes))]() {
mozilla::ipc::PBackgroundChild* actorChild =
mozilla::ipc::BackgroundChild::
GetOrCreateForSocketParentBridgeForCurrentThread();
if (actorChild) {
Unused << actorChild->SendPSelectTLSClientAuthCertConstructor(
selectClientAuthCertificate, hostname, originAttributes,
port, providerFlags, providerTlsFlags,
ByteArray(serverCertBytes), caNamesBytes);
}
}));
info->SetPendingSelectClientAuthCertificate(
std::move(remoteSelectClientAuthCertificate));
} else {
ClientAuthInfo authInfo(info->GetHostName(), info->GetOriginAttributes(),
info->GetPort(), info->GetProviderFlags(),
info->GetProviderTlsFlags());
RefPtr<SelectClientAuthCertificate> selectClientAuthCertificate(
nsCOMPtr<nsIRunnable> selectClientAuthCertificate(
new SelectClientAuthCertificate(
std::move(authInfo), std::move(serverCert), std::move(caNames),
std::move(potentialClientCertificates), continuation));
if (NS_FAILED(NS_DispatchToMainThread(selectClientAuthCertificate))) {
PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
return SECFailure;
}
info->SetPendingSelectClientAuthCertificate(
std::move(selectClientAuthCertificate));
}

// Meanwhile, tell NSS this connection is blocking for now.
Expand Down
65 changes: 65 additions & 0 deletions security/manager/ssl/TLSClientAuthCertSelection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include "nsThreadUtils.h"
#include "ssl.h"

class NSSSocketControl;

// NSS callback to select a client authentication certificate. See documentation
// at the top of TLSClientAuthCertSelection.cpp.
SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket,
Expand Down Expand Up @@ -52,4 +54,67 @@ class ClientAuthCertificateSelected : public ClientAuthCertificateSelectedBase {
RefPtr<NSSSocketControl> mSocketInfo;
};

// This class is used to store the needed information for invoking the client
// cert selection UI.
class ClientAuthInfo final {
public:
explicit ClientAuthInfo(const nsACString& hostName,
const mozilla::OriginAttributes& originAttributes,
int32_t port, uint32_t providerFlags,
uint32_t providerTlsFlags);
~ClientAuthInfo() = default;
ClientAuthInfo(ClientAuthInfo&& aOther) noexcept;

const nsACString& HostName() const;
const mozilla::OriginAttributes& OriginAttributesRef() const;
int32_t Port() const;
uint32_t ProviderFlags() const;
uint32_t ProviderTlsFlags() const;

ClientAuthInfo(const ClientAuthInfo&) = delete;
void operator=(const ClientAuthInfo&) = delete;

private:
nsCString mHostName;
mozilla::OriginAttributes mOriginAttributes;
int32_t mPort;
uint32_t mProviderFlags;
uint32_t mProviderTlsFlags;
};

// Helper runnable to select a client authentication certificate. Gets created
// on the socket thread or an IPC thread, runs on the main thread, and then runs
// its continuation on the socket thread.
class SelectClientAuthCertificate : public mozilla::Runnable {
public:
SelectClientAuthCertificate(
ClientAuthInfo&& info, mozilla::UniqueCERTCertificate&& serverCert,
nsTArray<nsTArray<uint8_t>>&& caNames,
mozilla::UniqueCERTCertList&& potentialClientCertificates,
ClientAuthCertificateSelectedBase* continuation)
: Runnable("SelectClientAuthCertificate"),
mInfo(std::move(info)),
mServerCert(std::move(serverCert)),
mCANames(std::move(caNames)),
mPotentialClientCertificates(std::move(potentialClientCertificates)),
mContinuation(continuation) {}

NS_IMETHOD Run() override;

private:
mozilla::pkix::Result BuildChainForCertificate(
nsTArray<uint8_t>& certBytes,
nsTArray<nsTArray<uint8_t>>& certChainBytes);
void DoSelectClientAuthCertificate();

ClientAuthInfo mInfo;
mozilla::UniqueCERTCertificate mServerCert;
nsTArray<nsTArray<uint8_t>> mCANames;
mozilla::UniqueCERTCertList mPotentialClientCertificates;
RefPtr<ClientAuthCertificateSelectedBase> mContinuation;

nsTArray<nsTArray<uint8_t>> mEnterpriseCertificates;
nsTArray<uint8_t> mSelectedCertBytes;
};

#endif // SECURITY_MANAGER_SSL_TLSCLIENTAUTHCERTSELECTION_H_
2 changes: 2 additions & 0 deletions security/manager/ssl/nsNSSIOLayer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,8 @@ static int16_t nsSSLIOLayerPoll(PRFileDesc* fd, int16_t in_flags,
: "[%p] poll SSL socket using lower %d\n",
fd, (int)in_flags));

socketInfo->MaybeDispatchSelectClientAuthCertificate();

// We want the handshake to continue during certificate validation, so we
// don't need to do anything special here. libssl automatically blocks when
// it reaches any point that would be unsafe to send/receive something before
Expand Down
Loading

0 comments on commit 9db5d24

Please sign in to comment.