4 follow-up patches shipped: wildfly-0002 + wildfly-0003 + log4j2-0001 + nakama-0001
Acting on the 4 borderline candidates flagged in the session-summary intel. All 4 surfaced after the unmoad scanner enhancements cleared M3/M4 noise. UNDF-1306 wildfly-0002 (HIGH) - ElytronSecurityDomainContextImpl.isValid() sets currentIdentity ThreadLocal with no paired cleanup contract. Subject populated at line 69 is the canonical handover; the ThreadLocal stash leaks to next request on the pool thread. Fix: drop the .set(identity) line. UNDF-1307 wildfly-0003 (LOW) - TransactionRollbackSetupAction.depth.set(null) should be depth.remove() to fully delete the ThreadLocal entry; current pattern leaves null binding pinning the WildFly classloader during undeploy/redeploy. Functional clear, classloader-retention only. UNDF-1308 log4j2-0001 (HIGH) - Log4jMDCAdapter.clear() only clears the log4j ThreadContext map, NOT the SLF4J pushByKey/popByKey stacks (mapOfStacks ThreadLocal). SLF4J spec mandates clear() means "clear all MDC". Per-key Deques accumulate across requests. Fix: add clear() to ThreadLocalMapOfStacks (calls tlMapOfStacks.remove()) and call from the public clear(). UNDF-1309 nakama-0001 (HIGH MOAD-0004) - social/social.go logs OAuth access tokens, ID tokens, oauth2.Token objects (incl. refresh tokens), Steam publisherKey + ticket at debug level via zap.String/zap.Any. 11 call sites. Fix: replace value-logging with shape-logging (token_len, has_token bool) — preserves debug value, redacts secret bytes. First MOAD-0004 patch this session. Companion to the 3 MOAD-0003 patches (wildfly-0001/0002/0003) extending the inverse-pipeline pattern across projects: scanner enhancement -> noise reduction -> human triage finds defects that were buried. Total session flagships: 9 (was 6) — 5 CWE-407 + 3 MOAD-0003 + 1 MOAD-0004.
This commit is contained in:
parent
d149f2aaf0
commit
2eea7d128c
11 changed files with 771 additions and 0 deletions
76
docs/tickets/log4j2-0001-mdcadapter-clear-leaks-stacks.md
Normal file
76
docs/tickets/log4j2-0001-mdcadapter-clear-leaks-stacks.md
Normal file
|
|
@ -0,0 +1,76 @@
|
|||
# log4j2-0001: Log4jMDCAdapter.clear() leaves SLF4J pushByKey/popByKey stacks bound to thread
|
||||
|
||||
**Target:** apache/logging-log4j2
|
||||
**Severity:** HIGH
|
||||
**CWE:** CWE-668 (Exposure of Resource to Wrong Sphere)
|
||||
**MOAD:** MOAD-0003 (A Leaked Context)
|
||||
**File:** `log4j-slf4j2-impl/src/main/java/org/apache/logging/slf4j/Log4jMDCAdapter.java:55-57, 118-150`
|
||||
**Language:** Java
|
||||
**Status:** open
|
||||
|
||||
## Description
|
||||
|
||||
`Log4jMDCAdapter` (the SLF4J→log4j MDC bridge) maintains TWO per-thread state holders:
|
||||
|
||||
1. **`ThreadContext` map** — log4j-core's canonical MDC, the one most callers think of as "MDC"
|
||||
2. **`mapOfStacks: ThreadLocalMapOfStacks`** (line 37) — the SLF4J adapter's own `ThreadLocal<Map<String, Deque<String>>>` carrying the per-key stack semantics that SLF4J added with `pushByKey/popByKey/peekByKey/clearByKey/getCopyOfDequeByKey`
|
||||
|
||||
The `clear()` method:
|
||||
|
||||
```java
|
||||
@Override
|
||||
public void clear() {
|
||||
ThreadContext.clearMap(); // clears holder #1 only
|
||||
// mapOfStacks is NOT cleared — silently leaks to the next request
|
||||
}
|
||||
```
|
||||
|
||||
SLF4J's `MDC.clear()` spec mandates "clear all MDC state for this thread." Web frameworks (Spring, Quarkus, etc.) call `MDC.clear()` between requests in pool-thread environments. With log4j-slf4j2-impl, the per-key Deques accumulate. A subsequent `peekByKey()` / `popByKey()` / `getCopyOfDequeByKey()` for a key set by a prior request returns the prior request's value.
|
||||
|
||||
## Severity Note
|
||||
|
||||
If application code uses `pushByKey/popByKey` to track per-request state (tenant IDs, trace contexts, user roles, auth tokens), Request B sees Request A's leftover stack contents on a re-used pool thread. Defense-in-depth failure when MDC is used for security-sensitive identifiers.
|
||||
|
||||
Bounded blast radius: only affects callers using SLF4J's stack-API (`pushByKey/popByKey`) — the most common `MDC.put/get` API users are unaffected because that path goes through log4j ThreadContext (which IS cleared).
|
||||
|
||||
## Root Cause
|
||||
|
||||
```java
|
||||
// Log4jMDCAdapter.java:54-57
|
||||
@Override
|
||||
public void clear() {
|
||||
ThreadContext.clearMap();
|
||||
}
|
||||
// ThreadLocalMapOfStacks (lines 118-150) has clearByKey() but no clear-all.
|
||||
```
|
||||
|
||||
## Fix
|
||||
|
||||
Two-line change:
|
||||
|
||||
1. Add a `clear()` method to `ThreadLocalMapOfStacks` that calls `tlMapOfStacks.remove()` (also addresses minor classloader-retention in app-server thread pools).
|
||||
2. Call `mapOfStacks.clear()` from the public `Log4jMDCAdapter.clear()`.
|
||||
|
||||
```java
|
||||
@Override
|
||||
public void clear() {
|
||||
ThreadContext.clearMap();
|
||||
mapOfStacks.clear(); // <-- new
|
||||
}
|
||||
|
||||
private static class ThreadLocalMapOfStacks {
|
||||
// ... existing methods ...
|
||||
|
||||
public void clear() {
|
||||
tlMapOfStacks.remove(); // <-- new
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`tlMapOfStacks.remove()` (vs `set(new HashMap<>())`) deletes the ThreadLocal entry, preventing classloader retention in app-server pools across application redeploys.
|
||||
|
||||
## Discovery context
|
||||
|
||||
Surfaced after unmoad scanner enhancement `1f48798` (Java ThreadLocal-scoped `.set()` leak detection) cleared 79% of log4j2's M3 false-positive noise. Manual triage of the residual 95 findings identified this as the only flagship-grade defect; the other 94 are layout/StringBuilder buffer ThreadLocals that are intentional performance caches with no value-leak risk.
|
||||
|
||||
This is the first patch from a project that joined the clean-scan honor roll in our Wave 23 survey — the inverse-pipeline pattern (scanner improves SNR → triage finds defect that previously was invisible) holds beyond WildFly.
|
||||
|
|
@ -0,0 +1,96 @@
|
|||
# nakama-0001: social.go logs OAuth access tokens, Steam publisher key, signed-player-info at debug level
|
||||
|
||||
**Target:** heroiclabs/nakama
|
||||
**Severity:** HIGH
|
||||
**CWE:** CWE-532 (Insertion of Sensitive Information into Log File)
|
||||
**MOAD:** MOAD-0004 (A Logged Secret)
|
||||
**File:** `social/social.go:235, 250, 290, 353, 435, 439, 443, 448, 452, 630`
|
||||
**Language:** Go
|
||||
**Status:** open
|
||||
|
||||
## Description
|
||||
|
||||
Nakama's social-auth client at `social/social.go` has 11 debug-level `zap.Field` call sites that log third-party authentication SECRETS as full string/object values:
|
||||
|
||||
| Line | Provider | Logged secret |
|
||||
|------|----------|---------------|
|
||||
| 235 | Facebook | `accessToken` (full Graph API access) |
|
||||
| 250 | Facebook | `accessToken` (friends scope) |
|
||||
| 290 | Facebook Instant Game | `signedPlayerInfo` (HMAC-signed authenticator) |
|
||||
| 353 | Google | `idToken` (user identity assertion) |
|
||||
| 435 | Google | `auth_token` = `idToken` (auth-code-exchange retry) |
|
||||
| 439 | Google | `oauth2.Token` object `t` (incl. AccessToken AND RefreshToken) |
|
||||
| 443 | Google | `oauth2.Token` object `t` |
|
||||
| 448 | Google | `oauth2.Token` object `t` |
|
||||
| 452 | Google | `oauth2.Token` object `t` (success path) |
|
||||
| 630 | Steam | `publisherKey` (developer's Steam web API key) + `ticket` |
|
||||
|
||||
Game-server operators run nakama with debug logging enabled in development and frequently leave it on in production. Log files routed to centralized aggregators (ELK, Datadog, Loki, Sumo) inherit the leaked tokens and become a credential exfiltration target.
|
||||
|
||||
## Severity Note
|
||||
|
||||
| Token type | Impact if leaked |
|
||||
|------------|------------------|
|
||||
| Facebook accessToken | Full Graph API access for the user — read profile, post, message friends |
|
||||
| Google idToken | User identity assertion; can be reused against APIs that accept ID tokens directly |
|
||||
| `*oauth2.Token` (full object) | Contains `AccessToken` AND `RefreshToken`; refresh token grants long-lived backend access |
|
||||
| Steam `publisherKey` | The SERVER's Steam web API key — compromise gives full access to the developer's Steam app/inventory APIs |
|
||||
| Game Center `signedPlayerInfo` / signature | Per-player authenticator strings; useful for replay attacks |
|
||||
|
||||
These are all credential-equivalents. CWE-532 applies; CWE-209 (info exposure through error messages) applies to the failure-path log lines (435, 443, 448).
|
||||
|
||||
## Root Cause
|
||||
|
||||
Pattern across all sites: `zap.String("token", X)` or `zap.Any("token", X)` directly logs the secret as a structured field value. Once persisted to a log sink, the secret is now wherever logs go.
|
||||
|
||||
```go
|
||||
// L235
|
||||
c.logger.Debug("Getting Facebook profile", zap.String("token", accessToken))
|
||||
|
||||
// L439
|
||||
c.logger.Debug("Exchanged an authorization code for an access token.",
|
||||
zap.Any("token", t), zap.Error(err))
|
||||
```
|
||||
|
||||
## Fix
|
||||
|
||||
Replace value logging with shape logging — log the FACT that we had a token (and its length) without logging the bytes. Standard CWE-532 remediation pattern.
|
||||
|
||||
```go
|
||||
// Before
|
||||
c.logger.Debug("Getting Facebook profile", zap.String("token", accessToken))
|
||||
// After
|
||||
c.logger.Debug("Getting Facebook profile", zap.Int("token_len", len(accessToken)))
|
||||
```
|
||||
|
||||
```go
|
||||
// Before
|
||||
c.logger.Debug("Exchanged an authorization code for an access token.",
|
||||
zap.Any("token", t), zap.Error(err))
|
||||
// After
|
||||
c.logger.Debug("Exchanged an authorization code for an access token.",
|
||||
zap.Bool("has_token", t != nil), zap.Error(err))
|
||||
```
|
||||
|
||||
For Steam `publisherKey` + `ticket`, the right fix is to redact entirely; neither presence nor length is a useful debug signal here (the request will fail visibly if the key is missing). The non-secret `appID` and `errorDescription` fields already provide debug value:
|
||||
|
||||
```go
|
||||
// Before
|
||||
c.logger.Debug("Getting Steam profile",
|
||||
zap.String("publisherKey", publisherKey),
|
||||
zap.Int("appID", appID),
|
||||
zap.String("ticket", ticket))
|
||||
// After
|
||||
c.logger.Debug("Getting Steam profile",
|
||||
zap.Int("appID", appID),
|
||||
zap.Int("publisherKey_len", len(publisherKey)),
|
||||
zap.Int("ticket_len", len(ticket)))
|
||||
```
|
||||
|
||||
Patch covers 11 call sites; preserves debug value (can confirm whether the call site received a non-empty token) without leaking bytes.
|
||||
|
||||
## Discovery context
|
||||
|
||||
Documented in Wave 22 survey (`/wave22-eda-games-hpc-codecs-httpd-survey/`) as the only real MOAD-0004 finding in that wave. Routed to MOAD-0004 disclosure pipeline (this is the first M4 patch shipped this autonomous-loop session).
|
||||
|
||||
Note: nakama's Wave 22 entry was **excluded from the clean-scan honor roll** because of these real M4 findings — joining the roll requires zero real defects, not just zero false positives. Honor roll status: pending fix-acceptance upstream.
|
||||
|
|
@ -0,0 +1,85 @@
|
|||
# wildfly-0002: ElytronSecurityDomainContextImpl.isValid() ThreadLocal currentIdentity leak
|
||||
|
||||
**Target:** wildfly/wildfly
|
||||
**Severity:** HIGH
|
||||
**CWE:** CWE-668 (Exposure of Resource to Wrong Sphere)
|
||||
**MOAD:** MOAD-0003 (A Leaked Context)
|
||||
**File:** `webservices/server-integration/src/main/java/org/jboss/as/webservices/security/ElytronSecurityDomainContextImpl.java:68`
|
||||
**Language:** Java
|
||||
**Status:** open
|
||||
|
||||
## Description
|
||||
|
||||
`ElytronSecurityDomainContextImpl` is the WildFly Elytron bridge for JBossWS web service security. The class has three call sites that set the per-thread `currentIdentity` ThreadLocal:
|
||||
|
||||
| Line | Method | Has paired cleanup? |
|
||||
|-----:|--------|--------------------|
|
||||
| 68 | `isValid(Principal, password, Subject)` | **NO** — leak point |
|
||||
| 80 | `runAs(Callable)` | yes — clears in `try/finally` (same method) |
|
||||
| 96 | `pushSubjectContext(Subject, Principal, credential)` | yes — paired with `cleanupSubjectContext()` (line 132) |
|
||||
|
||||
`isValid()` validates credentials and populates the caller's `Subject` (line 69). Once it returns true, the caller already has the SecurityIdentity inside the populated Subject — there's no need to also stash a copy in the per-thread `currentIdentity`. JBossWS / Apache CXF callers that use `isValid()` purely for credential validation (without proceeding to `runAs()` or `pushSubjectContext()`/`cleanupSubjectContext()`) leak the prior request's SecurityIdentity into the next Work item on the same pool thread.
|
||||
|
||||
## Root Cause
|
||||
|
||||
```java
|
||||
// ElytronSecurityDomainContextImpl.java:55-71
|
||||
@Override
|
||||
public boolean isValid(Principal principal, Object password, Subject subject) {
|
||||
if (subject == null) {
|
||||
subject = new Subject();
|
||||
}
|
||||
String username = principal.getName();
|
||||
if (!(password instanceof String)) {
|
||||
throw WSLogger.ROOT_LOGGER.onlyStringPasswordAccepted();
|
||||
}
|
||||
SecurityIdentity identity = authenticate(username, (String) password);
|
||||
if (identity == null) {
|
||||
return false;
|
||||
}
|
||||
this.currentIdentity.set(identity); // <-- LEAK: no paired cleanup
|
||||
SubjectUtil.fromSecurityIdentity(identity, subject);
|
||||
return true;
|
||||
}
|
||||
```
|
||||
|
||||
## Severity Note
|
||||
|
||||
Same MOAD-0003 family as wildfly-0001 (UNDF-1305). Multi-tenant JBossWS deployments where SOAP/REST endpoints sit behind a Work-Manager-pooled execution model are exposed: thread N processes Request A's `isValid()` (sets `currentIdentity = Alice`), returns to the pool, then processes Request B which reads `getSecurityContext()` (or downstream code that consults `currentIdentity.get()`) before `setSecurityContext()` overwrites it.
|
||||
|
||||
Defense-in-depth value: even if all current JBossWS callers happen to follow up with `pushSubjectContext()` (which would overwrite the leak), any future caller that uses `isValid()` purely for "is this user/password valid?" intent — without intending to run subsequent work under that identity — silently leaks.
|
||||
|
||||
## Fix
|
||||
|
||||
Drop the `this.currentIdentity.set(identity)` line in `isValid()`. The Subject populated at line 69 remains the canonical handover for credential-validation callers. Callers that actually need the per-thread identity install should use `pushSubjectContext()` (paired with `cleanupSubjectContext()`) or `runAs()` (auto-cleared in finally).
|
||||
|
||||
```java
|
||||
@Override
|
||||
public boolean isValid(Principal principal, Object password, Subject subject) {
|
||||
if (subject == null) {
|
||||
subject = new Subject();
|
||||
}
|
||||
String username = principal.getName();
|
||||
if (!(password instanceof String)) {
|
||||
throw WSLogger.ROOT_LOGGER.onlyStringPasswordAccepted();
|
||||
}
|
||||
SecurityIdentity identity = authenticate(username, (String) password);
|
||||
if (identity == null) {
|
||||
return false;
|
||||
}
|
||||
// Removed: this.currentIdentity.set(identity);
|
||||
// Subject already carries the identity for the caller. ThreadLocal
|
||||
// installation belongs in pushSubjectContext()/cleanupSubjectContext()
|
||||
// or runAs() — never in a credential-validator without paired cleanup.
|
||||
SubjectUtil.fromSecurityIdentity(identity, subject);
|
||||
return true;
|
||||
}
|
||||
```
|
||||
|
||||
## Companion to wildfly-0001
|
||||
|
||||
Same project, same MOAD, different entry point. Both finds surfaced after unmoad scanner commit `1f48798` cleared 99.2% of the WildFly M3 noise that had previously buried them. wildfly-0001 fixed the Elytron-JCA bridge; wildfly-0002 fixes the Elytron-JBossWS bridge.
|
||||
|
||||
## Why this surfaced now
|
||||
|
||||
This is the second M3 finding extracted from the cleaned wildfly M3 surface (4840 → 37 after `1f48798`). Manual triage of the residual 37 found this and wildfly-0001 as the two clear flagship-grade defects; the other 35 are properly cleaned ThreadLocals (try/finally pairs) or intentional state toggles.
|
||||
|
|
@ -0,0 +1,63 @@
|
|||
# wildfly-0003: TransactionRollbackSetupAction uses depth.set(null) instead of depth.remove()
|
||||
|
||||
**Target:** wildfly/wildfly
|
||||
**Severity:** LOW
|
||||
**CWE:** CWE-668 (Exposure of Resource to Wrong Sphere)
|
||||
**MOAD:** MOAD-0003 (A Leaked Context — minor)
|
||||
**File:** `transactions/src/main/java/org/jboss/as/txn/deployment/TransactionRollbackSetupAction.java:102`
|
||||
**Language:** Java
|
||||
**Status:** open
|
||||
|
||||
## Description
|
||||
|
||||
`TransactionRollbackSetupAction` tracks transaction depth per thread in a `ThreadLocal<Holder>`. When the depth counter hits zero, the code does `depth.set(null)` to "clear" the holder. This functionally works (the next caller's `depth.get()` returns null and re-initializes), but it leaves the underlying ThreadLocal entry alive in the thread's internal `threadLocals` map.
|
||||
|
||||
In Java EE app servers with persistent thread pools across application lifecycles, every `set(null)` accumulates an entry that:
|
||||
1. Pins the WildFly classloader of the deployed application (the ThreadLocal key reference)
|
||||
2. Survives `undeploy` / `redeploy`, growing the thread's `threadLocals` map slowly
|
||||
3. Prevents the `Holder` class from being unloaded along with its application classloader
|
||||
|
||||
`ThreadLocal.remove()` actually deletes the entry, allowing both the holder and the application classloader to be reclaimed during deployment churn.
|
||||
|
||||
## Root Cause
|
||||
|
||||
```java
|
||||
// TransactionRollbackSetupAction.java:99-104
|
||||
holder.depth += increment;
|
||||
if (holder.depth == 0) {
|
||||
depth.set(null); // <-- should be depth.remove()
|
||||
return holder.actuallyCleanUp;
|
||||
}
|
||||
return false;
|
||||
```
|
||||
|
||||
## Severity Note
|
||||
|
||||
This is a **MOAD-0003 minor** finding — no per-request value leak (the value IS nulled). The defect class is classloader retention during deployment churn:
|
||||
- Long-running production app servers (uptime measured in months) accumulate dangling ThreadLocal entries equal to (active threads) × (deployments × redeploys)
|
||||
- Each entry pins ~1KB of memory plus the application classloader's reachability graph
|
||||
- For a deployment with 100 worker threads and 50 redeploys, this is 5,000 dangling entries — modest but real
|
||||
|
||||
Not exploit-grade. Defense-in-depth and memory hygiene.
|
||||
|
||||
## Fix
|
||||
|
||||
One-line change: `depth.set(null)` → `depth.remove()`.
|
||||
|
||||
```java
|
||||
holder.depth += increment;
|
||||
if (holder.depth == 0) {
|
||||
depth.remove();
|
||||
return holder.actuallyCleanUp;
|
||||
}
|
||||
return false;
|
||||
```
|
||||
|
||||
## Related findings
|
||||
|
||||
Same project (WildFly), same MOAD-0003 family:
|
||||
- `wildfly-0001` (UNDF-1305): ElytronSecurityIntegration — security-grade leak
|
||||
- `wildfly-0002` (UNDF-1306): ElytronSecurityDomainContextImpl.isValid — security-grade leak
|
||||
- `wildfly-0003` (this): TransactionRollbackSetupAction — classloader-retention only
|
||||
|
||||
All three surfaced after unmoad scanner commit `1f48798` cleared 99.2% of the WildFly M3 noise.
|
||||
Loading…
Add table
Add a link
Reference in a new issue