Repository navigation
[HIGH] Bound peer-created stream state - #201
OskarEichler wants to merge 5 commits into
Conversation
|
Addressed in 1a011bf: promised streams now use a counter updated on activation/close instead of an O(n) scan, with a lifecycle regression; simplified priority defaults and the remote-client/push-disabled checks. One deliberate difference from the suggestion: cached idle priority is deleted even when HEADERS carries its own PRIORITY, otherwise the old entry stays retained after activation. HEADERS priority still takes precedence. Updated the RBS counter declaration. Verification: 450 examples, zero failures; 41 RuboCop files clean; git diff --check passes. |
|
CI follow-up: all nine Ruby runtime jobs pass on the pushed revision. The coverage job fails when its reporting action tries to create a GitHub check run and receives 403 Resource not accessible by integration; the log does not report a coverage-threshold failure. This appears to require maintainer-side workflow permissions rather than a runtime code change. |
| ) | ||
| } | ||
| @idle_stream_priorities[stream_id] = priority | ||
| stream = Stream.new(connection: self, id: stream_id, **priority) |
There was a problem hiding this comment.
this feels strange. why create a Stream object directly and bypass activate_stream? It feels stream creation should either be bypassed, or the frame should be kept around for eventual handling once the stream is open and activated for good.
|
@OskarEichler fair enough, it's a known issue, I've been waiting for @igrigorik to either make me repository admin, or he himself update the repo permissions so CI can perform that operation. |
|
Addressed the Stream.new concern: idle PRIORITY frames now only update the bounded metadata cache. Stream creation and the :stream event occur on actual activation through activate_stream. Updated the existing idle-priority expectation; the existing repeated-priority/opening regression still passes. Verified all 450 available non-fixture examples, targeted RuboCop, syntax, and git diff --check. |
|
@OskarEichler your changes broke one of the conformance tests. |
|
cherry-pick to main. |
|
@OskarEichler After looking at this again, I'm not so convinced that the following claims are true, nor were they fixed (and the fact that the added test in
Yes, a stream object is retained; however, the PRIORITY parameters are also retained. A stream is created, period. Sure, the parameters are lighter in comparison, but it's not like a memory leak is happening. I'd like a better explanation for why your LLM rated this as a HIGH risk. I also don't think that this patch accurately counts against the concurrent-stream limit. FWIW this is used when: activating a stream (and it's matched against active stream count, which only accounts for at-least open streams, i.e. discards the idle streams that PRIORITY frames create); when receiving a PUSH_PROMISE, and it's matched against
I don't think this is accurate. The headers are validated when they arrive (i.e. when the HEADERS frame is processed). pseudo-header validation happens when stream objects transition to "open". LMK if you have thoughts on this. I'd like to cut a release soon, and if we don't get to a consensus here, I'm tempted to revert the mentioned changes until we do. |
|
this has been partially reverted in 8633de6 |
2 participants
Summary
Security impact
A peer could send a PRIORITY frame for an idle stream, causing a Stream object to be retained without counting against the concurrent-stream limit. Repeating that with distinct IDs grew the connection registry linearly: 1,000 small frames retained 1,000 streams while active_stream_count stayed at zero.
The retained idle object also changed later HEADERS handling. A request missing :authority was rejected on a fresh stream but accepted when a PRIORITY frame pre-created that stream, because new-stream pseudo-header validation was skipped.
Servers also accepted client-sent PUSH_PROMISE frames even though clients cannot push. A focused reproduction retained 1,001 streams from one active request after 1,000 promises. Clients did not enforce SETTINGS_ENABLE_PUSH=0 or bound reserved promises.
After this change, the priority reproduction retains no live stream objects, limits compatibility IDs to 100, and rejects the missing-authority request. Servers reject client push, disabled clients reject push, and the 101st concurrently retained promise is refused when the configured limit is 100.
Verification
rbenv exec bundle exec rake: 448 examples, 0 failures; 41 RuboCop files, no offensesrbenv exec bundle exec rake buildgit diff --checkAdded focused regression coverage for repeated idle PRIORITY updates and the priority retained when that stream later opens.
Limitations
The repository's HPACK fixture task cannot currently run because its fixtures are absent (tracked separately in issue #200). Live sockets, alternate Ruby engines, and a long-running production server were not exercised.
Breaking changes
Illegal client PUSH_PROMISE frames and pushes received while disabled now close the connection with PROTOCOL_ERROR. Excess valid promises are refused. Untracked PRIORITY frames no longer enter the live stream registry; the existing stream callback remains available for the first bounded set of distinct idle priority IDs.