diff --git a/doc/manual/change-authors.yml b/doc/manual/change-authors.yml index 313e0647c..f20cd3831 100644 --- a/doc/manual/change-authors.yml +++ b/doc/manual/change-authors.yml @@ -73,6 +73,9 @@ detroyejr: display_name: Jonathan De Troye github: detroyejr +edef: + github: edef1c + edolstra: display_name: Eelco Dolstra github: edolstra @@ -229,6 +232,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..466ca995c --- /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: [5553] +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 2a04e1797..094d3d3b5 100644 --- a/lix/libutil/archive.cc +++ b/lix/libutil/archive.cc @@ -384,7 +384,7 @@ struct Parser 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 274ac3515..f76775693 100644 --- a/tests/unit/libutil/archive.cc +++ b/tests/unit/libutil/archive.cc @@ -521,4 +521,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); +} }