Skip to content

Commit

Permalink
Remove the backdoor found in 5.6.0 and 5.6.1 (CVE-2024-3094).
Browse files Browse the repository at this point in the history
While the backdoor was inactive (and thus harmless) without inserting
a small trigger code into the build system when the source package was
created, it's good to remove this anyway:

  - The executable payloads were embedded as binary blobs in
    the test files. This was a blatant violation of the
    Debian Free Software Guidelines.

  - On machines that see lots bots poking at the SSH port, the backdoor
    noticeably increased CPU load, resulting in degraded user experience
    and thus overwhelmingly negative user feedback.

  - The maintainer who added the backdoor has disappeared.

  - Backdoors are bad for security.

This reverts the following without making any other changes:

6e63681 Tests: Update two test files.
a3a29bb Tests: Test --single-stream can decompress bad-3-corrupt_lzma2.xz.
0b4ccc9 Tests: Update RISC-V test files.
8c9b8b2 liblzma: Fix typos in crc32_fast.c and crc64_fast.c.
82ecc53 liblzma: Fix false Valgrind error report with GCC.
cf44e4b Tests: Add a few test files.
3060e10 Tests: Use smaller dictionary size in RISC-V test files.
e2870db Tests: Add two RISC-V Filter test files.

The RISC-V test files also have real content that tests the filter
but the real content would fit into much smaller files. A generator
program would need to be available as well.

Thanks to Andres Freund for finding and reporting it and making
it public quickly so others could act without a delay.
See: https://www.openwall.com/lists/oss-security/2024/03/29/4
  • Loading branch information
Larhzu committed Apr 9, 2024
1 parent f9cf4c0 commit e93e13c
Show file tree
Hide file tree
Showing 12 changed files with 8 additions and 66 deletions.
7 changes: 5 additions & 2 deletions src/liblzma/check/crc32_fast.c
Expand Up @@ -135,8 +135,11 @@ typedef uint32_t (*crc32_func_type)(
// This resolver is shared between all three dispatch methods. It serves as
// the ifunc resolver if ifunc is supported, otherwise it is called as a
// regular function by the constructor or first call resolution methods.
// The function attributes are needed for safe IFUNC resolver usage with GCC.
lzma_resolver_attributes
// The __no_profile_instrument_function__ attribute support is checked when
// determining if ifunc can be used, so it is safe to use here.
#ifdef CRC_USE_IFUNC
__attribute__((__no_profile_instrument_function__))
#endif
static crc32_func_type
crc32_resolve(void)
{
Expand Down
4 changes: 3 additions & 1 deletion src/liblzma/check/crc64_fast.c
Expand Up @@ -98,7 +98,9 @@ typedef uint64_t (*crc64_func_type)(
# pragma GCC diagnostic ignored "-Wunused-function"
#endif

lzma_resolver_attributes
#ifdef CRC_USE_IFUNC
__attribute__((__no_profile_instrument_function__))
#endif
static crc64_func_type
crc64_resolve(void)
{
Expand Down
25 changes: 0 additions & 25 deletions src/liblzma/check/crc_common.h
Expand Up @@ -128,31 +128,6 @@
# endif
#endif

#ifdef CRC_USE_IFUNC
// Two function attributes are needed to make IFUNC safe with GCC.
//
// no-omit-frame-pointer prevents false Valgrind issues when combined with
// a few other compiler flags. The optimize attribute is supported on
// GCC >= 4.4 and is not supported with Clang.
# if TUKLIB_GNUC_REQ(4,4) && !defined(__clang__)
# define no_omit_frame_pointer \
__attribute__((optimize("no-omit-frame-pointer")))
# else
# define no_omit_frame_pointer
# endif

// The __no_profile_instrument_function__ attribute support is checked when
// determining if ifunc can be used, so it is safe to use unconditionally.
// This attribute is needed because GCC can add profiling to the IFUNC
// resolver, which calls functions that have not yet been relocated leading
// to a crash on liblzma start up.
# define lzma_resolver_attributes \
__attribute__((__no_profile_instrument_function__)) \
no_omit_frame_pointer
#else
# define lzma_resolver_attributes
#endif

// For CRC32 use the generic slice-by-eight implementation if no optimized
// version is available.
#if !defined(CRC32_ARCH_OPTIMIZED) && !defined(CRC32_GENERIC)
Expand Down
27 changes: 0 additions & 27 deletions tests/files/README
Expand Up @@ -41,8 +41,6 @@
good-0catpad-empty.xz has two zero-Block Streams concatenated with
four-byte Stream Padding between the Streams.

good-2cat.xz has two Streams with one Block each.

good-1-check-none.xz has one Stream with one Block with two
uncompressed LZMA2 chunks and no integrity check.

Expand Down Expand Up @@ -83,14 +81,6 @@
good-1-arm64-lzma2-2.xz is like good-1-arm64-lzma2-1.xz but with
non-zero start offset. XZ Embedded doesn't support this file.

good-1-riscv-lzma2-1.xz uses the RISC-V filter and LZMA2. The
uncompressed data is constructed so it tests all of the instructions
that should be encoded and a few that should not. Additionally, the
file contains random bytes to help test unforeseen corner cases.

good-1-riscv-lzma2-2.xz is like good-1-riscv-lzma2-1.xz but with
non-zero start offset. XZ Embedded doesn't support this file.

good-1-lzma2-1.xz has two LZMA2 chunks, of which the second sets
new properties.

Expand Down Expand Up @@ -294,11 +284,6 @@
Uncompressed Size bytes of output will have been produced but
the LZMA2 decoder doesn't indicate end of stream.

bad-3-corrupt_lzma2.xz has three Streams in it. The first and third
streams are valid xz Streams. The middle Stream has a correct Stream
Header, Block Header, Index and Stream Footer. Only the LZMA2 data
is corrupt. This file should decompress if --single-stream is used.


3. Descriptions of Individual .lzma Files

Expand All @@ -315,14 +300,6 @@
will give an error at the end of the file after producing the
correct uncompressed output.

good-small_compressed.lzma was created with a small dictionary (2^16).
It contains the string "Hello World" repeated 100,000 times. This tests
match decoding and wrapping the dictionary.

good-large_compressed.lzma was created with a mix of repeated
characters and random data to test a data stream containing many
matches and many literals.


3.2. Bad Files

Expand All @@ -344,10 +321,6 @@
bad-too_small_size-without_eopm-3.lzma is like -1 above but instead
of a literal the problem occurs in the middle of a match.

bad-dict_size.lzma has a valid dictionary size according to the .lzma
File Format, but will be rejected by XZ Utils because it is not 2^n or
2^n + 2^(n-1).


4. Descriptions of Individual .lz (lzip) Files

Expand Down
Binary file removed tests/files/bad-3-corrupt_lzma2.xz
Binary file not shown.
Binary file removed tests/files/bad-dict_size.lzma
Binary file not shown.
Binary file removed tests/files/good-1-riscv-lzma2-1.xz
Binary file not shown.
Binary file removed tests/files/good-1-riscv-lzma2-2.xz
Binary file not shown.
Binary file removed tests/files/good-2cat.xz
Binary file not shown.
Binary file removed tests/files/good-large_compressed.lzma
Binary file not shown.
Binary file removed tests/files/good-small_compressed.lzma
Binary file not shown.
11 changes: 0 additions & 11 deletions tests/test_files.sh
Expand Up @@ -149,17 +149,6 @@ else
exit 1
fi

# Test that --single-stream can decompress bad-3-corrupt_lzma2.xz.
# The first Stream in this file should decompress without errors.
# This file cannot be decompressed with xzdec.
I="$srcdir/files/bad-3-corrupt_lzma2.xz"
if test -z "$XZ" || "$XZ" -dc --single-stream $NO_WARN "$I" > /dev/null; then
:
else
echo "Good first Stream failed xz with --single-stream: $I"
exit 1
fi


#########
# .lzma #
Expand Down

14 comments on commit e93e13c

@ItzSwirlz
Copy link

@ItzSwirlz ItzSwirlz commented on e93e13c Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mazel tov :) Lets hope this never happens again.

@Eckoa
Copy link

@Eckoa Eckoa commented on e93e13c Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Backdoors are bad for security.

gave me a good chuckle, one of my favorite commit messages by far. πŸ˜„

Good luck and Godspeed, thank you for all your work these years and sorry if this is just noise.

@dylanmtaylor
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to lie, I laughed at the commit message

@UnknownSuperficialNight

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XD cursed commit message

@danielgran
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love ya

@slavanap
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Backdoors are bad for security.

oh dang
removes backdoor from my house

@Euro-pol
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@CR3Swapper
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

@cybik
Copy link

@cybik cybik commented on e93e13c Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, while I applaud everyone keeping a cool head and responding to this issue promptly, the fact of the matter is that these files still "exist" within the history of this repository.

At the risk of breaking build automata everywhere, may I suggest abusing git filter-branch to purge the backdoor payloads from this repository permanently?

@Larhzu
Copy link
Member Author

@Larhzu Larhzu commented on e93e13c Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be discussed in #103. There are pros and cons. Currently the thinking is more towards not rewriting history (200 commits!). The backdoor files are harmless without inserting a trigger code when a release tarball is created.

@N-R-K
Copy link

@N-R-K N-R-K commented on e93e13c Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backdoors are bad for security

Can't argue with this logic.

@marcoss2009
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you πŸ₯ΉπŸ™ŒπŸΌ

@carlosmundaray
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

back door happy to sad πŸ₯ΉπŸ™ŒπŸΌ

@pixeldoc2000
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.