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

@@ -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
 

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.

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<size_t>::max())
+#define READ_STRING() READ_STRING_LIMITED(1048576)
 #define READ_PADDING(size)                                                    \
     do {                                                                      \
         if ((size) % 8) {                                                     \

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