Make the NAR parser much stricter wrt field order

We really want to enforce a canonical representation since NAR
hashing/signing/deduplication depends on that.

src/libutil/archive.cc

@@ -168,120 +168,97 @@ 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");
-
     auto getString = [&]() {
         checkInterrupt();
         return readString(source);
     };
 
-    // For first iteration
+    auto expectTag = [&](std::string_view expected) {
-    s = getString();
+        auto tag = getString();
+        if (tag != expected)
+            throw badArchive("expected tag '%s', got '%s'", expected, tag);
+    };
 
-    while (1) {
+    expectTag("(");
 
-        if (s == ")") {
+    expectTag("type");
-            break;
-        }
 
-        else if (s == "type") {
+    auto type = getString();
-            std::string t = getString();
 
-            if (t == "regular") {
+    if (type == "regular") {
-                sink.createRegularFile(path, [&](auto & crf) {
+        sink.createRegularFile(path, [&](auto & crf) {
-                    while (1) {
+            auto tag = getString();
-                        s = getString();
 
-                        if (s == "contents") {
+            if (tag == "executable") {
-                            parseContents(crf, source);
+                auto s2 = getString();
-                        }
+                if (s2 != "") throw badArchive("executable marker has non-empty value");
-
+                crf.isExecutable();
-                        else if (s == "executable") {
+                tag = getString();
-                            auto s2 = getString();
-                            if (s2 != "") throw badArchive("executable marker has non-empty value");
-                            crf.isExecutable();
-                        }
-
-                        else break;
-                    }
-                });
             }
 
-            else if (t == "directory") {
+            if (tag == "contents")
-                sink.createDirectory(path);
+                parseContents(crf, source);
 
-                std::map<Path, int, CaseInsensitiveCompare> names;
+            expectTag(")");
-
+        });
-                std::string prevName;
-
-                while (1) {
-                    s = getString();
-
-                    if (s == "entry") {
-                        std::string name;
-
-                        s = getString();
-                        if (s != "(") throw badArchive("expected open tag '%s'", s);
-
-                        while (1) {
-                            s = getString();
-
-                            if (s == ")") {
-                                break;
-                            } else if (s == "name") {
-                                name = getString();
-                                if (name.empty() || name == "." || name == ".." || name.find('/') != std::string::npos || name.find((char) 0) != std::string::npos)
-                                    throw badArchive("NAR contains invalid file name '%1%'", name);
-                                if (name <= prevName)
-                                    throw badArchive("NAR directory is not sorted");
-                                prevName = name;
-                                if (archiveSettings.useCaseHack) {
-                                    auto i = names.find(name);
-                                    if (i != names.end()) {
-                                        debug("case collision between '%1%' and '%2%'", i->first, name);
-                                        name += caseHackSuffix;
-                                        name += std::to_string(++i->second);
-                                        auto j = names.find(name);
-                                        if (j != names.end())
-                                            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");
-                                parse(sink, source, path / name);
-                            } else
-                                throw badArchive("unknown field '%s'", s);
-                        }
-                    }
-
-                    else break;
-                }
-            }
-
-            else if (t == "symlink") {
-                s = getString();
-
-                if (s != "target")
-                    throw badArchive("expected 'target', got '%s'", s);
-
-                std::string target = getString();
-                sink.createSymlink(path, target);
-
-                // for the next iteration
-                s = getString();
-            }
-
-            else throw badArchive("unknown file type '%s'", t);
-
-        }
-
-        else
-            throw badArchive("unknown field '%s'", s);
     }
+
+    else if (type == "directory") {
+        sink.createDirectory(path);
+
+        std::map<Path, int, CaseInsensitiveCompare> names;
+
+        std::string prevName;
+
+        while (1) {
+            auto tag = getString();
+
+            if (tag == ")") break;
+
+            if (tag != "entry")
+                throw badArchive("expected tag 'entry' or ')', got '%s'", tag);
+
+            expectTag("(");
+
+            expectTag("name");
+
+            auto name = getString();
+            if (name.empty() || name == "." || name == ".." || name.find('/') != std::string::npos || name.find((char) 0) != std::string::npos)
+                throw badArchive("NAR contains invalid file name '%1%'", name);
+            if (name <= prevName)
+                throw badArchive("NAR directory is not sorted");
+            prevName = name;
+            if (archiveSettings.useCaseHack) {
+                auto i = names.find(name);
+                if (i != names.end()) {
+                    debug("case collision between '%1%' and '%2%'", i->first, name);
+                    name += caseHackSuffix;
+                    name += std::to_string(++i->second);
+                    auto j = names.find(name);
+                    if (j != names.end())
+                        throw badArchive("NAR contains file name '%s' that collides with case-hacked file name '%s'", prevName, j->first);
+                } else
+                    names[name] = 0;
+            }
+
+            expectTag("node");
+
+            parse(sink, source, path / name);
+
+            expectTag(")");
+        }
+    }
+
+    else if (type == "symlink") {
+        expectTag("target");
+
+        auto target = getString();
+        sink.createSymlink(path, target);
+
+        expectTag(")");
+    }
+
+    else throw badArchive("unknown file type '%s'", type);
 }
 
 

tests/functional/nars.sh

@@ -112,3 +112,11 @@ expectStderr 1 nix-store --restore "$TEST_ROOT/out" < slash.nar | grepQuiet "NAR
 # 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'"