From 0eb56266a046bae7e7a97c06b946fb5749050c10 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 20 Apr 2026 11:30:02 +0200 Subject: [PATCH] libutil: fix nar parser buffer overflow string data shares a buffer with the binary string length field. size calculations for string read buffers always include the length field; sufficiently large length fields can cause these calculations to wrap. a malicious nar could use this for OOB writes in the daemon (as root). since we use strings only as tags for archive members and for symlinks with their OS-dependent length limits we can simply limit string size. 1 MiB should be sufficient for all symlinks, and tags are always tiny. Change-Id: I89fb05f73c1dbeda45d91244aba4cd526a3d83e1 --- doc/manual/change-authors.yml | 6 +++++ doc/manual/rl-next/unsigned-overflow-vuln.md | 28 ++++++++++++++++++++ lix/libutil/archive.cc | 24 ++++++++++------- tests/unit/libutil/archive.cc | 23 ++++++++++++++++ 4 files changed, 72 insertions(+), 9 deletions(-) create mode 100644 doc/manual/rl-next/unsigned-overflow-vuln.md diff --git a/doc/manual/change-authors.yml b/doc/manual/change-authors.yml index f79395a46..7510834f1 100644 --- a/doc/manual/change-authors.yml +++ b/doc/manual/change-authors.yml @@ -70,6 +70,9 @@ detroyejr: display_name: Jonathan De Troye github: detroyejr +edef: + github: edef1c + edolstra: display_name: Eelco Dolstra github: edolstra @@ -197,6 +200,9 @@ roberth: display_name: Robert Hensing github: roberth +sandydoo: + github: sandydoo + seppel3210: github: Seppel3210 diff --git a/doc/manual/rl-next/unsigned-overflow-vuln.md b/doc/manual/rl-next/unsigned-overflow-vuln.md new file mode 100644 index 000000000..a81816a73 --- /dev/null +++ b/doc/manual/rl-next/unsigned-overflow-vuln.md @@ -0,0 +1,28 @@ +--- +synopsis: "Fix unsigned overflow leading to out-of-band write in the NAR parser" +cls: [5537] +category: "Fixes" +credits: [horrors, raito, edef, sandydoo] +issues: [] +--- + +The NAR parser contained an unsigned integer overflow that could be used by an +attacker to write arbitrary data to an unknown memory location and possibly +achieve code execution. A successful attack on the system-wide Lix daemon +could lead to privilege escalation to root. Any process that involves NAR +serialization could trigger this issue, including (but not limited to) + + - local user interaction, whether the users are trusted or untrusted + - malicious substituters sending malformed NARs + - remote builders sending malformed build results + - remote daemons sending malformed inputs when requesting remote builds + +Successful attacks using this bug require ASLR weakening of some sort, whether +by architecture constraints (e.g. on 32 bit systems, where little randomization +is possible) or system configuration (e.g. low ASLR entropy when loading +libraries), and millions of attempts. Local attacks can be mounted in less than +an hour. Remote builds typically require a fresh SSH connection for each build +and are thus less susceptible. Only one attempt can be made by substituters for +every build using substituters, they are thus not a likely vector for attacks. + +At the time of writing, MITRE has not assigned this a CVE yet. diff --git a/lix/libutil/archive.cc b/lix/libutil/archive.cc index 80e0c9781..2169bd493 100644 --- a/lix/libutil/archive.cc +++ b/lix/libutil/archive.cc @@ -367,16 +367,22 @@ struct Parser buffer.clear(); \ u; \ }) -#define READ_STRING_LIMITED(limit) \ - ({ \ - size_t len = FETCH_INT(size_t); \ - co_yield WantBytes{len + (8 - len % 8) % 8}; \ - StringSource src(std::string_view(buffer.data(), buffer.size())); \ - auto str = readString(src, (limit)); \ - buffer.clear(); \ - std::move(str); \ +#define READ_STRING_LIMITED(limit) \ + ({ \ + size_t len = FETCH_INT(size_t); \ + if (len > (limit)) { \ + throw SerialisationError( \ + "found malformed string tag. input may be a compressed NAR, which cannot be read " \ + "directly" \ + ); \ + } \ + co_yield WantBytes{len + (8 - len % 8) % 8}; \ + StringSource src(std::string_view(buffer.data(), buffer.size())); \ + auto str = readString(src, (limit)); \ + buffer.clear(); \ + std::move(str); \ }) -#define READ_STRING() READ_STRING_LIMITED(std::numeric_limits::max()) +#define READ_STRING() READ_STRING_LIMITED(1048576) #define READ_PADDING(size) \ do { \ if ((size) % 8) { \ diff --git a/tests/unit/libutil/archive.cc b/tests/unit/libutil/archive.cc index 51189d092..05b997701 100644 --- a/tests/unit/libutil/archive.cc +++ b/tests/unit/libutil/archive.cc @@ -509,4 +509,27 @@ INSTANTIATE_TEST_SUITE_P( concat({header, make_directory({{"DE", make_file(false, "meow")}, {"de", make_file(false, "mrrp")}})}) )) ); + +TEST_F(NarTest, stringSizeLimit) +{ + GeneratorSource source([]() -> Generator { + const char preamble[] = + "\x0d\x00\x00\x00\x00\x00\x00\x00nix-archive-1\x00\x00\x00" + "\x01\x00\x00\x00\x00\x00\x00\x00(\x00\x00\x00\x00\x00\x00\x00" + "\x04\x00\x00\x00\x00\x00\x00\x00type\x00\x00\x00\x00"; + co_yield Bytes{preamble, sizeof(preamble) - 1}; + // the nar parser keeps all strings in a buffer with the 8 byte length prefix in front. + // sufficiently large strings overflowed caused the buffer size calculation to overflow + // and thus allowed out-of-bounds writes in the daemon and potentially privesc to root. + co_yield Bytes{"\xf7\xff\xff\xff\xff\xff\xff\xff", 8}; + // overflow would happen while reading data + while (true) { + co_yield Bytes{"foo-", 4}; + } + }()); + + auto parser = nar::parse(source); + + ASSERT_THROW(parser.next(), SerialisationError); +} }