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); +} }