Skip to content

fix(server_utils): use unpredictable per-process secret for session signing - #2109

Open
AUTHENSOR wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
AUTHENSOR:fix/session-secret-key-predictable
Open

AUTHENSOR wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
AUTHENSOR:fix/session-secret-key-predictable

Conversation

@AUTHENSOR

Copy link
Copy Markdown

Summary

get_session_middleware_key() returned a deterministic string built from two public components — the Python class name and the benchmark's config name:

def get_session_middleware_key(self) -> str:
    return f"{self.__class__.__name__}___{self.config.name}"

Both are knowable by any client: the class name appears in source and in error responses (server_utils.py:567); the config name is the public benchmark server name. This string was reused as the signing secret for both:

  1. Starlette SessionMiddleware (server_utils.py:555, secret_key=session_middleware_key) — signs session cookies via itsdangerous.
  2. MCP auto-exposure URLSafeSerializer (mcp_auto_exposure.py:760-761, secret = server.get_session_middleware_key()) — signs per-rollout MCP session tokens.

Knowing the secret, any client can forge session cookies and MCP tokens offline.

The code's own comment acknowledged this was a placeholder:

"This method is here to override in case we want to ever use an actual session middleware secret key. e.g. for an actual product."

…but no benchmark in the repo overrides it, so the default ships with every example environment.

Impact

A resources server is a single process holding session_state for all concurrent rollouts (base_resources_server.py), keyed by session_id. A forged cookie carrying another rollout's session_id lets one rollout read or clobber another's env state. A forged MCP token with "tools": None defeats the per-rollout tool allow-list (mcp_allowed_tools_for_session at base_resources_server.py:132). This breaks rollout isolation even on localhost — the threat isn't a remote attacker, it's one rollout's agent accessing another rollout's state within the same benchmark run.

Honest scope: this does not grant remote code execution or network-level access. The integrity failure is within-process: cross-rollout session_state access and MCP tool-allow-list bypass. That's still a real trust-boundary failure in a multi-tenant RL eval framework.

Reproduction

from itsdangerous import URLSafeTimedSerializer, URLSafeSerializer

# Attacker reconstructs the predictable secret from public information
secret = "SimpleResourcesServer___blackjack"  # class name + config name

# Forge a session cookie for any session_id
serializer = URLSafeTimedSerializer(secret, salt="cookie-session")
forged_cookie = serializer.dumps({"session_id": "victim-rollout-session-id"})
# → accepted by the server as session_id="victim-rollout-session-id"

# Forge an MCP token that defeats the tool allow-list
mcp_serializer = URLSafeSerializer(secret, salt="mcp_session_token")
forged_token = mcp_serializer.dumps({"sid": "victim-rollout-session-id", "tools": None})
# → tools=None defeats mcp_allowed_tools_for_session narrowing

Verified locally: both forgeries succeed with the pre-fix secret and fail with a random per-process secret.

The fix

Split the concerns. get_session_middleware_key() continues to return the stable, human-readable identifier (used as the cookie name, in error messages, and as a profiling-dir name — all non-security contexts). A new get_session_secret_key() returns a cached per-process random secret (secrets.token_hex(32)) used only as the signing key for SessionMiddleware and the MCP token serializer:

def get_session_secret_key(self) -> str:
    cached = getattr(self, "_session_secret_key", None)
    if cached is None:
        cached = secrets.token_hex(32)
        self._session_secret_key = cached
    return cached

The caching is important: get_session_secret_key() is called from both setup_session_middleware and install_auto_exposure, and they must agree on the same secret. For multi-process deployments where all workers must share the key, override get_session_secret_key() to return a deployment-wide stable value (read from an env var or secret store).

Verification

  • PoC: the pre-fix forgery (above) succeeds; with the fix, the per-process secret is unpredictable so the same forgery fails signature verification.
  • Caching verified: the same instance returns the same secret across calls (so SessionMiddleware and MCP agree); different instances get different secrets.
  • ruff check passes (line-length 119, full rule set).
  • Both changed files pass AST/syntax validation.
  • Tests: I could not run tests/unit_tests/test_server_utils.py and tests/unit_tests/test_mcp_auto_exposure.py locally due to missing optional deps in my environment (omegaconf, ray); CI will run them. The change is additive (new method + two call-site swaps) and does not alter any existing method's signature or return type.

Checklist

  • Predictable signing secret replaced with cached per-process random secret
  • Session cookie forgery defeated
  • MCP token forgery defeated
  • get_session_middleware_key() unchanged (still used for cookie name, error messages, profiling dir)
  • Override hook documented for multi-process deployments
  • ruff check passes
  • No new dependencies
  • Honest scope: within-process integrity (cross-rollout state + tool allow-list), not RCE

…igning

get_session_middleware_key() returned a deterministic string built from
public class and config names:

    f'{self.__class__.__name__}___{self.config.name}'

Both components are knowable by any client (class name is in source and
error responses; config name is the public benchmark server name). This
string was then reused as the signing secret for:

  1. Starlette SessionMiddleware (secret_key=) — signs session cookies
  2. MCP auto-exposure URLSafeSerializer — signs per-rollout MCP tokens

Knowing the secret, any client can forge session cookies and MCP tokens
offline. A resources server holds session_state for ALL concurrent
rollouts in a single process, keyed by session_id; a forged cookie
carrying another rollout's session_id lets one rollout read or clobber
another's state. A forged MCP token with tools=None defeats the
per-rollout tool allow-list (mcp_allowed_tools_for_session).

The code's own comment acknowledged this was a placeholder ('override
in case we want to ever use an actual session middleware secret key'),
but no benchmark in the repo overrode it.

Fix: split the concerns. get_session_middleware_key() continues to
return the stable, human-readable identifier (used as the cookie name,
in error messages, and as a profiling-dir name — all non-security
contexts). A new get_session_secret_key() returns a cached per-process
random secret (secrets.token_hex(32)) used only as the signing key.
Override get_session_secret_key() for multi-process deployments where
all workers must agree on the key.

PoC (before fix): forge a session cookie offline with the predictable
secret; server accepts it as any session_id. After fix: the per-process
secret is unpredictable, so forgery fails.
@copy-pr-bot

copy-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@nemo-automation-bot nemo-automation-bot Bot added the community-request Issue reported or requested by someone from the community label Jul 22, 2026
@github-actions github-actions Bot added the sla:triage-overdue Review assignment is over the one-business-day SLA label Jul 23, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Jul 25, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci added area:core Shared APIs, servers, telemetry, health, and registries bug Something isn't working labels Oct 20, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core Shared APIs, servers, telemetry, health, and registries bug Something isn't working community-request Issue reported or requested by someone from the community sla:triage-overdue Review assignment is over the one-business-day SLA waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants