From 7aa3e7e3a5281acf350eff0fe039656cd4986e2c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Sep 2024 15:57:46 +0200 Subject: [PATCH] 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 | 177 ++++++++---------- .../functional/executable-after-contents.nar | Bin 0 -> 320 bytes tests/functional/name-after-node.nar | Bin 0 -> 288 bytes tests/functional/nars.sh | 8 + 4 files changed, 85 insertions(+), 100 deletions(-) create mode 100644 tests/functional/executable-after-contents.nar create mode 100644 tests/functional/name-after-node.nar diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index c9362d6cb..20d8a1e09 100644 --- a/src/libutil/archive.cc +++ b/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 - 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") { - sink.createRegularFile(path, [&](auto & crf) { - while (1) { - s = getString(); + if (type == "regular") { + sink.createRegularFile(path, [&](auto & crf) { + auto tag = getString(); - if (s == "contents") { - parseContents(crf, source); - } - - else if (s == "executable") { - auto s2 = getString(); - if (s2 != "") throw badArchive("executable marker has non-empty value"); - crf.isExecutable(); - } - - else break; - } - }); + if (tag == "executable") { + auto s2 = getString(); + if (s2 != "") throw badArchive("executable marker has non-empty value"); + crf.isExecutable(); + tag = getString(); } - else if (t == "directory") { - sink.createDirectory(path); + if (tag == "contents") + parseContents(crf, source); - std::map names; - - 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); + expectTag(")"); + }); } + + else if (type == "directory") { + sink.createDirectory(path); + + std::map 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); } diff --git a/tests/functional/executable-after-contents.nar b/tests/functional/executable-after-contents.nar new file mode 100644 index 0000000000000000000000000000000000000000..f8c003480d786957a141aca605ae3e78819f61d9 GIT binary patch literal 320 zcmah@Ne;p=3=Co|690fh4?HQPhDHiD3NC7YPiiK|3d_=X#>@ERe!+2UeGYy6P5|HVR1lLB%1_BGN=+`wFRFy{S)p`l zUI|zXmpOU)DPVJO$;0enhniQEnqHcdSj4~qqH1W`puGQgd?hxe)Hwgo?x5 YotKykwvQPqo|c~vX2I--sYmAn07Q};jQ{`u literal 0 HcmV?d00001 diff --git a/tests/functional/nars.sh b/tests/functional/nars.sh index 68b9b45d9..87fd7beec 100755 --- a/tests/functional/nars.sh +++ b/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'"