From 17a053f0bc9eab192342ff5e0ce01dce763b001c Mon Sep 17 00:00:00 2001
From: Mingtao Yang <mingtao@meta.com>
Date: Thu, 25 Apr 2024 13:31:55 -0700
Subject: [PATCH] Ensure clientAuthRequested is set when negotiated PSK does
 not have certs

Summary:
The `Finished` handler assumes that `clientAuthRequested` is always set. Prior
to this diff, if a CachedPsk was used for a resumption connection where the
CachedPsk itself does not have a `serverCert` or a `clientCert` set (which can
happen if the PSK serialization/deserialization does not support encoding/decoding
the certificate), then this would not be set, causing the handshake to fail
even though the client successfully negotiated a resumption.

This diff changes the behavior so that if a PSK was successfully negotiated
(but the certs are null), then this would be treated as a `NotRequested`
client auth state. Since this is a resumed connection anyway, there would not
be a client certificate sent, so this field is effectively a no-op wrt TLS
logic.

Reviewed By: knekritz

Differential Revision: D56281894

fbshipit-source-id: b528d745e8e17e9328063b490d646aee79956cde
---
 fizz/client/ClientProtocol.cpp          | 17 ++++++++--------
 fizz/client/test/ClientProtocolTest.cpp | 26 +++++++++++++++++++++++++
 fizz/protocol/Types.h                   | 16 +++++++++++++++
 3 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/fizz/client/ClientProtocol.cpp b/fizz/client/ClientProtocol.cpp
index 7eda071c2c3..1f575bcb183 100644
--- a/fizz/client/ClientProtocol.cpp
+++ b/fizz/client/ClientProtocol.cpp
@@ -1297,9 +1297,7 @@ EventHandler<ClientTypes, StateEnum::ExpectingServerHello, Event::ServerHello>::
   }
 
   // Servers cannot accept GREASE PSK
-  if (echStatus == ECHStatus::Rejected &&
-      (negotiatedPsk.type == PskType::External ||
-       negotiatedPsk.type == PskType::Resumption)) {
+  if (echStatus == ECHStatus::Rejected && isPskAccepted(negotiatedPsk.type)) {
     throw FizzException(
         "ech rejected but server accepted psk",
         AlertDescription::illegal_parameter);
@@ -1346,10 +1344,12 @@ EventHandler<ClientTypes, StateEnum::ExpectingServerHello, Event::ServerHello>::
       folly::IOBuf::copyBuffer(handshakeReadSecret.secret);
 
   folly::Optional<ClientAuthType> authType;
-  if (negotiatedPsk.clientCert) {
-    authType = ClientAuthType::Stored;
-  } else if (negotiatedPsk.serverCert) {
-    authType = ClientAuthType::NotRequested;
+  if (isPskAccepted(negotiatedPsk.type)) {
+    if (negotiatedPsk.clientCert) {
+      authType = ClientAuthType::Stored;
+    } else {
+      authType = ClientAuthType::NotRequested;
+    }
   }
 
   std::chrono::system_clock::time_point handshakeTime;
@@ -1803,8 +1803,7 @@ Actions EventHandler<
         }
       });
 
-  if (state.pskType() == PskType::Resumption ||
-      state.pskType() == PskType::External) {
+  if (isPskAccepted(*state.pskType())) {
     return actions(
         std::move(mutateState),
         MutateState(&Transition<StateEnum::ExpectingFinished>));
diff --git a/fizz/client/test/ClientProtocolTest.cpp b/fizz/client/test/ClientProtocolTest.cpp
index 1a0cc224d0b..2fd07f09e61 100644
--- a/fizz/client/test/ClientProtocolTest.cpp
+++ b/fizz/client/test/ClientProtocolTest.cpp
@@ -173,6 +173,7 @@ class ClientProtocolTest : public ProtocolTest<ClientTypes, Actions> {
     state_.version() = TestProtocolVersion;
     state_.cipher() = CipherSuite::TLS_AES_128_GCM_SHA256;
     state_.state() = StateEnum::ExpectingEncryptedExtensions;
+    state_.pskType() = PskType::NotAttempted;
     state_.requestedExtensions() = std::vector<ExtensionType>(
         {ExtensionType::supported_versions,
          ExtensionType::key_share,
@@ -5478,6 +5479,31 @@ TEST_F(ClientProtocolTest, TestWaitForDataSizeHint) {
   auto wfd = expectAction<WaitForData>(actions);
   EXPECT_EQ(wfd.recordSizeHint, 17);
 }
+
+TEST_F(ClientProtocolTest, TestPskWithoutCerts) {
+  // Because CachedPsks can be serialized, and because certificates may fail
+  // to serialize for whatever reason, there may be an instance where a client
+  // uses a deserialized cached psk that does not contain either a client or
+  // a server certificate, but the PSK itself is valid (and the server accepted
+  // the offered PSK).
+  setupExpectingServerHello();
+
+  CachedPsk psk = getCachedPsk();
+  psk.clientCert = nullptr;
+  psk.serverCert = nullptr;
+
+  state_.attemptedPsk() = psk;
+
+  auto actions = detail::processEvent(state_, TestMessages::serverHelloPsk());
+  processStateMutations(actions);
+
+  ASSERT_TRUE(state_.pskType().has_value());
+  EXPECT_EQ(state_.pskType().value(), PskType::Resumption);
+
+  ASSERT_TRUE(state_.clientAuthRequested().has_value());
+  EXPECT_EQ(state_.clientAuthRequested().value(), ClientAuthType::NotRequested);
+}
+
 } // namespace test
 } // namespace client
 } // namespace fizz
diff --git a/fizz/protocol/Types.h b/fizz/protocol/Types.h
index 772762e1f14..07f3602ec6f 100644
--- a/fizz/protocol/Types.h
+++ b/fizz/protocol/Types.h
@@ -23,6 +23,22 @@ enum class PskType {
   Resumption
 };
 
+/**
+ * `isPskAccepted` returning true implies that the handshake negotiated a PSK to
+ * be used.
+ */
+inline bool isPskAccepted(PskType t) {
+  switch (t) {
+    case PskType::NotSupported:
+    case PskType::NotAttempted:
+    case PskType::Rejected:
+      return false;
+    case PskType::External:
+    case PskType::Resumption:
+      return true;
+  }
+}
+
 /**
  * Encryption level for the TLS layer.
  */