server: refuse a CREATE2 whose tag does not fit the message - #216
Merged
Conversation
`CREATE2(WANT_STATUS)` promises exactly one outcome and documents that a
malformed field answers `INVALID` (docs/protocol.md). Four of the six
fields do that. The tag did not: an out-of-range `tag_len` and a
non-UTF-8 tag both fell back to an empty tag and let the create succeed.
let tag = if data.len() >= 10 + tag_len {
std::str::from_utf8(&data[10..10 + tag_len]).unwrap_or_default()
} else {
""
};
Both cases are wrong, and the first is worse than a mislabelled
terminal. `cursor` is set to `10 + tag_len` regardless, so an
overrunning length leaves it past the end of the message. The cwd and
deadline arms bounds-check what they read and would refuse — but a
`CREATE2` that carries a command and neither of those has nothing left
to catch it: `data.get(cursor..)` returns `None`, `create_payload` is
`None`, and the server spawns the default shell instead of the command
it was sent. A client that asked to run one thing silently gets a
prompt.
The non-UTF-8 case is quieter: the terminal exists with an empty tag, so
a client correlating by tag can never match it and has no refusal to
react to.
Pull the read into `create2_tag`, which returns the tag or the detail
string to refuse with, and refuse on both. Extracted rather than
inlined so the two failures are testable — `supervise` and the create
arms need an `AppState` that nothing in the suite constructs, which is
the same reason `armed_deadline` and `slots_to_evict` are pure.
Three tests cover a well-formed tag (including one followed by later
fields), a length past the end, off by one and far, and a non-UTF-8
tag. Checked they are not vacuous by restoring the empty-tag fallback
and confirming both refusal tests fail.
Workspace clippy and `cargo fmt` clean, 290 blit-server tests passing.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Coverage
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #204, from reviewing the merged PTY lifecycle work.
The bug
CREATE2(WANT_STATUS)promises exactly one outcome, anddocs/protocol.mdsays a malformed field answersINVALID. Four of the six fields do. The tag did not:An out-of-range
tag_lenand a non-UTF-8 tag both fell back to an empty tag and let the create succeed.The first is worse than a mislabelled terminal.
cursorbecomes10 + tag_lenregardless, so an overrunning length leaves it past the end of the message. The cwd and deadline arms bounds-check what they read and would refuse — but aCREATE2carrying a command and neither of those has nothing left to catch it:data.get(cursor..)returnsNone,create_payloadisNone, and the server spawns the default shell instead of the command it was sent. A client that asked to run one thing silently gets a prompt.The non-UTF-8 case is quieter: the terminal exists with an empty tag, so a client correlating by tag can never match it and has no refusal to react to.
The fix
Pull the read into
create2_tag, which returns the tag or the detail string to refuse with, and refuse on both.Extracted rather than inlined so the two failures are testable — the create arms need an
AppStatethat nothing in the suite constructs, which is the same reasonarmed_deadlineandslots_to_evictare pure.Verification
Three tests: a well-formed tag (including one followed by later fields), a length past the end (off by one and far), and a non-UTF-8 tag. Checked they are not vacuous by restoring the empty-tag fallback and confirming both refusal tests fail.
Workspace clippy and
cargo fmtclean, 290 blit-server tests passing.