Merge pull request #11492 from DeterminateSystems/canon-nars

Make the NAR parser stricter and add some tests

src/libutil/archive.cc

@@ -82,7 +82,7 @@ void SourceAccessor::dumpPath(
                         name.erase(pos);
                     }
                     if (!unhacked.emplace(name, i.first).second)
-                        throw Error("file name collision in between '%s' and '%s'",
+                        throw Error("file name collision between '%s' and '%s'",
                             (path / unhacked[name]),
                             (path / i.first));
                 } else
@@ -128,9 +128,10 @@ void dumpString(std::string_view s, Sink & sink)
 }
 
 
-static SerialisationError badArchive(const std::string & s)
+template<typename... Args>
+static SerialisationError badArchive(std::string_view s, const Args & ... args)
 {
-    return SerialisationError("bad archive: " + s);
+    return SerialisationError("bad archive: " + s, args...);
 }
 
 
@@ -167,75 +168,65 @@ struct CaseInsensitiveCompare
 
 static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath & path)
 {
-    std::string s;
-
-    s = readString(source);
-    if (s != "(") throw badArchive("expected open tag");
-
-    std::map<Path, int, CaseInsensitiveCompare> names;
-
     auto getString = [&]() {
         checkInterrupt();
         return readString(source);
     };
 
-    // For first iteration
-    s = getString();
+    auto expectTag = [&](std::string_view expected) {
+        auto tag = getString();
+        if (tag != expected)
+            throw badArchive("expected tag '%s', got '%s'", expected, tag);
+    };
 
-    while (1) {
+    expectTag("(");
 
-        if (s == ")") {
-            break;
-        }
+    expectTag("type");
 
-        else if (s == "type") {
-            std::string t = getString();
+    auto type = getString();
 
-            if (t == "regular") {
+    if (type == "regular") {
         sink.createRegularFile(path, [&](auto & crf) {
-                    while (1) {
-                        s = getString();
+            auto tag = getString();
 
-                        if (s == "contents") {
-                            parseContents(crf, source);
-                        }
-
-                        else if (s == "executable") {
+            if (tag == "executable") {
                 auto s2 = getString();
                 if (s2 != "") throw badArchive("executable marker has non-empty value");
                 crf.isExecutable();
+                tag = getString();
             }
 
-                        else break;
-                    }
+            if (tag == "contents")
+                parseContents(crf, source);
+
+            expectTag(")");
         });
     }
 
-            else if (t == "directory") {
+    else if (type == "directory") {
         sink.createDirectory(path);
 
+        std::map<Path, int, CaseInsensitiveCompare> names;
+
         std::string prevName;
 
         while (1) {
-                    s = getString();
+            auto tag = getString();
 
-                    if (s == "entry") {
-                        std::string name;
+            if (tag == ")") break;
 
-                        s = getString();
-                        if (s != "(") throw badArchive("expected open tag");
+            if (tag != "entry")
+                throw badArchive("expected tag 'entry' or ')', got '%s'", tag);
 
-                        while (1) {
-                            s = getString();
+            expectTag("(");
 
-                            if (s == ")") {
-                                break;
-                            } else if (s == "name") {
-                                name = getString();
+            expectTag("name");
+
+            auto name = getString();
             if (name.empty() || name == "." || name == ".." || name.find('/') != std::string::npos || name.find((char) 0) != std::string::npos)
-                                    throw Error("NAR contains invalid file name '%1%'", name);
+                throw badArchive("NAR contains invalid file name '%1%'", name);
             if (name <= prevName)
-                                    throw Error("NAR directory is not sorted");
+                throw badArchive("NAR directory is not sorted");
             prevName = name;
             if (archiveSettings.useCaseHack) {
                 auto i = names.find(name);
@@ -245,42 +236,29 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath
                     name += std::to_string(++i->second);
                     auto j = names.find(name);
                     if (j != names.end())
-                                            throw Error("NAR contains file name '%s' that collides with case-hacked file name '%s'", prevName, j->first);
+                        throw badArchive("NAR contains file name '%s' that collides with case-hacked file name '%s'", prevName, j->first);
                 } else
                     names[name] = 0;
             }
-                            } else if (s == "node") {
-                                if (name.empty()) throw badArchive("entry name missing");
+
+            expectTag("node");
+
             parse(sink, source, path / name);
-                            } else
-                                throw badArchive("unknown field " + s);
+
+            expectTag(")");
         }
     }
 
-                    else break;
-                }
-            }
+    else if (type == "symlink") {
+        expectTag("target");
 
-            else if (t == "symlink") {
-                s = getString();
-
-                if (s != "target")
-                    throw badArchive("expected 'target' got " + s);
-
-                std::string target = getString();
+        auto target = getString();
         sink.createSymlink(path, target);
 
-                // for the next iteration
-                s = getString();
+        expectTag(")");
     }
 
-            else throw badArchive("unknown file type " + t);
-
-        }
-
-        else
-            throw badArchive("unknown field " + s);
-    }
+    else throw badArchive("unknown file type '%s'", type);
 }
 
 

tests/functional/nars.sh

@@ -67,6 +67,12 @@ expectStderr 1 nix-store --restore "$TEST_ROOT/out" < "$TEST_ROOT/tmp.nar" | gre
 rm -rf "$TEST_ROOT/case"
 opts=("--option" "use-case-hack" "true")
 nix-store "${opts[@]}" --restore "$TEST_ROOT/case" < case.nar
+[[ -e "$TEST_ROOT/case/xt_CONNMARK.h" ]]
+[[ -e "$TEST_ROOT/case/xt_CONNmark.h~nix~case~hack~1" ]]
+[[ -e "$TEST_ROOT/case/xt_connmark.h~nix~case~hack~2" ]]
+[[ -e "$TEST_ROOT/case/x/FOO" ]]
+[[ -d "$TEST_ROOT/case/x/Foo~nix~case~hack~1" ]]
+[[ -e "$TEST_ROOT/case/x/foo~nix~case~hack~2/a~nix~case~hack~1/foo" ]]
 nix-store "${opts[@]}" --dump "$TEST_ROOT/case" > "$TEST_ROOT/case.nar"
 cmp case.nar "$TEST_ROOT/case.nar"
 [ "$(nix-hash "${opts[@]}" --type sha256 "$TEST_ROOT/case")" = "$(nix-hash --flat --type sha256 case.nar)" ]
@@ -92,3 +98,31 @@ else
     [[ -e $TEST_ROOT/out/â ]]
     [[ -e $TEST_ROOT/out/â ]]
 fi
+
+# Unpacking a NAR with a NUL character in a file name should fail.
+rm -rf "$TEST_ROOT/out"
+expectStderr 1 nix-store --restore "$TEST_ROOT/out" < nul.nar | grepQuiet "NAR contains invalid file name 'f"
+
+# Likewise for a '.' filename.
+rm -rf "$TEST_ROOT/out"
+expectStderr 1 nix-store --restore "$TEST_ROOT/out" < dot.nar | grepQuiet "NAR contains invalid file name '.'"
+
+# Likewise for a '..' filename.
+rm -rf "$TEST_ROOT/out"
+expectStderr 1 nix-store --restore "$TEST_ROOT/out" < dotdot.nar | grepQuiet "NAR contains invalid file name '..'"
+
+# Likewise for a filename containing a slash.
+rm -rf "$TEST_ROOT/out"
+expectStderr 1 nix-store --restore "$TEST_ROOT/out" < slash.nar | grepQuiet "NAR contains invalid file name 'x/y'"
+
+# Likewise for an empty filename.
+rm -rf "$TEST_ROOT/out"
+expectStderr 1 nix-store --restore "$TEST_ROOT/out" < empty.nar | grepQuiet "NAR contains invalid file name ''"
+
+# Test that the 'executable' field cannot come before the 'contents' field.
+rm -rf "$TEST_ROOT/out"
+expectStderr 1 nix-store --restore "$TEST_ROOT/out" < executable-after-contents.nar | grepQuiet "expected tag ')', got 'executable'"
+
+# Test that the 'name' field cannot come before the 'node' field in a directory entry.
+rm -rf "$TEST_ROOT/out"
+expectStderr 1 nix-store --restore "$TEST_ROOT/out" < name-after-node.nar | grepQuiet "expected tag 'name'"