diff --git a/.github/scripts/build_packages.py b/.github/scripts/build_packages.py index 5e328619..cba819c5 100644 --- a/.github/scripts/build_packages.py +++ b/.github/scripts/build_packages.py @@ -27,11 +27,15 @@ import os import re import shutil +import stat import sys 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_]+)\}\}") @@ -104,10 +108,22 @@ def refuse(message): def read_text(path): + """A file of this repository - the templates, go.mod, CNAME, the changelog. + + These are not inputs a person chooses, so a failure here is a broken + checkout and a traceback says so. What a person DOES choose - the checksum + file - is read by read_sums, which refuses with a sentence instead. + """ with open(path, encoding="utf-8-sig") as handle: return handle.read().replace("\r\n", "\n") +# 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 +# reading stops one byte past it. +SUMS_LIMIT = 1024 * 1024 + + def repository(): """owner/name, from the module path - the one place the address is written.""" found = re.search(r"^module github\.com/([^/\s]+/[^/\s]+)\s*$", @@ -150,8 +166,26 @@ 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. """ + # Opened once and read at most one byte past the limit, from a regular file + # only: a size asked for first and a read made after could see two different + # files, and a pipe or a device has no size to ask (outside review of #145). + hint = "Download verify-SHA256SUMS.txt from the release you are packaging and pass that" + try: + with open(path, "rb") as handle: + if not stat.S_ISREG(os.fstat(handle.fileno()).st_mode): + refuse("%s is not a file. %s" % (path, hint)) + data = handle.read(SUMS_LIMIT + 1) + except OSError as err: + refuse("cannot read the checksum file %s: %s. %s" % (path, err.strerror or err, hint)) + if len(data) > SUMS_LIMIT: + refuse("%s is larger than %d bytes, so it is not a release's checksum file. %s" + % (path, SUMS_LIMIT, hint)) + try: + text = data.decode("utf-8-sig").replace("\r\n", "\n") + except UnicodeDecodeError: + refuse("%s is not UTF-8 text, so it is not a release's checksum file. %s" % (path, hint)) sums = {} - for number, line in enumerate(read_text(path).split("\n"), 1): + for number, line in enumerate(text.split("\n"), 1): if not line.strip(): continue found = re.fullmatch(r"([0-9A-Fa-f]{64}) [ *](\S.*)", line.strip()) @@ -322,17 +356,46 @@ 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 " "them somewhere outside it" % out) - if os.path.exists(out) and (not os.path.isdir(out) or os.listdir(out)): - refuse("--out %s already holds something. Nothing is overwritten - pass an empty " - "or new directory" % out) + if os.path.exists(out): + try: + holds = not os.path.isdir(out) or bool(os.listdir(out)) + except OSError as err: + refuse("cannot look inside --out %s: %s. Pass a new directory, or one you can " + "read and write" % (out, err.strerror or err)) + if holds: + refuse("--out %s already holds something. Nothing is overwritten - pass an empty " + "or new directory" % out) return out +def missing_folders(path): + """The folders of path that do not exist yet, deepest first - what this run + will have made, and the most a failed run may take away again.""" + made = [] + while not os.path.exists(path): + made.append(path) + up = os.path.dirname(path) + if up == path: + break + path = up + return made + + +def remove_empty(folders): + """Remove the folders this run made, deepest first, stopping at the first one + that is not empty - it then holds something that is not ours.""" + for folder in folders: + try: + os.rmdir(folder) + except OSError: + return + + def build(tag, sums_path, out): """Render every package into out, all or nothing.""" version = parse_tag(tag) @@ -342,8 +405,15 @@ 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) + made = missing_folders(parent) + try: + os.makedirs(parent, exist_ok=True) + work = tempfile.mkdtemp(prefix=".packages-", dir=parent) + except OSError as err: + remove_empty(made) + refuse("cannot create a working folder in %s: %s. Check that you can write there, " + "or pass another --out" % (parent, err.strerror or err)) + finished = False try: used = set() known = set() @@ -366,9 +436,17 @@ def build(tag, sums_path, out): if os.path.isdir(out): os.rmdir(out) os.rename(work, out) + finished = True + except OSError as err: + refuse("cannot write the packages to %s: %s. Check that you can write there, or " + "pass another --out. Nothing was left behind" % (out, err.strerror or err)) finally: + # A refusal is a SystemExit, so it reaches here too: nothing half + # rendered, and no folder this run made, outlives a run that failed. if os.path.isdir(work): shutil.rmtree(work) + if not finished: + remove_empty(made) return out diff --git a/internal/guard/packaging_test.go b/internal/guard/packaging_test.go index a9b05ffe..67527ba6 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 39291985..46ed4e71 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,146 @@ 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"}, + // A device opens and reads like a file and has no size worth asking: + // NUL answers nothing, /dev/zero answers forever. + {"a checksum file that is a device", map[bool]string{true: "NUL", false: "/dev/zero"}[runtime.GOOS == "windows"], + filepath.Join(t.TempDir(), "packages"), "is not a file"}, + } { + 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) + } +} + +// A run that fails part way leaves nothing behind - neither its working +// folder nor the folders it made to hold --out - and a destination it cannot +// look into is refused with a sentence. An outside review of #145 found all +// three: the parents made by makedirs outlived a refusal that said "Nothing was +// left behind", no case reached the handler for a write that fails after the +// working folder exists, and os.listdir on --out could still raise. +func TestTheRendererLeavesNothingBehindWhenItFailsPartWay(t *testing.T) { + unreleased := []byte(strings.ReplaceAll(string(fixtureSums()), "0.4.0", "9.9.9")) + nested := t.TempDir() + long := t.TempDir() + for _, c := range []struct { + what, tag string + sums []byte + out, base string + says string + }{ + // The refusal comes from the changelog, after the parents are made. + {"a refusal after new parent folders were made", "v9.9.9", unreleased, + filepath.Join(nested, "new", "deeper", "packages"), nested, "CHANGELOG.md"}, + // Every file is written into the working folder first, and the name is + // too long for any file system this runs on only at the last rename. + {"a write that fails after the working folder exists", packagingTag, fixtureSums(), + filepath.Join(long, strings.Repeat("x", 300)), long, "cannot write the packages"}, + } { + r := renderPackages(t, c.tag, 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, c.base); left != "" { + t.Errorf("%s: the failed run left %s behind in %s", c.what, left, c.base) + } + } + + // A folder this account cannot list, asked of the system first rather than + // assumed - an administrator can list some of these, and then the case says + // so instead of passing on nothing. + denied := map[string]string{"windows": `C:\System Volume Information`, "darwin": "/private/var/root"}[runtime.GOOS] + if denied == "" { + denied = "/root" + } + if _, err := os.ReadDir(denied); !os.IsPermission(err) { + t.Logf("NOT ASKED: %s answered %v rather than a refusal to list it, so there is no folder here "+ + "this account cannot look into", denied, err) + return + } + r := renderPackages(t, packagingTag, fixtureSums(), denied) + if r.code != 1 || !strings.Contains(r.said, "cannot look inside --out") || strings.Contains(r.said, "Traceback") { + t.Errorf("a destination this account cannot list: exit %d, and it has to be refused with a sentence:\n%s", + r.code, r.said) + } +} + +// 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("making a junction %s to %s: %w (%s)", link, target, 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 diff --git a/packaging/README.md b/packaging/README.md index cc7acb4b..9d176d01 100644 --- a/packaging/README.md +++ b/packaging/README.md @@ -10,6 +10,18 @@ the product name and the licence, are held to their Go originals by a guard. python .github/scripts/build_packages.py --tag v0.4.0 \ --sums verify-SHA256SUMS.txt --out +The renderer refuses, with a sentence that names the input and says what to do, +and never a traceback: a tag that is a release candidate or not a tag, a +checksum file of another release or missing an archive, a version the changelog +never dated, a checksum file that is missing, unreadable, not a regular file, +not UTF-8 or over a megabyte (a release's is under a kilobyte), a destination +inside the repository - compared as a resolved path, so a link or a junction +does not get round it - or one that already holds something or cannot be looked +into, and a value that would break the file it lands in. It renders into a +working folder beside the destination and renames it at the end, and a run that +fails removes the working folder and any folder it made to hold the +destination. + ## Four packages, two per feed | | the window | the command line |