Stop telling clients where this machine keeps things - #106
Merged
Conversation
`GET /jobs/{id}` returns a failed job's `error_message`, and that message came
from the engine, which names the file it was working on. For a job that file
is inside the staging directory, so an ordinary upload learned the server's
storage layout:
Compression failed: failed to unpack `/var/lib/collapse/jobs/<uuid>/tree/…`
Reachable with a tar whose second entry has the first, a plain file, for a
parent. The server has no authentication (issue #72, untouched here), so "a
client" is anyone who can reach the port.
Two things were wrong and only one was obvious. The obvious one: unpacking a
tar envelope did not go through `failure_message` at all. The other: the
curation it skipped would not have helped much, because it rewrote one variant
and passed the rest through, on the stated reasoning that they "already read
as a sentence about something the client did".
So the rule is inverted rather than extended. Redact unless there is a curated
sentence. Enumerating the leaky variants would have been the smaller change
and the wrong one: `CompressionError` gains variants, and the next one would
have leaked until somebody noticed. `From<String> for Failure` redacts too, so
a new error path is safe by default rather than safe only if someone remembers.
Redaction is blunt on purpose. The server has no reason to tell a client where
anything lives, so removing every absolute path is correct rather than merely
convenient, and it does not depend on knowing which variant produced the
message or where the staging directory is mounted. Relative paths survive:
those are the client's own entry names.
`Failure { client, log }` splits the two audiences. An operator wants the whole
truth including the path; a client must not have it. Verified end to end
against a running server:
client: Compression failed: failed to unpack `<path>`
log: Compression failed: failed to unpack `/private/tmp/.../tree/…`
One existing test asserted the old policy by name,
`other_engine_errors_are_passed_through_word_for_word`. Its assertions still
passed, since none of its fixtures held a path, but its name and comment
claimed something no longer true. Rewritten rather than left to mislead.
Both mutations are caught: making the redaction a no-op, and letting
`extract_tar` bypass the curation again.
Closes #66
Closed
6 tasks
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
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.
Fixes #66. No authentication here: that is #72 and stays untouched.
The leak
GET /jobs/{id}returns a failed job'serror_message. That message came from the engine, which names the file it was working on, and for a job that file lives inside the staging directory. Reproduced against a running server with an ordinary upload:The upload is a tar whose second entry has the first, a plain file, for a parent. Since the server has no authentication, "a client" is anyone who can reach the port.
Two problems, one obvious
The obvious one: unpacking a tar envelope did not go through
failure_messageat all (queue.rs:116).The other: the curation it skipped would not have helped much. It rewrote one variant and passed the rest through, on this stated reasoning:
That was not true, and the counterexample is the message above.
Inverted, not extended
Redact unless there is a curated sentence. Enumerating the leaky variants would have been the smaller change and the wrong one:
CompressionErrorgains variants, and the next one would leak until somebody noticed.From<String> for Failureredacts too, so a new error path is safe by default rather than safe only if someone remembers.Redaction is blunt on purpose. The server has no reason to tell a client where anything lives, so removing every absolute path is correct rather than merely convenient, and it does not depend on knowing which variant produced the message or where the staging directory happens to be mounted. Relative paths survive, because those are the client's own entry names and exactly what it needs to see.
The operator loses nothing
Failure { client, log }splits the two audiences. Verified end to end against a running server, same job:A test that asserted the old policy
other_engine_errors_are_passed_through_word_for_wordstill passed, because none of its fixtures held a path. But its name and comment claimed something no longer true, which is how the next reader gets misled. Rewritten asan_engine_error_that_names_no_location_is_passed_through_word_for_word, with a twin for the case it never covered.Tests
Six new. Two drive a real job end to end and assert the returned
error_messagenames neither the staging directory nor the job id; the rest cover the redactor, including that a relative path is not mistaken for a location.extract_tarbypasses the curation again614 Rust tests and 116 Vitest, 499 offline.
threat_model.mdgains section 8b, which had no entry for error-message disclosure at all.Not in scope
Piece 1 of #66, giving
CompressionErrorreal structure so callers can tell causes apart, is a design decision with three candidate shapes and does not block this. Worth reopening or splitting once this lands.