Merge branch 'main' into penpal-openssl-cleanse-sweep

prasden · web-flow · commit 9f2322cb6259 · 2026-05-08T09:34:42.000-07:00
diff --git a/crypto/fipsmodule/rand/internal.h b/crypto/fipsmodule/rand/internal.h
@@ -28,6 +28,15 @@ OPENSSL_EXPORT uint64_t get_private_thread_reseed_calls_since_initialization(voi
 OPENSSL_EXPORT uint64_t get_public_thread_generate_calls_since_seed(void);
 OPENSSL_EXPORT uint64_t get_public_thread_reseed_calls_since_initialization(void);
 
+// rand_thread_local_state_clear_all_FOR_TESTING runs the same shutdown
+// zeroization sequence that is registered via |atexit|. It is exposed for tests
+// that need to verify shutdown behavior (e.g. that a thread exiting after
+// shutdown does not deadlock). After this is called, no further |RAND_bytes|
+// calls in the test process will return output, so the caller is responsible
+// for arranging that no other code paths in the same process require
+// randomness afterwards.
+OPENSSL_EXPORT void rand_thread_local_state_clear_all_FOR_TESTING(void);
+
 // CTR_DRBG_STATE contains the state of a CTR_DRBG based on AES-256. See SP
 // 800-90Ar1.
 struct ctr_drbg_state_st {
diff --git a/crypto/fipsmodule/rand/rand.c b/crypto/fipsmodule/rand/rand.c
@@ -47,6 +47,16 @@ OPENSSL_STATIC_ASSERT((sizeof((struct rand_thread_local_state*)0)->generate_call
 DEFINE_BSS_GET(struct rand_thread_local_state *, thread_states_list_head)
 DEFINE_STATIC_MUTEX(thread_local_states_list_lock)
 
+// thread_local_drbg_shutdown_started is set to a non-zero value during process
+// exit by |rand_thread_local_state_clear_all|, while the linked-list lock is
+// held. All reads and writes occur under |thread_local_states_list_lock|, so
+// no atomic accessors are needed; doing the check under the lock also avoids
+// the otherwise-racy window where a TLS destructor could observe the flag as
+// unset, then block on the lock while shutdown zeroization runs, and then
+// free a state whose |state_clear_lock| has been intentionally write-locked
+// forever.
+DEFINE_BSS_GET(int, thread_local_drbg_shutdown_started)
+
 #if defined(_MSC_VER)
 #pragma section(".CRT$XCU", read)
 static void rand_thread_local_state_clear_all(void);
@@ -67,19 +77,42 @@ static void rand_thread_local_state_clear_all(void) __attribute__ ((destructor))
 // randomness from a non-valid state. The linked application should obviously
 // arrange that all threads are gracefully exited before exiting the process.
 // Yet, in cases where such graceful exit does not happen we ensure that no
-// output can be returned by locking all thread-local states and deliberately
-// not releasing the lock. A synchronization step in the core randomness
-// generation routine |RAND_bytes_core| then ensures that no randomness
-// generation can occur after a thread-local state has been locked. It also
-// ensures |rand_thread_local_state_free| cannot free any thread state while we
-// own the lock.
+// output can be returned by locking each thread-local state's
+// |state_clear_lock| and deliberately not releasing it. A synchronization step
+// in the core randomness generation routine |RAND_bytes_core| then ensures
+// that no randomness generation can occur after a thread-local state has been
+// locked.
+//
+// We additionally set |thread_local_drbg_shutdown_started| under the
+// linked-list lock so that any thread which has not yet registered a
+// thread-local state cannot do so after this routine has begun zeroization;
+// without this, a fresh thread could allocate a state and bypass zeroization.
+// The linked-list lock itself is released at the end of this function. If we
+// instead held it forever, |thread_local_list_delete_node| (called from a
+// thread's TLS destructor in |rand_thread_local_state_free|) would block
+// indefinitely. On Windows, TLS destructors run under the loader lock, so
+// blocking there causes |ExitProcess| to hang waiting for the loader lock.
+// See https://github.com/aws/aws-lc/issues/3197.
 //
-// When a thread-local DRBGs is gated from returning output, we can invoke the
+// When a thread-local DRBG is gated from returning output, we can invoke the
 // entropy source zeroization from |state->entropy_source|. The entropy source
 // implementation can assume that any returned seed is never used to generate
 // any randomness that is later returned to a consumer.
 static void rand_thread_local_state_clear_all(void) {
   CRYPTO_STATIC_MUTEX_lock_write(thread_local_states_list_lock_bss_get());
+
+  // Idempotency guard. Under normal operation this routine runs exactly once
+  // (via |atexit|), but it is also exposed for testing via
+  // |rand_thread_local_state_clear_all_FOR_TESTING|, and re-running would
+  // attempt to write-lock per-state |state_clear_lock|s that are already held
+  // write-locked by the first invocation -- which is UB for a non-recursive
+  // rwlock.
+  if (*thread_local_drbg_shutdown_started_bss_get() != 0) {
+    CRYPTO_STATIC_MUTEX_unlock_write(thread_local_states_list_lock_bss_get());
+    return;
+  }
+  *thread_local_drbg_shutdown_started_bss_get() = 1;
+
   for (struct rand_thread_local_state *state = *thread_states_list_head_bss_get();
     state != NULL; state = state->next) {
     CRYPTO_MUTEX_lock_write(&state->state_clear_lock);
@@ -90,13 +123,34 @@ static void rand_thread_local_state_clear_all(void) {
     state != NULL; state = state->next) {
     state->entropy_source->methods->zeroize_thread(state->entropy_source);
   }
+
+  CRYPTO_STATIC_MUTEX_unlock_write(thread_local_states_list_lock_bss_get());
+}
+
+void rand_thread_local_state_clear_all_FOR_TESTING(void) {
+  rand_thread_local_state_clear_all();
 }
 
-static void thread_local_list_delete_node(
+// thread_local_list_delete_node removes |node_delete| from the global
+// linked list and returns 1. If process-wide shutdown zeroization has already
+// begun, the node is left in place and 0 is returned -- the caller must not
+// free the node in that case because |rand_thread_local_state_clear_all| has
+// write-locked its |state_clear_lock| and intentionally never releases it.
+static int thread_local_list_delete_node(
   struct rand_thread_local_state *node_delete) {
 
   // Mutating the global linked list. Need to synchronize over all threads.
   CRYPTO_STATIC_MUTEX_lock_write(thread_local_states_list_lock_bss_get());
+
+  // Re-check the shutdown flag under the lock. This makes the
+  // "free vs. shutdown-zeroize" decision atomic with respect to
+  // |rand_thread_local_state_clear_all|: either we delete and free before
+  // shutdown begins, or shutdown wins and we leak the node deliberately.
+  if (*thread_local_drbg_shutdown_started_bss_get() != 0) {
+    CRYPTO_STATIC_MUTEX_unlock_write(thread_local_states_list_lock_bss_get());
+    return 0;
+  }
+
   struct rand_thread_local_state *node_head = *thread_states_list_head_bss_get();
 
   // We have [node_delete->previous] <--> [node_delete] <--> [node_delete->next]
@@ -127,6 +181,7 @@ static void thread_local_list_delete_node(
   }
 
   CRYPTO_STATIC_MUTEX_unlock_write(thread_local_states_list_lock_bss_get());
+  return 1;
 }
 
 // thread_local_list_add adds the state |node_add| to the linked list. Note that
@@ -141,6 +196,19 @@ static void thread_local_list_add_node(
   // Mutating the global linked list. Need to synchronize over all threads.
   CRYPTO_STATIC_MUTEX_lock_write(thread_local_states_list_lock_bss_get());
 
+  // If process-wide zeroization has already started, do not add a new state to
+  // the list -- it would not have been zeroized by
+  // |rand_thread_local_state_clear_all|. Instead, write-lock the state's
+  // |state_clear_lock| and never release it. This causes any subsequent
+  // |RAND_bytes_core| call on this thread to block forever on the read lock,
+  // matching the FIPS-derived guarantee that no output is returned after
+  // shutdown zeroization.
+  if (*thread_local_drbg_shutdown_started_bss_get() != 0) {
+    CRYPTO_MUTEX_lock_write(&node_add->state_clear_lock);
+    CRYPTO_STATIC_MUTEX_unlock_write(thread_local_states_list_lock_bss_get());
+    return;
+  }
+
   // First get a reference to the pointer of the head of the linked list.
   // That is, the pointer to the head node node_head is *thread_states_head.
   struct rand_thread_local_state **thread_states_head = thread_states_list_head_bss_get();
@@ -171,7 +239,16 @@ static void rand_thread_local_state_free(void *state_in) {
     return;
   }
 
-  thread_local_list_delete_node(state);
+  // If process-wide shutdown zeroization has begun, the per-state
+  // |state_clear_lock| has been write-locked and is intentionally never
+  // released (so any in-flight |RAND_bytes| call cannot return output from a
+  // zeroized state). |thread_local_list_delete_node| therefore detects this
+  // case under the linked-list lock and refuses to delete; we must then leak
+  // the node, since freeing it would destroy a held mutex. The OS reclaims
+  // the memory at process exit. See issue #3197.
+  if (thread_local_list_delete_node(state) == 0) {
+    return;
+  }
 
   // Potentially, something could kill the thread before an entropy source has
   // been associated to the thread-local randomness generator object.
diff --git a/crypto/fipsmodule/rand/rand_test.cc b/crypto/fipsmodule/rand/rand_test.cc
@@ -21,12 +21,15 @@
 
 #if defined(OPENSSL_THREADS)
 #include <array>
+#include <condition_variable>
+#include <mutex>
 #include <thread>
 #include <vector>
 #endif
 
 #if !defined(OPENSSL_WINDOWS)
 #include <errno.h>
+#include <signal.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
@@ -524,6 +527,88 @@ TEST_F(randTest, MixedUsageMultiThreaded) {
 }
 #endif  // OPENSSL_THREADS
 
+#if defined(OPENSSL_THREADS) && !defined(OPENSSL_WINDOWS) && \
+    !defined(OPENSSL_IOS) && !defined(OPENSSL_WASM) && \
+    !defined(BORINGSSL_UNSAFE_DETERMINISTIC_MODE)
+// Regression test for https://github.com/aws/aws-lc/issues/3197.
+//
+// Pre-fix, |rand_thread_local_state_clear_all| acquired
+// |thread_local_states_list_lock| and intentionally never released it. A
+// thread that had registered a thread-local state and exited *after* shutdown
+// zeroization would deadlock in its TLS destructor while waiting for that
+// lock. On Windows that destructor runs under the loader lock, which
+// indirectly hangs |ExitProcess|. This test reproduces the same lock cycle
+// (the lock is the same on POSIX -- only the loader-lock fallout is
+// Windows-specific) and verifies the child exits cleanly.
+//
+// We cannot run this in-process because shutdown zeroization permanently
+// disables the RNG for the whole process, breaking any later test. We fork a
+// child, run the scenario, and impose a hard timeout in the parent so a
+// regression manifests as a test failure rather than a hung suite.
+TEST_F(randTest, ShutdownDoesNotDeadlockExitingThread) {
+  pid_t child = fork();
+  ASSERT_GE(child, 0) << "fork failed: " << strerror(errno);
+
+  if (child == 0) {
+    // Register a thread-local DRBG state on a worker thread, then trigger
+    // shutdown zeroization on the main thread, then let the worker exit.
+    std::mutex m;
+    std::condition_variable cv;
+    bool shutdown_done = false;
+    bool worker_registered = false;
+
+    std::thread worker([&] {
+      uint8_t buf[16];
+      RAND_bytes(buf, sizeof(buf));
+      {
+        std::lock_guard<std::mutex> lk(m);
+        worker_registered = true;
+      }
+      cv.notify_one();
+
+      std::unique_lock<std::mutex> lk(m);
+      cv.wait(lk, [&] { return shutdown_done; });
+      // Worker now exits. Pre-fix this exit would block forever in the TLS
+      // destructor; post-fix it returns immediately.
+    });
+
+    {
+      std::unique_lock<std::mutex> lk(m);
+      cv.wait(lk, [&] { return worker_registered; });
+    }
+    rand_thread_local_state_clear_all_FOR_TESTING();
+    {
+      std::lock_guard<std::mutex> lk(m);
+      shutdown_done = true;
+    }
+    cv.notify_one();
+    worker.join();
+    _exit(0);
+  }
+
+  // Parent: enforce a timeout so a regression doesn't hang the suite.
+  constexpr int kTimeoutSeconds = 30;
+  for (int i = 0; i < kTimeoutSeconds * 10; i++) {
+    int status;
+    pid_t ret = waitpid(child, &status, WNOHANG);
+    ASSERT_GE(ret, 0) << "waitpid failed: " << strerror(errno);
+    if (ret == child) {
+      ASSERT_TRUE(WIFEXITED(status))
+          << "child terminated abnormally, status=" << status;
+      ASSERT_EQ(WEXITSTATUS(status), 0);
+      return;
+    }
+    usleep(100 * 1000);
+  }
+
+  kill(child, SIGKILL);
+  waitpid(child, nullptr, 0);
+  FAIL() << "child did not exit within " << kTimeoutSeconds
+         << "s (likely a deadlock in TLS destructor after shutdown)";
+}
+#endif  // OPENSSL_THREADS && !OPENSSL_WINDOWS && !OPENSSL_IOS &&
+        // !OPENSSL_WASM && !BORINGSSL_UNSAFE_DETERMINISTIC_MODE
+
 #if defined(OPENSSL_X86_64) && defined(SUPPORTS_ABI_TEST)
 TEST_F(randTest, RdrandABI) {
   if (!have_hw_rng_x86_64_for_testing()) {
diff --git a/crypto/x509/v3_ncons.c b/crypto/x509/v3_ncons.c
@@ -686,6 +686,21 @@ static int nc_uri(const ASN1_IA5STRING *uri, const ASN1_IA5STRING *base) {
     return X509_V_ERR_UNSUPPORTED_NAME_SYNTAX;
   }
 
+  // RFC 3986 §3.2 defines authority = [userinfo "@"] host [":" port].
+  // Certificate SAN URIs have no standards-defined use for userinfo. If present,
+  // the '@' delimiter would cause the host extraction below to parse the
+  // userinfo as the host, matching against attacker-chosen bytes instead of
+  // the URI's actual host.
+  for (size_t i = 0; i < CBS_len(&uri_cbs); i++) {
+    uint8_t c = CBS_data(&uri_cbs)[i];
+    if (c == '/' || c == '?' || c == '#') {
+      break;
+    }
+    if (c == '@') {
+      return X509_V_ERR_UNSUPPORTED_NAME_SYNTAX;
+    }
+  }
+
   // Look for a port indicator as end of hostname first. Otherwise look for
   // trailing slash, or the end of the string.
   CBS host;
@@ -698,6 +713,35 @@ static int nc_uri(const ASN1_IA5STRING *uri, const ASN1_IA5STRING *base) {
     return X509_V_ERR_UNSUPPORTED_NAME_SYNTAX;
   }
 
+  // Normalize absolute DNS names by removing the trailing dot, if any.
+  // RFC 1034 §3.1 defines that a trailing dot denotes the DNS root; names with
+  // and without it refer to the same host. This matches the normalization in
+  // nc_dns().
+  if (ends_with_byte(&host, '.')) {
+    uint8_t unused;
+    CBS_get_last_u8(&host, &unused);
+  }
+  if (ends_with_byte(&base_cbs, '.')) {
+    uint8_t unused;
+    CBS_get_last_u8(&base_cbs, &unused);
+  }
+
+  // RFC 5280 §4.2.1.10 requires the host to be a fully qualified domain name.
+  // Validate that the host contains only characters valid in a DNS name
+  // (RFC 1034 §3.5): letters, digits, hyphens, and dots. This rejects
+  // percent-encoded hosts and any other non-FQDN syntax, preventing
+  // equivalence bypasses (e.g., "b%61d.com" evading ".bad.com").
+  if (CBS_len(&host) == 0) {
+    return X509_V_ERR_UNSUPPORTED_NAME_SYNTAX;
+  }
+  for (size_t i = 0; i < CBS_len(&host); i++) {
+    uint8_t c = CBS_data(&host)[i];
+    if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') ||
+          (c >= '0' && c <= '9') || c == '-' || c == '.')) {
+      return X509_V_ERR_UNSUPPORTED_NAME_SYNTAX;
+    }
+  }
+
   // Special case: initial '.' is RHS match
   if (starts_with(&base_cbs, '.')) {
     if (has_suffix_case(&host, &base_cbs)) {
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
@@ -2640,6 +2640,68 @@ TEST(X509Test, NameConstraints) {
       // An incomplete IPv6 literal is also rejected.
       {GEN_URI, "foo://[2001:db8::1", "[2001:db8::1]",
        X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+
+      // RFC 3986 §3.2 defines authority = [userinfo "@"] host [":" port].
+      // URIs with userinfo are rejected to prevent host confusion: without
+      // this, "spiffe://x.team-a.corp:x@team-b.corp/admin" would be matched
+      // against "x.team-a.corp" instead of the actual host "team-b.corp",
+      // bypassing permittedSubtrees or evading excludedSubtrees.
+      //
+      // Basic userinfo before host.
+      {GEN_URI, "foo://user@example.com", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      // Userinfo with colon (user:password style) — the colon in userinfo
+      // would previously be mistaken for a port delimiter, extracting the
+      // userinfo prefix as the host.
+      {GEN_URI, "spiffe://x.team-a.corp:x@team-b.corp/admin", "team-a.corp",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      {GEN_URI, "spiffe://x.team-a.corp:x@team-b.corp/admin", "team-b.corp",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      // Userinfo with path, query, and fragment components.
+      {GEN_URI, "foo://user@example.com/path", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      {GEN_URI, "foo://user@example.com?query", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      {GEN_URI, "foo://user@example.com#fragment", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      // '@' in path (after '/') is not userinfo and should not be rejected.
+      {GEN_URI, "foo://example.com/@user", "example.com", X509_V_OK,
+       X509_V_ERR_EXCLUDED_VIOLATION},
+      {GEN_URI, "foo://example.com/path@thing", ".example.com",
+       X509_V_ERR_PERMITTED_VIOLATION, X509_V_OK},
+      // '@' after '?' or '#' is not in the authority and is not rejected.
+      // However, since the host parser doesn't treat '?' or '#' as authority
+      // terminators, the extracted host contains invalid FQDN characters and
+      // is rejected by FQDN validation.
+      {GEN_URI, "foo://example.com?x@y", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      {GEN_URI, "foo://example.com#x@y", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      // Empty userinfo and user:pass userinfo are both rejected.
+      {GEN_URI, "foo://@example.com", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      {GEN_URI, "foo://user:pass@example.com", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+
+      // Trailing dot normalization: "host." and "host" are the same FQDN.
+      // This matches the normalization in nc_dns().
+      {GEN_URI, "spiffe://admin.team-a.corp./admin", ".team-a.corp",
+       X509_V_OK, X509_V_ERR_EXCLUDED_VIOLATION},
+      {GEN_URI, "spiffe://admin.team-a.corp/admin", ".team-a.corp.",
+       X509_V_OK, X509_V_ERR_EXCLUDED_VIOLATION},
+      {GEN_URI, "spiffe://team-a.corp./admin", "team-a.corp",
+       X509_V_OK, X509_V_ERR_EXCLUDED_VIOLATION},
+
+      // FQDN validation: hosts must contain only a-z, A-Z, 0-9, '-', '.'.
+      // Percent-encoded hosts are rejected, preventing equivalence bypasses
+      // (e.g., "b%61d.com" should not evade an exclusion for ".bad.com").
+      {GEN_URI, "spiffe://evil.b%61d.com/resource", ".bad.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      {GEN_URI, "foo://ex%61mple.com/path", "example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
+      // Underscores are not valid in FQDNs.
+      {GEN_URI, "foo://my_host.example.com/path", ".example.com",
+       X509_V_ERR_UNSUPPORTED_NAME_SYNTAX, X509_V_ERR_UNSUPPORTED_NAME_SYNTAX},
   };
   for (const auto &t : kTests) {
     SCOPED_TRACE(t.type);
