diff --git a/.github/scripts/build_packages.py b/.github/scripts/build_packages.py index 5e32861..6c89fd8 100644 --- a/.github/scripts/build_packages.py +++ b/.github/scripts/build_packages.py @@ -31,7 +31,10 @@ import tempfile from collections import namedtuple -ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +# realpath, not abspath: the check that keeps --out outside the repository +# compares resolved paths, so a symbolic link or a junction pointing into the +# tree cannot walk the packages into it (outside review of #143). +ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))) TEMPLATES = os.path.join(ROOT, "packaging") TEMPLATE_SUFFIX = ".in" PLACEHOLDER = re.compile(r"\{\{([A-Z0-9_]+)\}\}") @@ -103,9 +106,26 @@ def refuse(message): raise SystemExit("build_packages: %s" % message) -def read_text(path): - with open(path, encoding="utf-8-sig") as handle: - return handle.read().replace("\r\n", "\n") +def read_text(path, what="the file"): + """A text file, or a refusal naming it - never a traceback. + + A file that is missing, unreadable or not UTF-8 is an input a person got + wrong, and the answer has to say which file and what to do, not print a + Python exception (outside review of #143). + """ + try: + with open(path, encoding="utf-8-sig") as handle: + return handle.read().replace("\r\n", "\n") + except UnicodeDecodeError: + refuse("%s is not UTF-8 text, so it is not %s" % (path, what)) + except OSError as err: + refuse("cannot read %s %s: %s" % (what, path, err.strerror or err)) + + +# A release's checksum file is under a kilobyte - 970 bytes for v0.4.0. Anything +# near this is another file passed by mistake, an archive for instance, and is +# refused by size before a byte of it is read. +SUMS_LIMIT = 1024 * 1024 def repository(): @@ -150,8 +170,15 @@ def read_sums(path): mode, and that star is not part of the name. A file saved on Windows may carry a byte order mark and CRLF. None of that may reach an address. """ + hint = "Download verify-SHA256SUMS.txt from the release you are packaging" + try: + size = os.path.getsize(path) + except OSError as err: + refuse("cannot read the checksum file %s: %s. %s" % (path, err.strerror or err, hint)) + if size > SUMS_LIMIT: + refuse("%s is %d bytes, so it is not a release's checksum file. %s" % (path, size, hint)) sums = {} - for number, line in enumerate(read_text(path).split("\n"), 1): + for number, line in enumerate(read_text(path, "a checksum file").split("\n"), 1): if not line.strip(): continue found = re.fullmatch(r"([0-9A-Fa-f]{64}) [ *](\S.*)", line.strip()) @@ -322,7 +349,7 @@ def destination(package, relative, version): def check_out(out): """--out is outside the repository and empty or absent, or a refusal.""" - out = os.path.abspath(out) + out = os.path.realpath(out) root = os.path.normcase(ROOT) if os.path.normcase(out) == root or os.path.normcase(out).startswith(root + os.sep): refuse("--out %s is inside the repository. Rendered packages are not source - put " @@ -342,8 +369,11 @@ def build(tag, sums_path, out): refuse("the icon %s is not in the repository" % ICON) parent = os.path.dirname(out) - os.makedirs(parent, exist_ok=True) - work = tempfile.mkdtemp(prefix=".packages-", dir=parent) + try: + os.makedirs(parent, exist_ok=True) + work = tempfile.mkdtemp(prefix=".packages-", dir=parent) + except OSError as err: + refuse("cannot create a working folder in %s: %s" % (parent, err.strerror or err)) try: used = set() known = set() @@ -366,6 +396,8 @@ def build(tag, sums_path, out): if os.path.isdir(out): os.rmdir(out) os.rename(work, out) + except OSError as err: + refuse("cannot write the packages to %s: %s. Nothing was left behind" % (out, err.strerror or err)) finally: if os.path.isdir(work): shutil.rmtree(work) diff --git a/internal/guard/packaging_test.go b/internal/guard/packaging_test.go index a9b05ff..67527ba 100644 --- a/internal/guard/packaging_test.go +++ b/internal/guard/packaging_test.go @@ -74,6 +74,13 @@ func fixtureSums() []byte { // renderPackages runs the renderer the way a person does. func renderPackages(t *testing.T, tag string, sums []byte, out string) rendering { + t.Helper() + return renderFrom(t, tag, sumsFile(t, sums), out) +} + +// renderFrom is renderPackages with the checksum file named rather than +// written, so a guard can hand it a path to a file that is not there. +func renderFrom(t *testing.T, tag, sumsPath, out string) rendering { t.Helper() python := pythonForGate(t) // The interpreter is the one found on PATH, the script is a file of this @@ -81,7 +88,7 @@ func renderPackages(t *testing.T, tag string, sums []byte, out string) rendering // just wrote - nothing here comes from anything a person typed. // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command cmd := exec.Command(python, packagingScript(t), - "--tag", tag, "--sums", sumsFile(t, sums), "--out", out) + "--tag", tag, "--sums", sumsPath, "--out", out) cmd.Dir = repoRoot(t) said, err := cmd.CombinedOutput() code := 0 diff --git a/internal/guard/packagingrefusal_test.go b/internal/guard/packagingrefusal_test.go index 3929198..42c6e92 100644 --- a/internal/guard/packagingrefusal_test.go +++ b/internal/guard/packagingrefusal_test.go @@ -1,10 +1,13 @@ package guard import ( + "bytes" + "fmt" "os" "os/exec" "path/filepath" "regexp" + "runtime" "strings" "testing" ) @@ -109,6 +112,86 @@ func entriesOf(t *testing.T, dir string) string { return strings.Join(names, ", ") } +// A file the renderer cannot read, and a destination that leads into the +// repository by another name, are refused with a sentence rather than a Python +// traceback or a write into the tree. An outside review of #143 found both: a +// missing --sums printed an exception, and --out was checked by how it was +// spelled rather than by where it leads. +func TestTheRendererRefusesAFileItCannotReadAndAPathThatLeadsIntoTheTree(t *testing.T) { + dir := t.TempDir() + notUTF8 := filepath.Join(dir, "latin1.txt") + huge := filepath.Join(dir, "huge.txt") + aFile := filepath.Join(dir, "a-file-not-a-folder") + for path, body := range map[string][]byte{ + notUTF8: {0xff, 0xfe, 0x41, 0x0a}, + huge: bytes.Repeat([]byte("a"), 2<<20), + aFile: []byte("x"), + } { + if err := os.WriteFile(path, body, 0o600); err != nil { + t.Fatal(err) + } + } + + // The link points at an EMPTY folder made for this guard inside the tree, + // not at the tree itself, so no cleanup that followed it could reach + // anything else. The link is removed before the temporary directory that + // holds it - cleanups run last registered first. + target := filepath.Join(repoRoot(t), "packaging-guard-link-target") + if _, err := os.Stat(target); err == nil { + t.Fatalf("%s already exists, so this guard cannot tell what the renderer wrote there", target) + } + if err := os.Mkdir(target, 0o700); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(target) }) + link := filepath.Join(t.TempDir(), "into-the-tree") + if err := makeDirectoryLink(link, target); err != nil { + t.Fatalf("making a link to %s: %v", target, err) + } + t.Cleanup(func() { _ = os.Remove(link) }) + + for _, c := range []struct{ what, sums, out, says string }{ + {"a checksum file that is not there", filepath.Join(dir, "missing.txt"), + filepath.Join(t.TempDir(), "packages"), "cannot read the checksum file"}, + {"a checksum file that is not UTF-8", notUTF8, + filepath.Join(t.TempDir(), "packages"), "is not UTF-8 text"}, + {"a file far too big to be a checksum file", huge, + filepath.Join(t.TempDir(), "packages"), "is not a release's checksum file"}, + {"a destination that leads into the tree through a link", sumsFile(t, fixtureSums()), + filepath.Join(link, "packages"), "inside the repository"}, + {"a destination under something that is a file", sumsFile(t, fixtureSums()), + filepath.Join(aFile, "packages"), "cannot create a working folder"}, + } { + r := renderFrom(t, packagingTag, c.sums, c.out) + if r.code != 1 || !strings.HasPrefix(r.said, "build_packages: ") || strings.Contains(r.said, "Traceback") { + t.Errorf("%s: exit %d, and a refusal is exit 1 with a sentence, not a crash:\n%s", c.what, r.code, r.said) + continue + } + if !strings.Contains(r.said, c.says) { + t.Errorf("%s: the refusal does not say %q:\n%s", c.what, c.says, r.said) + } + } + if left := entriesOf(t, target); left != "" { + t.Errorf("the renderer wrote into the tree through the link: %s", left) + } +} + +// makeDirectoryLink makes link lead to target: a junction on Windows, which +// needs no privilege where a symbolic link does, and a symbolic link elsewhere. +func makeDirectoryLink(link, target string) error { + if runtime.GOOS != "windows" { + return os.Symlink(target, link) + } + // Both paths are ones this guard just chose, under its own temporary + // directory and the repository - nothing a person typed. + // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command + out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput() + if err != nil { + return fmt.Errorf("%v: %s", err, out) + } + return nil +} + // sha256sum writes ' ' in text mode and ' *' in // binary mode, and a file saved on Windows may carry a byte order mark and // CRLF. The star is not part of the name, and none of it may reach an address