Stop extracting an archive whose listing cannot be read - #101
Merged
Conversation
`plan_for` swallowed a listing failure and carried on with the identity plan. The comment justifying that was right about the message and wrong about the timing: the extractor fails **while streaming**, so by the time it notices it has written every entry before the fault, and written them with no plan at all. No rewriting, no refusal, no collision check. Reproduced, same two entries, twice: intact listing report Ok(1) extract Err ON DISK [] unreadable report Err extract Err ON DISK [notes.txt:hidden, second.txt] On Windows `notes.txt:hidden` is not a file name, it is the `hidden` alternate data stream of `notes.txt`: the write succeeds, the bytes land where no listing shows them, and the user was told there was nothing to answer for. Issue #63's harm, performed without consent, arranged by appending one bad 512 byte header to a tar. The message half of the old argument turned out to be the weak half. Both passes go through the same parser, so they say close to the same thing; in the reproduction they differed only in where they were cut off. What this costs is partial recovery: a truncated tar used to hand back whatever preceded the damage and now hands back nothing. Deliberate, and not actually gone, since the backends take no options and never come through the planning pass. `recovering_from_a_damaged_archive_is_still_possible_through_ the_backend` pins that so the capability does not look deleted. Checked before committing to it that the listing parser is not stricter than the extractor for archives that are merely unusual: a tar with no end-of-archive marker and a zip with junk appended both list fine, so this does not start refusing good archives. Also makes the refusal readable, which is not scope creep but the consequence of the change: it is now the whole of what the user gets for a damaged archive, and the tar crate embeds the bytes it choked on. before: Compression failed: numeric field did not have utf-8 text: <4 bad bytes> when getting cksum for <100 more> after: Compression failed: this archive could not be read, so nothing was extracted: numeric field did not have utf-8 text: when getting cksum for Narrower than issue #66, which is about callers telling causes apart. This is only about what is fit to show a person. One desktop assertion moves with it, since the 7z message is now prefixed with what happened before why. Closes #89
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 #89. The cheap option of the two we discussed: refuse, rather than judge each name lazily as it is written.
The bug
plan_forswallowed a listing failure and carried on with the identity plan. The comment justifying that was right about the message and wrong about the timing: the extractor fails while streaming, so by the time it notices it has already written every entry before the fault, with no plan at all. No rewriting, no refusal, no collision check.Same two entries, twice. The second archive is identical except that its end-of-archive marker is replaced with garbage:
On Windows
notes.txt:hiddenis not a file name, it is thehiddenalternate data stream ofnotes.txt: the write succeeds, the bytes land where no listing shows them, and the user was told there was nothing to answer for. Issue #63's harm without consent, arranged by appending one bad 512 byte header to a tar.NamePlan::identity()meant two different things and the write sites could not tell them apart: "nobody asked for rules" (correct, that isextract_zipcalled directly) and "rules were asked for and I could not read the listing".Why the cheap option is defensible
The message half of the old argument turned out to be the weak half. Both passes go through the same parser, so they say close to the same thing; in the reproduction they differed only in where they were cut off.
What it costs, and what it does not
Partial recovery: a truncated tar used to hand back whatever preceded the damage and now hands back nothing.
Not actually gone, though. The backends take no options and never come through the planning pass, so
extract_tarstill salvages.recovering_from_a_damaged_archive_is_still_possible_through_the_backendpins that, so the capability does not look deleted.Checked before committing to this that the listing parser is not stricter than the extractor for archives that are merely unusual, or we would start refusing good ones:
extract()The message
Not scope creep: the refusal is now the whole of what the user gets for a damaged archive, and the tar crate embeds the bytes it choked on.
Narrower than #66, which is about callers telling causes apart. This is only about what is fit to show a person. One desktop assertion moves with it.
Tests
Four new, two of which fail against the old behaviour:
a_damaged_archive_writes_nothing_rather_than_writing_raw_namesthe_same_names_are_refused_whether_or_not_the_archive_is_damageda_damaged_archive_says_so_in_words_a_person_can_readrecovering_from_a_damaged_archive_is_still_possible_through_the_backendOne existing test was renamed:
an_archive_that_cannot_be_listed_still_fails_in_the_extractor_s_wordsstill passed, but its comment said "the listing pass is advisory on purpose", which is no longer true. It would have misled the next reader.Verified with the real binary too: a correctly damaged tar exits 1 and creates no output directory, while the intact one extracts normally.
Counts: 604 Rust and 113 Vitest, 489 offline.
threat_model.mdupdated.