From 421aa1add1cbae1fd51b8d9efa4ed47dce7a06ac Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Sep 2024 14:58:33 +0200 Subject: [PATCH 1/6] Add tests for invalid file names in NARs Note: in general, we rely on the OS to tell us if a name is invalid or if two names normalize in the same way. But for security, we do want to make sure that we catch '.', '..', slashes and NUL characters. (NUL characters aren't really a security issue, but since they would be truncated when we pass them to the OS, it would be canonicity problem.) --- tests/functional/dot.nar | Bin 0 -> 288 bytes tests/functional/dotdot.nar | Bin 0 -> 288 bytes tests/functional/empty.nar | Bin 0 -> 280 bytes tests/functional/nars.sh | 20 ++++++++++++++++++++ tests/functional/nul.nar | Bin 0 -> 288 bytes tests/functional/slash.nar | Bin 0 -> 288 bytes 6 files changed, 20 insertions(+) create mode 100644 tests/functional/dot.nar create mode 100644 tests/functional/dotdot.nar create mode 100644 tests/functional/empty.nar create mode 100644 tests/functional/nul.nar create mode 100644 tests/functional/slash.nar diff --git a/tests/functional/dot.nar b/tests/functional/dot.nar new file mode 100644 index 0000000000000000000000000000000000000000..3a9452f67fd7dc8b8c9328c767337c5c51b006c4 GIT binary patch literal 288 zcmd;OfPlQr3f;t_5|HVR1lLB%1_BGN=+`wFRFy{S)p`l zUI|zXmpOTfxnOf(@_JBxFnjXyQ&8k_xq}_5uP8OWG$*l$fdk4<&d)0Wx}lg2%Fjs6 R$;szJ_)8Ni4znK@9{^kc8+8Bx literal 0 HcmV?d00001 diff --git a/tests/functional/dotdot.nar b/tests/functional/dotdot.nar new file mode 100644 index 0000000000000000000000000000000000000000..f8d019c3926a8285fd3258798a8f88efc65df48d GIT binary patch literal 288 zcmd;OfPlQr3f;t_5|HVR1lLB%1_BGN=+`wFRFy{S)p`l zUI|zXmpOTfxnOgcpz8JXAPks2dHE?|d0hHo?qG-NFG@`>%}Fd`;DGXz^Yco8ZYXAh V@-tF%a`L$l{?des!_3FU2LN}%8>9dL literal 0 HcmV?d00001 diff --git a/tests/functional/empty.nar b/tests/functional/empty.nar new file mode 100644 index 0000000000000000000000000000000000000000..43434f2b4404161a74e8a90c0b5c8e3a11194fdf GIT binary patch literal 280 zcmd;OfPlQr3f;t_5|HVR1lLB%1_BGN=+`wFRFy{S)p`l zUI|zXmpOTfxnOgk${Aqh=jEq>#c}C_+0PEuSd^Mxnv+<>zyaka=jW9G?Jj16@-tF% Ra`L$l{?UYr!_3FU2LQ_<8%6*C literal 0 HcmV?d00001 diff --git a/tests/functional/nars.sh b/tests/functional/nars.sh index 9f5f43dc6..68b9b45d9 100755 --- a/tests/functional/nars.sh +++ b/tests/functional/nars.sh @@ -92,3 +92,23 @@ 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 ''" diff --git a/tests/functional/nul.nar b/tests/functional/nul.nar new file mode 100644 index 0000000000000000000000000000000000000000..9ae48baf6fa658433e37afdb6cae18b2d056a06c GIT binary patch literal 288 zcmd;OfPlQr3f;t_5|HVR1lLB%1_BGN=+`wFRFy{S)p`l zUI|zXmpOTfxnOgcq3Y8Z^1&>aJ$d;lV0m2nVeVju>Mu%7FU?6TV&H)Clk@XRfNm&e WgYq*{b8_;z5dPAHio?vu#RmYtiX0vQ literal 0 HcmV?d00001 diff --git a/tests/functional/slash.nar b/tests/functional/slash.nar new file mode 100644 index 0000000000000000000000000000000000000000..118a60216cc12d33df202aec4c5dd8fbfbcfce75 GIT binary patch literal 288 zcmd;OfPlQr3f;t_5|HVR1lLB%1_BGN=+`wFRFy{S)p`l zUI|zXmpOTfxnOgcq3SF2Atu1=$;(dx%j41ya|b(Ae^F|BX-;Ah0|%6!oS#<$bVD&4 Vl%J8BlatSd@Ruf39A-W)J^<^_9BBXm literal 0 HcmV?d00001 From 4de9587e50644b59ca7fcf0e1ad940d0dddcb89c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Sep 2024 15:27:55 +0200 Subject: [PATCH 2/6] Improve badArchive() --- src/libutil/archive.cc | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index e26b7eb93..d9a3afd41 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -128,9 +128,10 @@ void dumpString(std::string_view s, Sink & sink) } -static SerialisationError badArchive(const std::string & s) +template +static SerialisationError badArchive(std::string_view s, const Args & ... args) { - return SerialisationError("bad archive: " + s); + return SerialisationError("bad archive: " + s, args...); } @@ -223,7 +224,7 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath std::string name; s = getString(); - if (s != "(") throw badArchive("expected open tag"); + if (s != "(") throw badArchive("expected open tag '%s'", s); while (1) { s = getString(); @@ -233,9 +234,9 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath } else if (s == "name") { 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,7 +246,7 @@ 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; } @@ -253,7 +254,7 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath if (name.empty()) throw badArchive("entry name missing"); parse(sink, source, path / name); } else - throw badArchive("unknown field " + s); + throw badArchive("unknown field '%s'", s); } } @@ -265,7 +266,7 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath s = getString(); if (s != "target") - throw badArchive("expected 'target' got " + s); + throw badArchive("expected 'target', got '%s'", s); std::string target = getString(); sink.createSymlink(path, target); @@ -274,12 +275,12 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath s = getString(); } - else throw badArchive("unknown file type " + t); + else throw badArchive("unknown file type '%s'", t); } else - throw badArchive("unknown field " + s); + throw badArchive("unknown field '%s'", s); } } From 69bf9947c78ea80b761dae81053ad50ad7ad8532 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Sep 2024 15:29:54 +0200 Subject: [PATCH 3/6] Put 'names' in the right scope --- src/libutil/archive.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index d9a3afd41..9ddec44cd 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -173,8 +173,6 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath s = readString(source); if (s != "(") throw badArchive("expected open tag"); - std::map names; - auto getString = [&]() { checkInterrupt(); return readString(source); @@ -215,6 +213,8 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath else if (t == "directory") { sink.createDirectory(path); + std::map names; + std::string prevName; while (1) { From 27ec0def740208f6884a1aad862f4e2e181ca0a9 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Sep 2024 15:33:55 +0200 Subject: [PATCH 4/6] Typo --- src/libutil/archive.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index 9ddec44cd..c9362d6cb 100644 --- a/src/libutil/archive.cc +++ b/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 From 7aa3e7e3a5281acf350eff0fe039656cd4986e2c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Sep 2024 15:57:46 +0200 Subject: [PATCH 5/6] 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'" From 5737d31d4ea7afceaebde0a44fbf188bfe39783b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 12 Sep 2024 17:25:25 +0200 Subject: [PATCH 6/6] Test the case hack a bit more --- tests/functional/nars.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/functional/nars.sh b/tests/functional/nars.sh index 87fd7beec..28876e497 100755 --- a/tests/functional/nars.sh +++ b/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)" ]