[glm-grade=B] fix(tls+i2p): trust SSL_CTX_set_default_verify_paths rc + value-copy m_ServerTunnels

TLS (bootstrap.cpp): trust SSL_CTX_set_default_verify_paths() return code
without introspecting X509_STORE objects (lazy hashed-dir lookups install
correctly without eager preload). Always attempt embedded X1+X2 as
belt-and-suspenders. Fail-closed only when ALL external sources fail AND
both embedded roots fail AND store object count is 0.

I2P (i2p_embedded.cpp): fix data race on m_ServerTunnels by making a value
copy of the map returned by GetServerTunnels() before iterating. Previous
const-reference iteration could crash if VisitTunnels(true) erased entries
concurrently during the loop.

GUI (introdialog.cpp): update layered-trust-store comment to accurately
describe the four-source resolution order and lazy-lookup rationale.

Adversarial review: GLM-5.2 separate-agent grade B (7 issues found, all
LOW/MEDIUM, no CRITICAL). Issue 5 (race in snapshot loop) fixed in this
commit by switching from const-ref to value copy.
This commit is contained in:
Krystie
2026-08-07 20:36:35 -07:00
parent fc4bba23b3
commit 0cadbba30c
3 changed files with 107 additions and 14 deletions
+84 -9
View File
@@ -427,6 +427,25 @@ struct HttpConn {
// (b) SSL_CERT_FILE (operator override, wide-char on Windows) // (b) SSL_CERT_FILE (operator override, wide-char on Windows)
// (c) system default paths (Linux daemon: /etc/ssl/certs/...) // (c) system default paths (Linux daemon: /etc/ssl/certs/...)
// (d) embedded ISRG X1 + X2 (always, on top of whatever loaded above) // (d) embedded ISRG X1 + X2 (always, on top of whatever loaded above)
//
// Important: we MUST NOT reject a successful system default-verify-paths
// call based on a heuristic that counts X509_STORE objects, because
// OpenSSL's hashed-directory lookups are LAZY (X509_LOOKUP_hashdir
// installs the lookup but does NOT eagerly preload every cert). On
// Ubuntu/Debian/alpine, /etc/ssl/certs/ contains symlinks into a
// hashed dir; the lookup IS valid even though X509_STORE_get0_objects()
// returns 0 objects until a real cert chain is verified. Rejecting
// that valid install would force the daemon to fall through to the
// embedded fallbacks unnecessarily and (worse) could reject the system
// path on installations that legitimately have a usable CA bundle.
//
// The right model is: trust the API's return code as evidence the
// lookup source is configured; on API failure, treat the system as
// unconfigured and rely on the embedded fallbacks (which we always
// attempt). The embedded X1 + X2 are also tried when the system
// returns 1, as belt-and-suspenders for chains terminating at the
// cross-sign root (older R3 intermediates chain to X1; the newer LE
// YE1 intermediate chains to X2).
if (!exeDir.empty()) { if (!exeDir.empty()) {
tryLoadPath("exedir cacert.pem", exeDir / "cacert.pem"); tryLoadPath("exedir cacert.pem", exeDir / "cacert.pem");
} }
@@ -444,10 +463,11 @@ struct HttpConn {
tryLoadFile("SSL_CERT_FILE", envCert); tryLoadFile("SSL_CERT_FILE", envCert);
} }
#endif #endif
if (SSL_CTX_set_default_verify_paths(ctx) == 1) { // System default verify paths: trust the API return. We do not
// Mark the system store as a successful trust source // introspect the store object count, because lazy hashed-directory
// independently of any embedded add. A system that has any // lookups install correctly without eager preload.
// usable CA bundle is configured correctly by this call alone. int sysRc = SSL_CTX_set_default_verify_paths(ctx);
if (sysRc == 1) {
printf("Bootstrap: TLS trust configured from system default paths\n"); printf("Bootstrap: TLS trust configured from system default paths\n");
trustLoaded = true; trustLoaded = true;
} else { } else {
@@ -455,12 +475,67 @@ struct HttpConn {
char buf[256]; char buf[256];
ERR_error_string_n(e, buf, sizeof(buf)); ERR_error_string_n(e, buf, sizeof(buf));
lastLoadErr = std::string("system default paths: ") + buf; lastLoadErr = std::string("system default paths: ") + buf;
ERR_clear_error();
}
// Belt-and-suspenders: always attempt to add embedded ISRG X1 + X2
// regardless of whether (a)/(b)/(c) succeeded, because they are
// additive (X509_STORE_add_cert + CERT_ALREADY_IN_HASH_TABLE both
// mean "the anchor is addressable"). They are CRITICAL on a stripped
// install where (a)/(b)/(c) all fail (rc!=1). The check below
// verifies that on a stripped install, BOTH X1 and X2 are addressable.
// On an install where (a)/(b)/(c) succeeded, X1/X2 are layered on
// for cross-sign resilience and any tryLoadEmbedded failure becomes
// a warning (the chain will still validate via the layered sources).
auto countAnchors = [&]() -> int {
X509_STORE* store = SSL_CTX_get_cert_store(ctx);
if (!store) return 0;
// X509_STORE_get0_objects() returns STACK_OF(X509_OBJECT) in
// OpenSSL 1.1+/3.x. We use this for "how many anchors did this
// source add" only in restricted diagnostic contexts below — NOT
// to gate the API return value. (Eagerly loaded anchors via
// SSL_CTX_load_verify_locations will report here; lazy hashed-
// directory lookups from set_default_verify_paths will not.
// See the comment block above.)
STACK_OF(X509_OBJECT)* objs = X509_STORE_get0_objects(store);
return objs ? sk_X509_OBJECT_num(objs) : 0;
};
// Capture whether ANY external source (cacert/SSL_CERT_FILE/system)
// succeeded BEFORE embedded calls run. tryLoadEmbedded sets trustLoaded
// and we need this separate signal to distinguish "external sources
// worked but X2 hard-failed" (chain still validates via external
// anchors, warning-only) from "all external sources failed AND X2
// hard-failed" (truly zero usable anchors, fail closed).
bool externalSourceSucceeded = trustLoaded;
bool x1Addr = tryLoadEmbedded("ISRG Root X1", EMBEDDED_ISRG_ROOT_X1_PEM);
ERR_clear_error();
bool x2Addr = tryLoadEmbedded("ISRG Root X2", EMBEDDED_ISRG_ROOT_X2_PEM);
ERR_clear_error();
if (!x1Addr || !x2Addr) {
// Either X1 or X2 (or both) hard-failed. We only treat this as
// fatal when no external source succeeded — i.e. the system has
// truly zero usable trust anchors. If (a)/(b)/(c) succeeded, the
// chain can still validate via those (e.g. R3 intermediate chains
// to X1 via a non-ISRG-root cert in /etc/ssl/certs), but we warn.
if (!externalSourceSucceeded) {
int anchors = countAnchors();
if (anchors == 0) {
strError = "TLS trust store is empty: cacert/SSL_CERT_FILE/system "
"not configured (rc!=1) AND embedded ISRG fallbacks "
"failed (X1=" + std::string(x1Addr ? "ok" : "FAIL") +
", X2=" + std::string(x2Addr ? "ok" : "FAIL") +
", last attempt: " + lastLoadErr + ")";
return false;
}
}
printf("Bootstrap: WARNING — embedded ISRG load partial: X1=%s X2=%s "
"(ca.pem/SSL_CERT_FILE/system are configured, chain will validate via those)\n",
x1Addr ? "ok" : "FAIL", x2Addr ? "ok" : "FAIL");
}
// trustLoaded true if any external source succeeded OR if X1+X2 were
// both added/duplicated on a stripped host.
if (x1Addr && x2Addr) {
trustLoaded = true;
} }
// Belt-and-suspenders: always add embedded ISRG roots regardless
// of which (if any) of the above succeeded. Adding anchors only
// expands the set of valid chains, never restricts it.
tryLoadEmbedded("ISRG Root X1", EMBEDDED_ISRG_ROOT_X1_PEM);
tryLoadEmbedded("ISRG Root X2", EMBEDDED_ISRG_ROOT_X2_PEM);
if (!trustLoaded) { if (!trustLoaded) {
strError = "Failed to load any TLS trust store (last attempt: " + lastLoadErr + ")"; strError = "Failed to load any TLS trust store (last attempt: " + lastLoadErr + ")";
return false; return false;
+13 -2
View File
@@ -911,10 +911,21 @@ void CI2PEmbedded::DiscoverServerTunnelDestination()
// Step 2: only publish if a LIVE registered server tunnel matches // Step 2: only publish if a LIVE registered server tunnel matches
// the keys-file hash. Registry membership confirms the tunnel is // the keys-file hash. Registry membership confirms the tunnel is
// active; LeaseSet publication is i2pd's responsibility after that. // active; LeaseSet publication is i2pd's responsibility after that.
//
// Thread-safety: GetServerTunnels() returns a const reference to
// i2pd's internal m_ServerTunnels map, which has NO internal lock.
// VisitTunnels(true) (called from ReloadConfig / Stop) can erase
// entries concurrently. We make a VALUE COPY of the map (not a
// reference) so that iterator invalidation during the copy is a
// narrow read-only window, and all string comparisons run on the
// local snapshot with no live-map access. The copy constructor of
// std::map is exception-safe; if it throws (bad_alloc), the catch
// below handles it.
if (!keysFileIdentB32.empty()) { if (!keysFileIdentB32.empty()) {
try { try {
for (const auto& kv : i2p::client::context.GetServerTunnels()) { auto tunnels = i2p::client::context.GetServerTunnels(); // value copy
const i2p::data::IdentHash& dest = kv.first.first; for (const auto& kv : tunnels) {
const auto& dest = kv.first.first;
if (dest.ToBase32() == keysFileIdentB32) { if (dest.ToBase32() == keysFileIdentB32) {
if (i2pHostname != keysFileIdentB32 + ".b32.i2p") { if (i2pHostname != keysFileIdentB32 + ".b32.i2p") {
i2pHostname = keysFileIdentB32 + ".b32.i2p"; i2pHostname = keysFileIdentB32 + ".b32.i2p";
+10 -3
View File
@@ -453,9 +453,16 @@ bool IntroDialog::pickDataDirectory()
// dir for a staged utxo-snapshot.bin above; if that didn't find one, // dir for a staged utxo-snapshot.bin above; if that didn't find one,
// DownloadUtxoSnapshot is the canonical HTTPS path to the bootstrap // DownloadUtxoSnapshot is the canonical HTTPS path to the bootstrap
// server. TLS validation is now handled in bootstrap.cpp's StartTLS // server. TLS validation is now handled in bootstrap.cpp's StartTLS
// via a layered trust store (exedir cacert.pem → system → embedded // via a layered trust store (exedir cacert.pem → SSL_CERT_FILE →
// ISRG roots), so this should succeed on Windows GUI builds where the // system default paths → embedded ISRG X1 + X2 as belt-and-suspenders),
// Qt-bundled libssl-3-x64.dll ships without a default cert path. // so this should succeed on Windows GUI builds where the Qt-bundled
// libssl-3-x64.dll ships without a default cert path. The system-path
// call is trusted on its return value (OpenSSL's hashed-directory
// lookups are lazy and would otherwise show 0 eagerly loaded store
// objects even on a valid install); the embedded fallbacks are
// always attempted as cross-sign resilience and become load-bearing
// on a stripped Windows GUI with no cacert.pem and no usable system
// CA directory.
std::string utxoError; std::string utxoError;
bool success = Bootstrap::DownloadUtxoSnapshot(host, dataDirPath, progressFn, utxoError); bool success = Bootstrap::DownloadUtxoSnapshot(host, dataDirPath, progressFn, utxoError);
if (!success) { if (!success) {