From 1b502a37e0bd286b0b4ae5703a67f77208d39ffa Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 00:28:08 +0200 Subject: [PATCH] Let only the authority's root hold lines, refuse every line end, and keep places off the runtime's data and any .ssh The review of hq issue 339 found the PEM exception too wide (any module, any key, any label, anything base64), \v, \f, NEL and the Unicode separators still let a value end a line in some readers, and the spool, /opt, the container runtimes' data and an account's .ssh still placeable. Lines are now taken only in step-ca's root setting, as certificates encoding/pem decodes and x509 parses; every line end is refused; and those paths are the machine's own. The test certificate is a real one, made for the tests with its key thrown away. --- internal/catalogue/co_located_test.go | 11 +- internal/catalogue/placement.go | 18 +++- internal/catalogue/settings.go | 100 +++++++++---------- internal/catalogue/terminal_settings_test.go | 86 ++++++++++++---- 4 files changed, 137 insertions(+), 78 deletions(-) diff --git a/internal/catalogue/co_located_test.go b/internal/catalogue/co_located_test.go index bf098c16..3373936f 100644 --- a/internal/catalogue/co_located_test.go +++ b/internal/catalogue/co_located_test.go @@ -17,9 +17,16 @@ import ( // // 04-ISSUES/038 was the first of them (the port). These are the rest. -// A root certificate, in the shape a certificate authority serves one. +// A root certificate, as a certificate authority serves one: a real, self-signed one made for these tests (its key +// thrown away), because the one setting that may hold lines must parse as a certificate (novox/hq issue 339). const servedRoot = `-----BEGIN CERTIFICATE----- -MIIBeDCCAR2gAwIBAgIQfake000000000000000000000000 +MIIBPjCB8aADAgECAhRZG3p93hUUB5bz00uxhYo/nJnTQjAFBgMrZXAwFDESMBAG +A1UEAwwJdGVzdCByb290MCAXDTI2MTAwODIyMjE0NloYDzIxMjYwOTE0MjIyMTQ2 +WjAUMRIwEAYDVQQDDAl0ZXN0IHJvb3QwKjAFBgMrZXADIQA0pt/ld+W0MXwBhPfO +cuAt56kIW6Qcn+4vqWpuvHTiqaNTMFEwHQYDVR0OBBYEFIjgaLk4OVGIOeZQiqnN +p9vhciFjMB8GA1UdIwQYMBaAFIjgaLk4OVGIOeZQiqnNp9vhciFjMA8GA1UdEwEB +/wQFMAMBAf8wBQYDK2VwA0EAl9uWeSM2XAV8u0reyV3BLRxNVik+4FCRO1QKPs2k +IlB1rK9oAOYManH+VFuBMI/JJ31ajSti81q4E0CSw8r5DQ== -----END CERTIFICATE----- ` diff --git a/internal/catalogue/placement.go b/internal/catalogue/placement.go index 1f6a7e38..808151ea 100644 --- a/internal/catalogue/placement.go +++ b/internal/catalogue/placement.go @@ -54,18 +54,28 @@ var ownerShape = regexp.MustCompile(`^[0-9]+:[0-9]+$`) // before anything is kept or composed, and the node-engine refuses them again where it applies. // // systemTrees are refused at and below: the machine's system, the kernel's, the boot loader's, root's home, -// what lives only while the machine runs, and the node-engine's and the mesh's own state. systemRoots are +// what lives only while the machine runs, the spool (cron's tables are there), the container runtimes' data +// (every container's filesystem), /opt, and the node-engine's and the mesh's own state. Any home's .ssh is +// refused as well, wherever the home is. systemRoots are // refused at, and wherever a path holds one (an ancestor of /var/lib holds it): each is the parent of every // module's or every person's directories, and owning it is owning all of them. var ( systemTrees = []string{"/etc", "/usr", "/boot", "/root", "/run", "/var/run", "/var/lock", "/proc", "/sys", - "/dev", "/bin", "/sbin", "/lib", "/lib32", "/lib64", "/var/lib/mesh", "/var/lib/mesh-host"} - systemRoots = []string{"/", "/var", "/var/lib", "/var/cache", "/var/log", "/var/tmp", "/var/spool", "/home", - "/mnt", "/media", "/srv", "/opt", "/tmp", "/storage", "/data", "/services"} + "/dev", "/bin", "/sbin", "/lib", "/lib32", "/lib64", "/var/lib/mesh", "/var/lib/mesh-host", "/var/spool", + "/var/lib/docker", "/var/lib/containers", "/opt"} + systemRoots = []string{"/", "/var", "/var/lib", "/var/cache", "/var/log", "/var/tmp", "/home", + "/mnt", "/media", "/srv", "/tmp", "/storage", "/data", "/services"} ) // systemPath says why a clean absolute path is the machine's own and never a placement's or an access's, or "". func systemPath(path string) string { + // Any home's keys, wherever the home is: a .ssh directory is its account's, and the keys and the list of who + // may log in as it are in there. + for _, part := range strings.Split(path, "/") { + if part == ".ssh" { + return path + " is an account's .ssh, which holds its keys and who may log in as it" + } + } for _, tree := range systemTrees { if path == tree || strings.HasPrefix(path, tree+"/") { return path + " is in " + tree + ", the machine's own or the mesh's state" diff --git a/internal/catalogue/settings.go b/internal/catalogue/settings.go index f7042491..2fab2177 100644 --- a/internal/catalogue/settings.go +++ b/internal/catalogue/settings.go @@ -1,10 +1,14 @@ package catalogue import ( + "bytes" + "crypto/x509" "encoding/json" + "encoding/pem" "fmt" "regexp" "sort" + "strconv" "strings" ) @@ -209,18 +213,25 @@ func deepCopy(in map[string]any) map[string]any { return out } -// settingsHoldOneLine refuses a line break, a carriage return or a NUL in any string of any setting, for every +// settingsHoldOneLine refuses a line break, a carriage return, any other line end (lineEnds) or a NUL in any +// string of any setting, for every // module, at any depth, keys as well as values, in an object or a list (novox/hq issue 339). A value is // substituted into env and configuration files the node-engine writes as root — an app's env file, a logind // drop-in — and a line break there is a line of the caller's own: a directive, an assignment, a section. A NUL // ends a string early wherever C reads it. Judged where a layer is kept and again where it is composed, so a // layer that holds one, however it got into the store, is said with its key. What must hold lines is a file -// of the module's own, never a setting. +// of the module's own, never a setting — with one exception, the certificate authority's root as certificates +// (certificates), because that is how step-ca serves it. func settingsHoldOneLine(module string, layers []Layer) error { for _, layer := range layers { for _, key := range sortedKeysAny(layer.Values) { + if module == rootModule && key == rootSetting { + if text, ok := layer.Values[key].(string); ok && certificates(text) { + continue + } + } if at := lineBreakIn(layer.Values[key], key); at != "" { - return fmt.Errorf("%s: the setting %s in %q holds a line break, a carriage return or a NUL, which a "+ + return fmt.Errorf("%s: the setting %s in %q holds a line break, a carriage return, another line end or a NUL, which a "+ "file it is written into would read as a line of its own; a setting is one line (novox/hq "+ "issue 339)", module, at, layer.From) } @@ -229,66 +240,51 @@ func settingsHoldOneLine(module string, layers []Layer) error { return nil } -// pemBlocks says whether a value is PEM blocks and nothing else: the one value with lines a setting may hold, -// because a provider serves its certificate authority's root to its consumers as one (step-ca to the route -// proxy). Each block is a BEGIN line, base64 lines of exactly 64 characters but the last, and the END line of -// the same label; the last line is a whole number of base64 groups, padded at its end alone. So no line of it -// is a path, an option, a section or an assignment of a name a program reads — a line break with anything -// else around it is refused. -func pemBlocks(v string) bool { - lines := strings.Split(strings.TrimSuffix(v, "\n"), "\n") - if len(lines) < 3 { - return false - } - for i := 0; i < len(lines); { - label, ok := strings.CutPrefix(lines[i], "-----BEGIN ") - if !ok || !strings.HasSuffix(label, "-----") { - return false - } - label = strings.TrimSuffix(label, "-----") - if label == "" || strings.Trim(label, "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 ") != "" { - return false - } - i++ - body := 0 - for i < len(lines) && !strings.HasPrefix(lines[i], "-----END ") { - body++ - i++ - } - if body == 0 || i == len(lines) || lines[i] != "-----END "+label+"-----" { - return false - } - for j, line := range lines[i-body : i] { - if !base64Line(line, j == body-1) { - return false - } - } - i++ - } - return true -} +// lineEnds are every character a reader may take as the end of a line: \n and \r, vertical tab and form feed, +// NEL, and the Unicode line and paragraph separators; and NUL, which ends a string early wherever C reads it. +const lineEnds = "\n\r\v\f\x00\u0085\u2028\u2029" -// base64Line is one line of a PEM body: 64 characters of the base64 alphabet, or for the last line at most 64, -// a whole number of groups, with at most two '=' at its end. -func base64Line(line string, last bool) bool { - if line == "" || len(line) > 64 || (!last && len(line) != 64) || len(line)%4 != 0 { +// rootSetting is the one setting that may hold lines: the certificate authority's root, which step-ca serves to +// its consumers as the setting `root` and which they write into a bundle file (the co-located path, issue 038). +const ( + rootModule = "step-ca" + rootSetting = "root" +) + +// certificates says whether a value is one or more PEM CERTIFICATE blocks and nothing else: each decodes with +// encoding/pem, carries no headers, and parses with x509.ParseCertificate; nothing but a final line break +// surrounds them, and every line ends with \n alone. +func certificates(v string) bool { + if strings.ContainsAny(v, "\r\v\f\x00\u0085\u2028\u2029") { return false } - data := strings.TrimRight(line, "=") - if len(line)-len(data) > 2 || (!last && data != line) { - return false + rest := []byte(v) + found := 0 + for len(rest) > 0 { + if !bytes.HasPrefix(rest, []byte("-----BEGIN ")) { + return false + } + block, after := pem.Decode(rest) + if block == nil || block.Type != "CERTIFICATE" || len(block.Headers) > 0 { + return false + } + if _, err := x509.ParseCertificate(block.Bytes); err != nil { + return false + } + found++ + rest = after } - return strings.Trim(data, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/") == "" + return found > 0 } // lineBreakIn is where the first string under v holding \n, \r or NUL is, or "". func lineBreakIn(v any, at string) string { - if strings.ContainsAny(at, "\n\r\x00") { - return strings.NewReplacer("\n", `\n`, "\r", `\r`, "\x00", `\0`).Replace(at) + if strings.ContainsAny(at, lineEnds) { + return strconv.Quote(at) } switch t := v.(type) { case string: - if strings.ContainsAny(t, "\n\r\x00") && !pemBlocks(t) { + if strings.ContainsAny(t, lineEnds) { return at } case map[string]any: diff --git a/internal/catalogue/terminal_settings_test.go b/internal/catalogue/terminal_settings_test.go index 27a2d4ea..bc3831fe 100644 --- a/internal/catalogue/terminal_settings_test.go +++ b/internal/catalogue/terminal_settings_test.go @@ -55,29 +55,75 @@ func TestASettingHoldsOneLine(t *testing.T) { } } -// PEM blocks alone may hold lines: a provider serves its authority's root as a setting. Anything around them, or -// a line in them that is not base64, is refused. -func TestAPEMBlockIsTheOneSettingWithLines(t *testing.T) { - m := Manifest{Module: "route-proxy"} - full := strings.Repeat("MIIB", 16) - pem := "-----BEGIN CERTIFICATE-----\n" + full + "\n" + full + "\nAbCd+/==\n-----END CERTIFICATE-----\n" - for _, ok := range []string{pem, pem + pem, strings.TrimSuffix(pem, "\n"), - "-----BEGIN CERTIFICATE-----\nMIIBeDCCAR2gAwIBAgIQfake000000000000000000000000\n-----END CERTIFICATE-----\n"} { - if err := JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{"root": ok}}}, false); err != nil { - t.Errorf("a PEM block: %v", err) +// Lines are allowed in one setting only: step-ca's `root`, as certificates that encoding/pem decodes and +// x509.ParseCertificate parses (novox/hq issue 339). Any other module or key, another label, or a block that is +// not a certificate is refused like any other line break. +func TestOnlyTheAuthoritysRootMayHoldLines(t *testing.T) { + ca := Manifest{Module: "step-ca"} + judge := func(m Manifest, key, v string) error { + return JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{key: v}}}, false) + } + for _, ok := range []string{servedRoot, servedRoot + servedRoot, strings.TrimSuffix(servedRoot, "\n")} { + if err := judge(ca, "root", ok); err != nil { + t.Errorf("the authority's root: %v", err) } } - for _, bad := range []string{ - pem + "PATH=/tmp\n", - "PATH=\n" + pem, - "-----BEGIN CERTIFICATE-----\n" + full + "\nPATH=\n-----END CERTIFICATE-----\n", - "-----BEGIN CERTIFICATE-----\nshort\n" + full + "\n-----END CERTIFICATE-----\n", - "-----BEGIN CERTIFICATE-----\n" + full + "\n-----END KEY-----\n", - "-----BEGIN CERTIFICATE-----\n-----END CERTIFICATE-----\n", - "-----BEGIN CERTIFICATE-----\r\n" + full + "\r\n-----END CERTIFICATE-----\r\n", + body := strings.TrimSuffix(strings.TrimPrefix(servedRoot, "-----BEGIN CERTIFICATE-----\n"), "-----END CERTIFICATE-----\n") + for name, bad := range map[string]string{ + "a line after it": servedRoot + "PATH=/tmp\n", + "a line before it": "PATH=\n" + servedRoot, + "another label": "-----BEGIN PRIVATE KEY-----\n" + body + "-----END PRIVATE KEY-----\n", + "headers": "-----BEGIN CERTIFICATE-----\nProc-Type: 4,ENCRYPTED\n\n" + body + "-----END CERTIFICATE-----\n", + "base64 that is no cert": "-----BEGIN CERTIFICATE-----\nMIIBeDCCAR2gAwIBAgIQfake000000000000000000000000\n-----END CERTIFICATE-----\n", + "carriage returns": strings.ReplaceAll(servedRoot, "\n", "\r\n"), } { - if err := JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{"root": bad}}}, false); err == nil { - t.Errorf("taken: %q", bad) + if err := judge(ca, "root", bad); err == nil { + t.Errorf("%s was taken", name) + } + } + if err := judge(ca, "other", servedRoot); err == nil { + t.Error("a certificate in another key of the authority was taken") + } + if err := judge(Manifest{Module: "mailu"}, "root", servedRoot); err == nil { + t.Error("a certificate in another module's setting was taken") + } + if err := JudgeSettings(ca, []Layer{{From: "anchor", Values: map[string]any{"root": []any{servedRoot}}}}, false); err == nil { + t.Error("a certificate below the top of the setting was taken") + } +} + +// Every character a reader takes as the end of a line is refused, not only \n and \r: vertical tab, form feed, +// NEL and the Unicode line and paragraph separators (novox/hq issue 339). +func TestEveryLineEndIsRefused(t *testing.T) { + m := Manifest{Module: "mailu"} + for _, v := range []string{"a\vb", "a\fb", "a\u0085b", "a\u2028b", "a\u2029b"} { + if err := JudgeSettings(m, []Layer{{From: "home", Values: map[string]any{"v": v}}}, false); err == nil || + !strings.Contains(err.Error(), "line break") { + t.Errorf("%q: %v", v, err) + } + } +} + +// The review's additions: the spool, the container runtimes' data, /opt, and any home's .ssh. +func TestTheRuntimesDataAndAnyHomesSSHAreTheMachinesOwn(t *testing.T) { + m := Manifest{Module: "notes", Resources: []map[string]any{{"id": "data", "type": "directory"}}, + Accesses: []Access{{ID: "media"}}} + for _, path := range []string{"/var/spool", "/var/spool/cron", "/var/lib/docker", "/var/lib/docker/volumes", + "/var/lib/containers/storage", "/opt", "/opt/app", "/home/alice/.ssh", "/home/alice/.ssh/keys", + "/srv/backup/.ssh", "/root/.ssh"} { + layers := []Layer{{From: "laptop", Values: map[string]any{PlacesSetting: map[string]any{"data": path}}}} + if _, err := Places(m, layers); err == nil || !strings.Contains(err.Error(), "issue 339") { + t.Errorf("a place at %s: %v", path, err) + } + layers = []Layer{{From: "laptop", Values: map[string]any{AccessesSetting: map[string]any{"media": path}}}} + if _, err := AccessPlaces(m, layers); err == nil || !strings.Contains(err.Error(), "issue 339") { + t.Errorf("an access at %s: %v", path, err) + } + } + for _, path := range []string{"/var/lib/dockerish", "/home/alice/ssh", "/home/alice/.sshd-notes", "/storage/media"} { + layers := []Layer{{From: "laptop", Values: map[string]any{PlacesSetting: map[string]any{"data": path}}}} + if _, err := Places(m, layers); err != nil { + t.Errorf("a place at %s: %v", path, err) } } }