dotnet / dotnet/aspnetcore

[SignalR] Client-result invocation IDs are a predictable process-wide counter, and the return-type lookup is not connection-scoped

Open
#69,284 0 comments 0 reactions 0 assignees View on GitHub
area-signalr
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 10h
Merged PRs (30d)
281

Description

**Observed on:** `release/10.0` @ 10.0.13, commit `1307f36b2990bf5c81aace7c6a1535a2be46d7ef`. I have not checked the 8.0/9.0 branches.

### Summary

Two defence-in-depth gaps in the client-results path. Neither is exploitable today
because the ownership check in `TryCompleteResult` holds, but both weaken it
unnecessarily.

### 1. Invocation IDs are predictable across all connections

`src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs:345`

```csharp
var id = Interlocked.Increment(ref _lastInvocationId);
var invocationId = $"s{id}";
```

`_lastInvocationId` is a single `ulong` field on the lifetime manager, shared by
every connection on the server, so IDs are `s1`, `s2`, `s3`... Any client can
enumerate the invocation IDs issued to every other client.

### 2. The return-type lookup is not connection-scoped

`src/SignalR/common/Shared/ClientResultsManager.cs:81,91` — `TryGetType` and
`GetReturnType` resolve by invocation ID alone, with no connection argument, and
run during message parsing, i.e. before any ownership check.

By contrast `TryCompleteResult` (line 52) *does* check ownership:

```csharp
if (item.ConnectionId != connectionId)
{
throw new InvalidOperationException($"Connection ID '{connectionId}' is not valid for invocation ID '{message.InvocationId}'.");
}
```

### Residual behaviour

Because the ownership check holds, a client cannot supply the result for another
client's invocation. What remains is an oracle: sending a `CompletionMessage` for a
guessed `sN` produces two distinguishable outcomes at
`DefaultHubDispatcher.cs:213` —

- `sN` pending for any connection -> `SetConnectionResultAsync` -> ownership throw
-> the probing connection is torn down.
- `sN` not pending -> `Log.UnexpectedCompletion` -> the probing connection survives.

So a client can test whether a given invocation ID is outstanding server-wide, at a
cost of one connection per probe. This discloses invocation-counter activity, not
user data.

### Suggested fix

1. Make invocation IDs unguessable, or scope the counter per connection so one
client's IDs say nothing about another's.
2. Give `TryGetReturnType` / `TryGetType` a `connectionId` parameter so the type
lookup is scoped the same way completion already is. This removes the oracle and
means the parse path no longer deserialises a payload against a type belonging
to a different connection's pending invocation.

### Verification status

Source-level trace, not an executed repro — building this servicing branch needs the
runtime toolset `10.0.13-servicing.26454.107`, which is not on the public feed. Noting
that rather than implying I observed the oracle behaviour at runtime. The ownership
check in `TryCompleteResult` is unambiguous in source, so I am confident this is a
hardening item and **not** a hijacking bug; please read it that way.

Contributor guide

Open the contributing guide

Research direction

Start with DefaultHubLifetimeManager.cs around line 345 and ClientResultsManager.cs around lines 52, 81, and 91, then trace parsing through DefaultHubDispatcher.cs around line 213. Compare the connection-scoped ownership check with the current invocation-ID and return-type lookups. Done means IDs no longer expose cross-connection activity and return-type resolution is scoped consistently; runtime verification requires the unavailable 10.0.13-servicing.26454.107 toolset.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.