sqfstar: improve tar parser stability #309
Merged
Conversation
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
The binary representation of sizes can overflow long long. Check for such values and stop further processing. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
GNU tar and libarchive check the isextended byte on bit level, not as real number. The code currently only works because isextended being -1 is still considered true and 0 (false) occurs because the octal parsing ends at '\0' and takes the already parsed value. This means that 0x00 becomes 0, everything else -1 and thus true. The compatibility ends if the ASCII character '0' is inserted, which sqfstar interprets as 0 (false), while GNU tar and libarchive treat it as != 0x00 and thus true. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Reading major and minor numbers of character and block devices within a tar archive could lead to subsequent left shifts triggering signed integer overflows. Prevent these overflows and check afterwards if st_rdev calculation is suitable for the architecture or not. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Fail with an error if read mtime cannot be represented with current system. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The pax extended header could be perfectly aligned with 512 bytes and thus contain no ending \0 character. Add it explicitly to make sure that string parsing functions never run out of boundary. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Pax headers are supposed to have exactly one blank between length and keyword. Skipping multiple blanks or even tabs can lead to diverging views on a tar file across multiple implementations. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The code cannot read more than INT_MAX bytes, which fails because the length value is an int and eventually compared with the original size kept in a long long. Print a proper error message if this happens. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Do not use sscanf but a custom function which guarantees positive numbers and counts the parsed bytes to mimic sscanf for easier replacement. Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
|
This fixes a couple of issues, thanks. |
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.
These commits fix different issues in sqfstar's tar parser and partially
depend on each other to mitigate the discussed issues fully:
Signed integer overflows are undefined behaviour and should be avoided.
From a C perspective, I do not care too much about them, but they can
lead to crashes later on in the code, e.g. because sqfstar does not
expect to encouter actually negative numbers for sizes later on.
Commits:
Proof of Concepts:
This crashes sqfstar because of negative sizes:
The crash occurs because the parsed number -2 leads to a read of 0 bytes
and a fragment added into the fragment processor. The processor itself
tries to create a checksum and eventually reads past the end of
allocated memory.
This triggers a signed integer overflow (compile with UBSAN):
This triggers an out ouf boundary read:
It is not covered by ASAN and hard to see in reality as a crash.
In fact, it took an OpenBSD system with its guard pages to prove that
out of boundary access can occur.
The sqfstar code has some unique traits in it which allows different
views on the same tar archive with different tools, e.g. GNU tar,
libarchive, docker (Go), and so on. It is possible to create a tar
archive which sneaks in special information into a squash filesystem
which otherwise is not detected. Sylab's singularity utilizes sqfstar
for conversions from OCI containers to sqfs. By making sqfstar behave
more like the other tar parsers and stay closer to the POSIX standard,
such attacks will be harder to be performed.
Commits:
Proof of Concepts:
This file can be parsed by GNU tar and libarchive, not by sqfstar:
This file sneaks in a different link into sqfs from tar:
You can see that the poc.sqfs contains a link to a different target than
the tar file implies with GNU tar or libarchive's bsdtar.
The sparse map parser allows a targeted out of boundary write with
attacker controlled data. Together with heap spraying due to leaking
memory if pax keywords are supplied multiple times, this might allow
an attacker to perform elevated attacks. Together with number 2, this
can be targeted specifically against sqfstar without triggering issues
with other tar implementations.
Commit:
Proof of Concept (compile with ASAN):
This allows an attacker to write multiples of 16 bytes beyond a multiple
of 16 bytes of allocated memory.
Some values might overflow system data types, e.g. times. These commits
are merely cleanups.
Commits: