A filter block size must not be larger than the lzss window, which is
defined
by dictionary size, which in turn can be derived from unpacked file
size.
While at it, improve error messages and fix lzss window wrap around
logic.
Fixes https://github.com/libarchive/libarchive/issues/2565
---------
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-authored-by: Tim Kientzle <kientzle@acm.org>
If a system is capable of handling 4 billion nodes in memory, a double
free could occur because of an unsigned integer overflow leading to a
realloc call with size argument of 0. Eventually, the client will
release that memory again, triggering a double free.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The refactoring of https://github.com/libarchive/libarchive/pull/2553
introduced three issues:
1. Introduction of a modifiable global static variable
This violates the goal of having no global variables as stated in [the
README.md](https://github.com/libarchive/libarchive/blob/b6f6557abb8235f604eced6facb42da8c7ab2a41/README.md?plain=1#L195)
which in turn leads to concurrency issues. Without any form of mutex
protection, multiple threads are not guaranteed to see the correct
min/max values. Since these are not needed in regular use cases but only
in edge cases, handle them in functions with local variables only.
Also the global variables are locale-dependent which can change during
runtime. In that case, future calls leads to issues.
2. Broken 32 bit support
The writers for zip and others affected by the previously mentioned PR
and test-suite on Debian 12 i686 are broken, because the calculation of
maximum MS-DOS time is not possible with a 32 bit time_t. Treat these
cases properly.
3. Edge case protection
Huge or tiny int64_t values can easily lead to unsigned integer
overflows. While these do not affect stability of libarchive, the
results are still wrong, i.e. are not capped at min/max as expected.
In total, the functions are much closer to their original versions again
(+ more range checks).
---------
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Make sure that size_t casts do not truncate the value of packed_size on
32 bit systems since it's 64 bit. Extensions to RAR format allow 64 bit
values to be specified in archives.
Also verify that 64 bit signed arithmetics do not overflow.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
It is possible to trigger a call stack overflow by repeatedly entering
the rar_read_ahead function. In normal circumstances, this recursion is
optimized away by common compilers, but default settings with MSVC keep
the recursion in place. Explicitly turn the recursion into a goto-loop
to avoid the overflow even with no compiler optimizations.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Reset avail_in and next_in if the next entry of a split archive is
parsed to always update its internal structure to access next bytes when
cache runs empty.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Support header sizes larger than 32 bit even on 32 bit systems, since
these normally have large file support. Otherwise an unsigned integer
overflow could occur, leading to erroneous parsing on these systems.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Most source files use tabs instead of spaces, but
archive_read_support_format_rar.c uses spaces most of the time. A few
lines contain a mixture of tabs and spaces, which leads to poorly
formatted output with many default settings.
Unify the style. No functional change and preparation for upcoming
changes to get rid of white space diffs.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Make sure to not skip past end of file for better error messages. One
such example is now visible with rar testsuite. You can see the
difference already by an actually not useless use of cat:
```
$ cat .../test_read_format_rar_ppmd_use_after_free.rar | bsdtar -t
bsdtar: Archive entry has empty or unreadable filename ... skipping.
bsdtar: Archive entry has empty or unreadable filename ... skipping.
bsdtar: Truncated input file (needed 119 bytes, only 0 available)
bsdtar: Error exit delayed from previous errors.
```
compared to
```
$ bsdtar -tf .../test_read_format_rar_ppmd_use_after_free.rar
bsdtar: Archive entry has empty or unreadable filename ... skipping.
bsdtar: Archive entry has empty or unreadable filename ... skipping.
bsdtar: Error exit delayed from previous errors.
```
Since the former cannot lseek, the error is a different one
(ARCHIVE_FATAL vs ARCHIVE_EOF). The piped version states explicitly that
truncation occurred, while the latter states EOF because the skip past
the end of file was successful.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The size_t data type is only 32 bit on 32 bit sytems while off_t is
generally 64 bit to support files larger than 2 GB.
If an entry is declared to be larger than 4 GB and the entry shall be
skipped, then 32 bit systems truncate the requested amount of bytes.
This leads to different interpretation of data in tar files compared to
64 bit systems.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
If a warc archive claims to have more than INT64_MAX - 4 content bytes,
the inevitable failure to skip all these bytes could lead to parsing
data which should be ignored instead.
The test case contains a conversation entry with that many bytes and if
the entry is not properly skipped, the warc implementation would read
the conversation data as a new file entry.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The skip functions are limited to 1 GB for cases in which libarchive
runs on a system with an off_t or long with 32 bits. This has negative
impact on 64 bit systems.
Instead, make sure that _all_ subsequent functions truncate properly.
Some of them already did and some had regressions for over 10 years.
Tests pass on Debian 12 i686 configured with --disable-largefile, i.e.
running with an off_t with 32 bits.
Casts added where needed to still pass MSVC builds.
---------
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The lafe_errc function adds a newline by itself already, so do not
insert one into the message.
You can reproduce with the following commands:
```
touch archive.tar
bsdtar -xf archive.tar -C /non-existing
```
```
bsdtar --exclude ""
```
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The parsers for 7zip and xar contain memory leaks. Also the testsuite
7zip leaks memory. Plug them.
Noticed while running test suite with address sanitizer on Linux.
Turning window_mask into ssize_t and adjusting the signature of
circular_memcpy reduces the amount of required casts. Since window_mask
depends on value window_size, which is ssize_t, this change is safe.
Also turned extra_data_size into int64_t, which is a no-op on 64 bit
systems but prevents 32 bit systems from truncating these huge values in
archives. Since these systems most likely have large file support, this
could have meant truncation -- in theory.
Reaching EOF for first time sets the correct amount of available bytes,
but each subsequent call returns 0.
Do not forget that the copy buffer can already contain data and return
the amount of bytes left in there.
See added test case.
This actually disabled CRC checks on regular builds and enabled it if
CRC checks were supposed to be disabled.
All other occurrences properly use ifndef instead of ifdef.
If libarchive is built without zlib support, it is possible to trigger a
NULL pointer dereference with specially crafted 7zip files.
It takes multiple conditions to be able to reach the issue in crc32 and
all 7zip-specific ones are fixed with this PR.
Most of the functions already use size_t for sizes. Do the same for
xml_data. The libraries expat and libxml2 do not support strings larger
than 2 GB (limit is less than this) but it fixes one last Visual Studio
compiler warning which I missed previously.
Use casts where appropriate (values cannot exceed data type limits).
Fixes following warnings:
warning C4244: '=': conversion from 'int64_t' to 'long', possible loss
of data
warning C4244: '=': conversion from 'uint64_t' to 'size_t', possible
loss of data
Adjust type of variables for their specific use case
Add a new inline function to unify casting (and clarify code blocks)
Use definition to explain magic number (and reduce casts)
Use `hpos` instead of magic number to highlight that additional bytes
will be parsed
Switch from int64_t to ssize_t for l and body, which is a no-op on 64
bit systems. On 32 bit systems, this change is okay because these
variables interact with other ssize_t variables in this scope.
Fixes compiler warning regarding line 584 in which ssize_t and int64_t
are mixed in calculations:
warning C4244: '=': conversion from 'int64_t' to 'long'
The cast in ppmd_read function is safe even on 32 bit systems. It is
called byte for byte which makes it impossible to actually reach
SIZE_MAX on any real world hardware.
Fixes Visual Studio warnings:
warning C4244: '=': conversion from 'uint64_t' to 'size_t', possible
loss of data
warning C4244: 'function': conversion from 'int64_t' to 'size_t',
possible loss of data
If EOF is encountered while reading the new filename after choosing 'r',
avoid out of boundary access and usage of undefined memory content by
treating it the same way as if the question itself was not answered.
Processing excessively long lines could lead to out of boundary writes
or denial of service due to O(n^2) runtime complexity.
The OOB is properly fixed with first commit. The second commit stops
processing of lines which are longer than uu allows due to its
specification.
The comp size could be around INT_MAX on huge archives, which would lead
to eventual integer truncation to size_t in archives with version 1
headers when fixed value 2 is added to comp_size on 32 bit systems.
This fix is a no-op on 64 bit systems because size_t and uint64_t are of
same size there.
Cast any of cd_offset or cd_size to int64_t to avoid truncation of
result because both variables are of type uint32_t.
The calculation happens before comparison with current_offset, so it is
not automatically expanded to int64_t during calculation.
If an RPM file contains a huge header which is larger than 4 GB then
libarchive starts parsing the RPM header as actual archive instead of
skipping it.
Switched to uint64_t from size_t for proper 32 bit support as well.
When working with matches, the code does not care about the actual
amount of times when it matched, but just if it matched so far at least
once or never.
Turning the counter into a boolean flag has the advantage that even
insanely huge archives will never lead to integer overflow here.
If call of archive_read_support_format_raw fails, do not override the
error return value with the return value of
archive_read_support_format_empty(). Instead, return error code as expected.
The uint64_t variable propertiesSize is eventually casted to size_t
which, on 32 bit systems, can result in integer truncation.
In such a situation, it is possible that less than the minimum of 5
properties are parsed and processed, which will result in out of
boundary reads in init_decompression because the error check `if
(coder1->propertiesSize < 5)` still takes the uint64_t variable into
account.
Passwords for encryption must not be empty. Neither through command line
option nor through interactive input.
With this PR applied:
```
$ bsdtar --format zip --options zip:encryption -cf archive.zip input.txt
Enter passphrase: <press enter>
bsdtar: Encryption needs passphrase
```
Output with command line argument (unaffected by this PR):
```
$ bsdtar --format zip --options zip:encryption --passphrase '' -cf archive.zip input.txt
bsdtar: Empty passphrase is unacceptable
```
The outputs differ due to internal difference in handling the results.
It is still possible to supply a passphrase through command line
argument which cannot be entered interactively, i.e. $'\r\n'. See
https://github.com/libarchive/libarchive/pull/2115 for more details.
If libarchive is compiled on Windows without cygwin, strip \r and \n the
same way as it is done on POSIX systems.
Also, entering an empty password as "\r\n" should lead to an empty
string. Right now, the newlines are kept.
Proof of Concept:
1. Compile libarchive with Visual Studio
2. Create a password-protected ZIP file
```
PS> bsdtar.exe --format zip --options 'zip:encryption' -cf archive.zip input.txt
Enter passphrase: <press enter>
```
3. Extract ZIP file on Windows
```
PS> bsdtar.exe -xf archive.zip
Enter passphrase: <press enter>
```
4. Extract ZIP file on Linux
```
$ bsdtar -xf archive.zip
Enter passphrase: <press enter>
Enter passphrase:
```
As can be seen in step 4, it is impossible to extract the file on Linux
with interactive input, because \r and \n are stripped.
The only way to extract the content is through command line option
passphrase:
```
$ bsdtar -xf archive.zip --passphrase $'\r\n'
```
It's also true the other way around: Creating a ZIP file with an empty
password on Linux cannot be extracted interactively on Windows. Not
allowing empty passwords at all should be part of another PR. This one
is about unifying Windows and POSIX systems regarding newline handling.
These files are created when building libarchive within Visual Studio
Code, its CMake plugin and Visual Studio 2022 as compiler.
Without this change, Visual Studio Code shows hundreds of files in
GitLense plugin waiting to be commited.
Supplying a lot of "-" arguments to tools can lead to stack overflow due
to recursive *_getopt function calls.
Proof of Concept:
1. Compile libarchive with Visual Studio 2022
2. Call bsdtar with insane amount of arguments
```
PS> bsdtar.exe ("- "*10000).split(" ")
```
The event log shows that bsdtar.exe failed with `0xc00000fd` (stack
overflow).
If compiled with gcc, this does not happen by default because the code
is internally optimized to use this suggested loop instead. You have to
compile with CFLAGS="-O0" to provoke it with gcc as well.
Calling `__archive_mkstemp` can lead to access of an uninitialized
variable in `__archive_mktempx`, because `temp_name` is only initialized
if supplied `template` argument is `NULL`.
If `template` is not `NULL`, it is eventually compared with
`temp_name.s` anyway.
The fix is simple: Always initialize `temp_name`, which merely sets
values in the struct. No memory allocation occurs and the check leads to
the expected result.
How to reproduce:
1. Compile libarchive with Visual Studio 2022 and CMake's Debug profile
2. Run test `bsdtar_test_option_safe_writes`
3. A popup (Microsoft Visual C++ Runtime Library) appears, stating that
variable temp_name is being used without being initialized
Tobias Stoeckmann
