High firefox Logic Error 🔧 Commit mapped

Overview

High
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Impacthigh
DescriptionIncorrect boundary conditions in the Security: PSM component
ComponentSecurity/NSS
Bug ClassLogic Error
Tracker2078946
Fix commit58f4688447b5 (firefox) +61/-78
CISA KEVNot listed
CreditedMozilla
Disclosed2026-10-27

Changed Functions

FunctionChangeNotes
if
security/manager/ssl/NSSSocketControl.h
modified
if
security/manager/ssl/nsNSSIOLayer.cpp
modified

Files Changed

  • security/manager/ssl/NSSSocketControl.cpp
  • security/manager/ssl/NSSSocketControl.h
  • security/manager/ssl/nsNSSIOLayer.cpp
diff --git a/security/manager/ssl/NSSSocketControl.cpp b/security/manager/ssl/NSSSocketControl.cpp
index 47aa645e9b4..8f06cab1676 100644
--- a/security/manager/ssl/NSSSocketControl.cpp
+++ b/security/manager/ssl/NSSSocketControl.cpp
@@ -43,9 +43,7 @@ NSSSocketControl::NSSSocketControl(
       mDrewResumptionToken(false),
       mNotedTimeUntilReady(false),
       mEchExtensionStatus(EchExtensionStatus::kNotPresent),
-      mIsShortWritePending(false),
-      mShortWritePendingByte(0),
-      mShortWriteOriginalAmount(-1),
+      mMaybeShortWrite(Nothing()),
       mKEAUsed(nsITLSSocketControl::KEY_EXCHANGE_UNKNOWN),
       mKEAKeyBits(0),
       mMACAlgorithmUsed(nsITLSSocketControl::SSL_MAC_UNKNOWN),
diff --git a/security/manager/ssl/NSSSocketControl.h b/security/manager/ssl/NSSSocketControl.h
index f8a554034c1..7b66553ab14 100644
--- a/security/manager/ssl/NSSSocketControl.h
+++ b/security/manager/ssl/NSSSocketControl.h
@@ -10,6 +10,7 @@
 #include "TLSClientAuthCertSelection.h"
 #include "mozilla/Casting.h"
 #include "mozilla/Maybe.h"
+#include "mozilla/Span.h"
 #include "nsNSSIOLayer.h"
 #include "nsThreadUtils.h"
 
@@ -215,50 +216,51 @@ class NSSSocketControl final : public CommonSocketControl {
     mMACAlgorithmUsed = mac;
   }
 
-  void SetShortWritePending(int32_t amount, unsigned char data) {
+  void SetShortWritePending(const void* data, size_t amount) {
     COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-    mIsShortWritePending = true;
-    mShortWriteOriginalAmount = amount;
-    mShortWritePendingByte = data;
+    MOZ_ASSERT(mMaybeShortWrite.isNothing());
+    nsTArray<uint8_t> shortWrite(amount);
+    shortWrite.AppendElements(
+        reinterpret_cast<uint8_t*>(const_cast<void*>(data)), amount);
+    mMaybeShortWrite.emplace(std::move(shortWrite));
   }
 
   bool IsShortWritePending() {
     COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-    return mIsShortWritePending;
+    return mMaybeShortWrite.isSome();
   }
 
-  unsigned char const* GetShortWritePendingByteRef() {
+  size_t ResetShortWritePending() {
     COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-    return &mShortWritePendingByte;
+    MOZ_ASSERT(mMaybeShortWrite.isSome());
+    size_t shortWriteLength = mMaybeShortWrite->Length();
+    mMaybeShortWrite.reset();
+    return shortWriteLength;
   }
 
-  int32_t ResetShortWritePending() {
+  // Ensure that the caller is trying to send the same data as it was
+  // previously when we hit the short-write. To do otherwise would be an error.
+  const void* CheckShortWriteBuffer(const void* data, int32_t amount) {
     COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-    mIsShortWritePending = false;
-    return mShortWriteOriginalAmount;
-  }
-
-#ifdef DEBUG
-  // These helpers assert that the caller does try to send the same data
-  // as it was previously when we hit the short-write.  This is a measure
-  // to make sure we communicate correctly to the consumer.
-  void RememberShortWrittenBuffer(const unsigned char* data) {
-    COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-    mShortWriteBufferCheck =
-        mozilla::MakeUnique<char[]>(mShortWriteOriginalAmount);
-    memcpy(mShortWriteBufferCheck.get(), data, mShortWriteOriginalAmount);
-  }
-  void CheckShortWrittenBuffer(const unsigned char* data, int32_t amount) {
-    COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
-    if (!mShortWriteBufferCheck) return;
-    MOZ_ASSERT(amount >= mShortWriteOriginalAmount,
-               "unexpected amount length after short write");
-    MOZ_ASSERT(
-        !memcmp(mShortWriteBufferCheck.get(), data, mShortWriteOriginalAmount),
-        "unexpected buffer content after short write");
-    mShortWriteBufferCheck = nullptr;
+    MOZ_ASSERT(mMaybeShortWrite.isSome());
+    if (mMaybeShortWrite.isNothing()) {
+      return nullptr;
+    }
+    MOZ_ASSERT(amount > 0);
+    if (amount <= 0) {
+      return nullptr;
+    }
+    if (static_cast<size_t>(amount) < mMaybeShortWrite->Length()) {
+      return nullptr;
+    }
+    if (mozilla::Span(*mMaybeShortWrite) !=
+        mozilla::Span(reinterpret_cast<uint8_t*>(const_cast<void*>(data)),
+                      mMaybeShortWrite->Length())) {
+      return nullptr;
+    }
+    // Return a pointer to the byte that has yet to be written.
+    return static_cast<const uint8_t*>(data) + mMaybeShortWrite->Length() - 1;
   }
-#endif
 
   nsresult SetResumptionTokenFromExternalCache(PRFileDesc* fd);
 
@@ -314,23 +316,10 @@ class NSSSocketControl final : public CommonSocketControl {
   bool mNotedTimeUntilReady;
   EchExtensionStatus mEchExtensionStatus;  // Currently only used for telemetry.
 
-  // True when SSL layer has indicated an "SSL short write", i.e. need
-  // to call on send one or more times to push all pending data to write.
-  bool mIsShortWritePending;
-
-  // These are only valid if mIsShortWritePending is true.
-  //
-  // Value of the last byte pending from the SSL short write that needs
-  // to be passed to subsequent calls to send to perform the flush.
-  unsigned char mShortWritePendingByte;
-
-  // Original amount of data the upper layer has requested to write to
-  // return after the successful flush.
-  int32_t mShortWriteOriginalAmount;
-
-#ifdef DEBUG
-  mozilla::UniquePtr<char[]> mShortWriteBufferCheck;
-#endif
+  // If libssl has indicated a "short write", the layer above PSM must retry
+  // sending the exact same data. If this is Some, all but the last byte has
+  // already been written to libssl.
+  mozilla::Maybe<nsTArray<uint8_t>> mMaybeShortWrite;
 
   // mKEA* are used in false start and http/2 detetermination
   // Values are from nsITLSSocketControl
diff --git a/security/manager/ssl/nsNSSIOLayer.cpp b/security/manager/ssl/nsNSSIOLayer.cpp
index b24a5b21513..c51a1456d50 100644
--- a/security/manager/ssl/nsNSSIOLayer.cpp
+++ b/security/manager/ssl/nsNSSIOLayer.cpp
@@ -821,7 +821,9 @@ static int32_t PSMRecv(PRFileDesc* fd, void* buf, int32_t amount, int flags,
 static int32_t PSMSend(PRFileDesc* fd, const void* buf, int32_t amount,
                        int flags, PRIntervalTime timeout) {
   NSSSocketControl* socketInfo = getSocketInfoIfRunning(fd, writing);
-  if (!socketInfo) return -1;
+  if (!socketInfo) {
+    return -1;
+  }
 
   if (flags != 0) {
     PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
@@ -833,15 +835,13 @@ static int32_t PSMSend(PRFileDesc* fd, const void* buf, int32_t amount,
 #endif
 
   if (socketInfo->IsShortWritePending() && amount > 0) {
-    // We got "SSL short write" last time, try to flush the pending byte.
-#ifdef DEBUG
-    socketInfo->CheckShortWrittenBuffer(static_cast<const unsigned char*>(buf),
-                                        amount);
-#endif
-
-    buf = socketInfo->GetShortWritePendingByteRef();
+    // We got a "short write" last time, try to flush the pending byte.
+    buf = socketInfo->CheckShortWriteBuffer(buf, amount);
+    if (!buf) {
+      PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
+      return -1;
+    }
     amount = 1;
-
     MOZ_LOG(gPIPNSSLog, LogLevel::Verbose,
             ("[%p] pushing 1 byte after SSL short write", fd));
   }
@@ -862,6 +862,10 @@ static int32_t PSMSend(PRFileDesc* fd, const void* buf, int32_t amount,
   // in the previous call. We then return the total number of bytes written,
   // which is the number that caused the short write plus the additional byte
   // we just wrote out.
+  // Note that the layer above *must* retry sending the exact same data. To do
+  // otherwise is a violation of NSS's expectations and is an error.
+  // Because early data can be truncated by libssl, only perform this
+  // optimization after the handshake has completed.
 
   // The 16384 value is based on libssl's maximum buffer size:
   //    MAX_FRAGMENT_LENGTH - 1
@@ -869,12 +873,11 @@ static int32_t PSMSend(PRFileDesc* fd, const void* buf, int32_t amount,
   // It's in a private header, though, filed bug 1394822 to expose it.
   static const int32_t kShortWrite16k = 16383;
 
-  if ((amount > 1 && bytesWritten == (amount - 1)) ||
-      (amount > kShortWrite16k && bytesWritten == kShortWrite16k)) {
-    // This is indication of an "SSL short write", block to force retry.
-    socketInfo->SetShortWritePending(
-        bytesWritten + 1,  // The amount to return after the flush
-        *(static_cast<const unsigned char*>(buf) + bytesWritten));
+  if (socketInfo->IsHandshakeCompleted() &&
+      ((amount > 1 && bytesWritten == (amount - 1)) ||
+       (amount > kShortWrite16k && bytesWritten == kShortWrite16k))) {
+    // This indicates a "short write", so block to force a retry.
+    socketInfo->SetShortWritePending(buf, bytesWritten + 1);
 
     MOZ_LOG(
         gPIPNSSLog, LogLevel::Verbose,
@@ -883,24 +886,17 @@ static int32_t PSMSend(PRFileDesc* fd, const void* buf, int32_t amount,
 
     bytesWritten = -1;
     PR_SetError(PR_WOULD_BLOCK_ERROR, 0);
-
-#ifdef DEBUG
-    socketInfo->RememberShortWrittenBuffer(
-        static_cast<const unsigned char*>(buf));
-#endif
-
   } else if (socketInfo->IsShortWritePending() && bytesWritten == 1) {
-    // We have now flushed all pending data in the SSL socket
-    // after the indicated short write.  Tell the upper layer
-    // it has sent all its data now.
+    // We have now flushed all pending data in the SSL socket after the
+    // indicated short write. Tell the upper layer it has sent all the data
+    // from that write now.
     MOZ_LOG(gPIPNSSLog, LogLevel::Verbose,
             ("[%p] finished SSL short write", fd));
-
     bytesWritten = socketInfo->ResetShortWritePending();
   }
 
   MOZ_LOG(gPIPNSSLog, LogLevel::Verbose,
-          ("[%p] wrote %d bytes\n", fd, bytesWritten));
+          ("[%p] wrote %d bytes", fd, bytesWritten));
 
   return checkHandshake(bytesWritten, false, fd, socketInfo);
 }
Loading diff…