From b0aac68d91e9563e991dcfc2b68e1c79344001da Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 11:33:46 +0000 Subject: [PATCH] feat(rules): add three container privilege and isolation rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no-dangerous-cap-add reports a Linux capability that lets a process act on the host rather than on the container, in "capAdd" or in a "--cap-add" entry. Capabilities the kernel confines to the container's own namespaces are deliberately absent: "SYS_PTRACE" and "NET_ADMIN" sound privileged but reach no further than the container does, and "ALL" is what no-cap-add-all reports. no-host-namespace reports a "runArgs" entry that puts the container in one of the host's namespaces — "--network=host", "--pid=host", "--ipc=host", "--uts=host", "--userns=host", "--cgroupns=host". A network value is read as docker/cli reads it: a field list when some part of it is a bare "word=word" pair, in which case "name" holds the network, and a network name otherwise, so "--network=name = host" asks for a network of that name rather than for the host's. no-apparmor-unconfined reports AppArmor confinement turned off, the counterpart of no-seccomp-unconfined. The rules read a flag through the engine's "runArgs" traversal, so each declares the flags it wants and is handed their occurrences. no-host-namespace derives its paths from its own table, so the two cannot drift apart, and a test pins every key to a flag the table of "docker run" flags holds. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NnTsBGF2nTtG8AS1osuHvJ --- README.md | 2 +- dockerargs/network.go | 45 +++++++++ dockerargs/network_test.go | 42 ++++++++ dockerargs/securityopt.go | 4 + rules/hostnamespace_internal_test.go | 24 +++++ rules/no_apparmor_unconfined.go | 80 +++++++++++++++ rules/no_apparmor_unconfined_test.go | 83 ++++++++++++++++ rules/no_dangerous_cap_add.go | 112 +++++++++++++++++++++ rules/no_dangerous_cap_add_test.go | 141 +++++++++++++++++++++++++++ rules/no_host_namespace.go | 113 +++++++++++++++++++++ rules/no_host_namespace_test.go | 128 ++++++++++++++++++++++++ rules/rules.go | 3 + 12 files changed, 776 insertions(+), 1 deletion(-) create mode 100644 dockerargs/network.go create mode 100644 dockerargs/network_test.go create mode 100644 rules/hostnamespace_internal_test.go create mode 100644 rules/no_apparmor_unconfined.go create mode 100644 rules/no_apparmor_unconfined_test.go create mode 100644 rules/no_dangerous_cap_add.go create mode 100644 rules/no_dangerous_cap_add_test.go create mode 100644 rules/no_host_namespace.go create mode 100644 rules/no_host_namespace_test.go diff --git a/README.md b/README.md index e3a628f..bb5cdca 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ runs without configuration; the rest are `off` until you enable them: | Category | Default | Rules | | --- | --- | --- | | [`correctness`](https://bare-devcontainer.github.io/decolint/rules/#correctness) | `error` | 13 | -| [`security`](https://bare-devcontainer.github.io/decolint/rules/#security) | `off` | 8 | +| [`security`](https://bare-devcontainer.github.io/decolint/rules/#security) | `off` | 11 | | [`reproducibility`](https://bare-devcontainer.github.io/decolint/rules/#reproducibility) | `off` | 4 | | [`style`](https://bare-devcontainer.github.io/decolint/rules/#style) | `off` | 2 | diff --git a/dockerargs/network.go b/dockerargs/network.go new file mode 100644 index 0000000..565c331 --- /dev/null +++ b/dockerargs/network.go @@ -0,0 +1,45 @@ +package dockerargs + +import ( + "encoding/csv" + "regexp" + "strings" +) + +// NetworkHost is the network mode that puts the container in the host's network namespace. +const NetworkHost = "host" + +// networkFieldList matches a "--network" value docker/cli reads as a field list rather than as a +// network name. It is docker/cli's own regexp, applied unanchored as it applies it, so one unspaced +// "key=value" anywhere makes the whole value a list: "name = host" names a network, where +// "alias=web, name = host " is a list whose "name" field holds one. +var networkFieldList = regexp.MustCompile(`\w+=\w+(,\w+=\w+)*`) + +// NetworkTarget returns the network a "--network" or "--net" value names. Docker takes either the +// network itself ("host") or a comma-separated field list in which "name" holds it +// ("name=host,alias=web"), the fields being a CSV record as a mount entry's are. It lower-cases a +// field's key and value and trims the space around them. +// +// A value naming no network — a list without a "name" field, or one Docker rejects outright — +// yields "", and a list naming several yields the last, which is the one Docker is left holding. +func NetworkTarget(value string) string { + if !networkFieldList.MatchString(value) { + return value + } + fields, err := csv.NewReader(strings.NewReader(value)).Read() + if err != nil { + return "" + } + target := "" + for _, field := range fields { + key, name, ok := strings.Cut(field, "=") + if !ok { + // Docker rejects a field carrying no value, leaving the value naming no network. + return "" + } + if strings.ToLower(strings.TrimSpace(key)) == "name" { + target = strings.ToLower(strings.TrimSpace(name)) + } + } + return target +} diff --git a/dockerargs/network_test.go b/dockerargs/network_test.go new file mode 100644 index 0000000..6852008 --- /dev/null +++ b/dockerargs/network_test.go @@ -0,0 +1,42 @@ +package dockerargs + +import "testing" + +func TestNetworkTarget(t *testing.T) { + t.Parallel() + + tests := []struct { + value, want string + }{ + {`host`, "host"}, + {`devnet`, "devnet"}, + // Docker compares the network mode exactly, so a differently cased spelling is a network name. + {`HOST`, "HOST"}, + {`name=host`, "host"}, + {`NAME=HOST`, "host"}, + {`alias=web,name=host`, "host"}, + {`alias=web, name = host `, "host"}, + {`name=devnet,name=host`, "host"}, + {`name=host,name=devnet`, "devnet"}, + {`alias=host`, ""}, + {`name=host,web`, ""}, + // The field-list reading is chosen by an unspaced "key=value" appearing somewhere, so a value + // whose every "=" has space around it is a network name that happens to contain one. + {`name = host`, "name = host"}, + {`name =host`, "name =host"}, + // The fields are a CSV record, so a field may be quoted and a record the reader rejects names + // no network. + {`"name=host"`, "host"}, + {`"name=host",alias=web`, "host"}, + {` name=host `, "host"}, + {`name=host,"alias=web`, ""}, + } + for _, tt := range tests { + t.Run(tt.value, func(t *testing.T) { + t.Parallel() + if got := NetworkTarget(tt.value); got != tt.want { + t.Errorf("NetworkTarget(%q) = %q, want %q", tt.value, got, tt.want) + } + }) + } +} diff --git a/dockerargs/securityopt.go b/dockerargs/securityopt.go index 609b97b..1d3f92f 100644 --- a/dockerargs/securityopt.go +++ b/dockerargs/securityopt.go @@ -12,6 +12,10 @@ const ( SeccompProfileUnconfined = "unconfined" ) +// AppArmorProfileUnconfined removes the container's AppArmor profile, as +// [SeccompProfileUnconfined] removes its seccomp one. +const AppArmorProfileUnconfined = "unconfined" + // noNewPrivileges is the one security option that may be written without a value. const noNewPrivileges = "no-new-privileges" diff --git a/rules/hostnamespace_internal_test.go b/rules/hostnamespace_internal_test.go new file mode 100644 index 0000000..ae3d8f6 --- /dev/null +++ b/rules/hostnamespace_internal_test.go @@ -0,0 +1,24 @@ +package rules + +import ( + "testing" + + "github.com/bare-devcontainer/decolint/dockerargs" +) + +// TestHostNamespaces_KeysAreRunFlags guards the lookup in checkNoHostNamespace, which is by the +// canonical long name the engine hands a rule. A key naming no "docker run" flag would leave its +// entry unreachable, and the rule reading a zero hostNamespace for every occurrence. +func TestHostNamespaces_KeysAreRunFlags(t *testing.T) { + t.Parallel() + + known := map[string]bool{} + for _, f := range dockerargs.RunFlags { + known[f.Name] = true + } + for flag := range hostNamespaces { + if !known[flag] { + t.Errorf("hostNamespaces is keyed by %q, which names no \"docker run\" flag", flag) + } + } +} diff --git a/rules/no_apparmor_unconfined.go b/rules/no_apparmor_unconfined.go new file mode 100644 index 0000000..e3f1e83 --- /dev/null +++ b/rules/no_apparmor_unconfined.go @@ -0,0 +1,80 @@ +package rules + +import ( + "fmt" + + "github.com/bare-devcontainer/decolint/dockerargs" + "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" +) + +// NoApparmorUnconfined reports a devcontainer.json or devcontainer-feature.json that disables +// AppArmor confinement, either via the "securityOpt" property or, in a devcontainer.json, a +// "--security-opt apparmor=unconfined" entry in "runArgs". It is the AppArmor counterpart of +// [NoSeccompUnconfined]: both remove a mandatory confinement layer the runtime applies by default. +var NoApparmorUnconfined = &linter.Rule{ + ID: "no-apparmor-unconfined", + Description: `disallow disabling AppArmor confinement via a devcontainer.json's or Feature's "securityOpt" property, or a "--security-opt apparmor=unconfined" entry in a devcontainer.json's "runArgs"`, + LongDescription: `A container runtime applies its own AppArmor profile ("docker-default" for Docker) to every container on +a host that has AppArmor enabled, restricting what the container may do to the host's filesystem, +capabilities, and network. "apparmor=unconfined" removes that profile outright, so the only thing left +between a process in the container and the host is the discretionary access control the container's user +is already subject to. The setting is usually copied from instructions for running nested containers or a +debugger, both of which have narrower settings that work.`, + References: []string{ + `https://containers.dev/implementors/json_reference/#general-properties`, + `https://docs.docker.com/engine/security/apparmor/`, + }, + Category: linter.CategorySecurity, + FileTypes: []linter.FileType{linter.Devcontainer, linter.Feature}, + Paths: []string{"/securityOpt/*", "/runArgs/--security-opt"}, + Example: linter.Example{ + Bad: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "image": "mcr.microsoft.com/devcontainers/base:ubuntu", + "securityOpt": ["apparmor=unconfined"] +} +`}, + }, + }, + Good: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "image": "mcr.microsoft.com/devcontainers/base:ubuntu", + "capAdd": ["SYS_PTRACE"] +} +`}, + }, + }, + }, + Check: checkNoApparmorUnconfined, +} + +func checkNoApparmorUnconfined(_ *linter.Context, node *linter.Node) []linter.Finding { + if node.Arg != nil { + if !securityOptDisablesAppArmor(node.Arg.Value) { + return nil + } + return []linter.Finding{{ + Message: fmt.Sprintf(`"runArgs" contains "--security-opt %s", disabling the container's AppArmor confinement`, node.Arg.Value), + Offset: node.Value.StartOffset, + }} + } + + lit, ok := node.Value.Value.(hujson.Literal) + if !ok || lit.Kind() != '"' || !securityOptDisablesAppArmor(lit.String()) { + return nil + } + return []linter.Finding{{ + Message: fmt.Sprintf(`"securityOpt" contains %q, disabling the container's AppArmor confinement`, lit.String()), + Offset: node.Value.StartOffset, + }} +} + +// securityOptDisablesAppArmor reports whether s, a single "securityOpt" entry, removes the +// container's AppArmor profile. +func securityOptDisablesAppArmor(s string) bool { + opt, ok := dockerargs.ParseSecurityOpt(s) + return ok && opt.Key == "apparmor" && opt.Value == dockerargs.AppArmorProfileUnconfined +} diff --git a/rules/no_apparmor_unconfined_test.go b/rules/no_apparmor_unconfined_test.go new file mode 100644 index 0000000..fa8db20 --- /dev/null +++ b/rules/no_apparmor_unconfined_test.go @@ -0,0 +1,83 @@ +package rules_test + +import ( + "testing" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" +) + +func TestNoApparmorUnconfined(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want []linter.Issue + }{ + {"no securityOpt property", `{"name": "test"}`, nil}, + {"securityOpt without apparmor", `{"securityOpt": ["no-new-privileges"]}`, nil}, + {"securityOpt with a custom apparmor profile", `{"securityOpt": ["apparmor=my-profile"]}`, nil}, + {"securityOpt with apparmor=unconfined", `{"securityOpt": ["apparmor=unconfined"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 18, RuleID: "no-apparmor-unconfined", + Message: `"securityOpt" contains "apparmor=unconfined", disabling the container's AppArmor confinement`}, + }}, + {"securityOpt with apparmor:unconfined", `{"securityOpt": ["apparmor:unconfined"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 18, RuleID: "no-apparmor-unconfined", + Message: `"securityOpt" contains "apparmor:unconfined", disabling the container's AppArmor confinement`}, + }}, + {"seccomp=unconfined is a different confinement", `{"securityOpt": ["seccomp=unconfined"]}`, nil}, + {"no runArgs", `{"runArgs": ["--init"]}`, nil}, + {"runArgs without apparmor", `{"runArgs": ["--security-opt", "seccomp=unconfined"]}`, nil}, + {"runArgs with security-opt apparmor=unconfined", `{"runArgs": ["--security-opt=apparmor=unconfined"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-apparmor-unconfined", + Message: `"runArgs" contains "--security-opt apparmor=unconfined", disabling the container's AppArmor confinement`}, + }}, + {"runArgs with security-opt apparmor=unconfined two tokens", `{"runArgs": ["--security-opt", "apparmor=unconfined"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 32, RuleID: "no-apparmor-unconfined", + Message: `"runArgs" contains "--security-opt apparmor=unconfined", disabling the container's AppArmor confinement`}, + }}, + {"every offending entry is reported", `{"runArgs": ["--security-opt=apparmor=unconfined", "--security-opt=apparmor:unconfined"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-apparmor-unconfined", + Message: `"runArgs" contains "--security-opt apparmor=unconfined", disabling the container's AppArmor confinement`}, + {Path: "devcontainer.json", Line: 1, Col: 52, RuleID: "no-apparmor-unconfined", + Message: `"runArgs" contains "--security-opt apparmor:unconfined", disabling the container's AppArmor confinement`}, + }}, + {"both securityOpt and runArgs", `{"securityOpt": ["apparmor=unconfined"], "runArgs": ["--security-opt=apparmor=unconfined"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 18, RuleID: "no-apparmor-unconfined", + Message: `"securityOpt" contains "apparmor=unconfined", disabling the container's AppArmor confinement`}, + {Path: "devcontainer.json", Line: 1, Col: 54, RuleID: "no-apparmor-unconfined", + Message: `"runArgs" contains "--security-opt apparmor=unconfined", disabling the container's AppArmor confinement`}, + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assertIssues(t, rules.NoApparmorUnconfined, linter.SeverityWarn, tt.src, tt.want) + }) + } +} + +func TestNoApparmorUnconfined_Feature(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want []linter.Issue + }{ + {"no securityOpt property", `{"id": "test", "version": "1.0.0", "name": "test"}`, nil}, + {"securityOpt with apparmor=unconfined", `{"id": "test", "securityOpt": ["apparmor=unconfined"]}`, []linter.Issue{ + {Path: "devcontainer-feature.json", Line: 1, Col: 32, RuleID: "no-apparmor-unconfined", + Message: `"securityOpt" contains "apparmor=unconfined", disabling the container's AppArmor confinement`}, + }}, + // "runArgs" has no meaning in a Feature, so it's not flagged there. + {"runArgs with apparmor=unconfined is ignored", `{"id": "test", "runArgs": ["--security-opt=apparmor=unconfined"]}`, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assertIssuesAt(t, rules.NoApparmorUnconfined, linter.SeverityWarn, "devcontainer-feature.json", linter.Feature, tt.src, tt.want) + }) + } +} diff --git a/rules/no_dangerous_cap_add.go b/rules/no_dangerous_cap_add.go new file mode 100644 index 0000000..b9139f8 --- /dev/null +++ b/rules/no_dangerous_cap_add.go @@ -0,0 +1,112 @@ +package rules + +import ( + "fmt" + + "github.com/bare-devcontainer/decolint/dockerargs" + "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" +) + +// dangerousCapabilities maps each Linux capability that lets a process act on the host rather than +// on the container to what it allows. The keys are as [dockerargs.Capability] returns a name, so a +// capability written in any of its spellings is found here. None of them is in a container runtime's default set, so one +// appearing in "capAdd" was granted deliberately. +// +// The list is deliberately narrower than "every capability a container does not need": adding one +// means arguing that granting it reaches past the container, which for these means reaching a +// kernel subsystem that is not namespaced. A capability the kernel confines to the container's own +// namespaces belongs elsewhere however privileged it sounds — "SYS_PTRACE" (process namespace) and +// "NET_ADMIN" (network namespace) are the standing examples, and sharing the host's namespaces is +// what [NoHostNamespace] reports. +// +// "ALL" is not here: granting every capability at once is what [NoCapAddAll] reports. +var dangerousCapabilities = map[string]string{ + "CAP_AUDIT_CONTROL": "allows reconfiguring the kernel's audit subsystem, which is not namespaced", + "CAP_BPF": "allows loading BPF programs into the host kernel", + "CAP_DAC_READ_SEARCH": "bypasses file read permission checks and allows opening files by handle, outside the container's filesystem", + "CAP_MAC_ADMIN": "allows changing the host's mandatory access control policy", + "CAP_MAC_OVERRIDE": "bypasses the host's mandatory access control policy", + "CAP_PERFMON": "grants access to the kernel's performance monitoring interfaces, which observe the whole host", + "CAP_SYSLOG": "allows reading the host kernel's log, which discloses kernel addresses", + "CAP_SYS_ADMIN": "grants a broad range of administrative operations, including mounting filesystems", + "CAP_SYS_BOOT": "allows rebooting the host", + "CAP_SYS_MODULE": "allows loading modules into the host kernel", + "CAP_SYS_RAWIO": "allows raw access to the host's I/O ports and memory devices", + "CAP_SYS_TIME": "allows setting the clock, which the container shares with the host", +} + +// NoDangerousCapAdd reports a devcontainer.json or devcontainer-feature.json that grants a Linux +// capability which lets a process reach past the container, either via the "capAdd" property or, in +// a devcontainer.json, a "--cap-add" entry in "runArgs". Unlike [NoCapAddAll], which only flags +// granting every capability at once, this rule flags the individual capabilities in +// dangerousCapabilities. +var NoDangerousCapAdd = &linter.Rule{ + ID: "no-dangerous-cap-add", + Description: `disallow granting a Linux capability that lets a process act on the host, e.g. "SYS_ADMIN" or "SYS_MODULE", via the "capAdd" property or a "--cap-add" entry in a devcontainer.json's "runArgs"`, + LongDescription: `Container runtimes withhold the capabilities that let a process act on the host rather than on the +container, and "capAdd" adds them back one at a time. Each capability this rule reports is on its own +enough to reach past the container — loading a module into the host kernel, opening a file by handle +outside the mounted filesystem, rebooting the machine — and none of them is granted by default, so one +that appears here was asked for. Grant only what the workload actually fails without. + +A capability the kernel confines to the container's own namespaces is not reported, however privileged +it sounds: "SYS_PTRACE" reaches no further than the container's process namespace, and "NET_ADMIN" no +further than its network namespace. What makes those dangerous is sharing the host's namespaces, which +is a separate rule.`, + References: []string{ + `https://containers.dev/implementors/json_reference/#general-properties`, + `https://docs.docker.com/engine/security/#linux-kernel-capabilities`, + `https://man7.org/linux/man-pages/man7/capabilities.7.html`, + }, + Category: linter.CategorySecurity, + FileTypes: []linter.FileType{linter.Devcontainer, linter.Feature}, + Paths: []string{"/capAdd/*", "/runArgs/--cap-add"}, + Example: linter.Example{ + Bad: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "image": "mcr.microsoft.com/devcontainers/base:ubuntu", + "capAdd": ["SYS_ADMIN"] +} +`}, + }, + }, + Good: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "image": "mcr.microsoft.com/devcontainers/base:ubuntu", + "capAdd": ["SYS_PTRACE"] +} +`}, + }, + }, + }, + Check: checkNoDangerousCapAdd, +} + +func checkNoDangerousCapAdd(_ *linter.Context, node *linter.Node) []linter.Finding { + if node.Arg != nil { + effect, dangerous := dangerousCapabilities[dockerargs.Capability(node.Arg.Value)] + if !dangerous { + return nil + } + return []linter.Finding{{ + Message: fmt.Sprintf(`"runArgs" contains "--cap-add=%s", which %s`, node.Arg.Value, effect), + Offset: node.Value.StartOffset, + }} + } + + lit, ok := node.Value.Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + return nil + } + effect, dangerous := dangerousCapabilities[dockerargs.Capability(lit.String())] + if !dangerous { + return nil + } + return []linter.Finding{{ + Message: fmt.Sprintf(`"capAdd" contains %q, which %s`, lit.String(), effect), + Offset: node.Value.StartOffset, + }} +} diff --git a/rules/no_dangerous_cap_add_test.go b/rules/no_dangerous_cap_add_test.go new file mode 100644 index 0000000..dcf06c4 --- /dev/null +++ b/rules/no_dangerous_cap_add_test.go @@ -0,0 +1,141 @@ +package rules_test + +import ( + "testing" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" +) + +// TestNoDangerousCapAdd_EveryCapability pins the rule's inventory: every capability it reports, +// and the effect each one is reported with, which is user-facing text. The list is written out here +// rather than read from the rule, so that dropping an entry or rewording an effect fails rather +// than quietly reporting one capability fewer. TestNoDangerousCapAdd covers the forms a capability +// may be written in; this covers which capabilities there are. +func TestNoDangerousCapAdd_EveryCapability(t *testing.T) { + t.Parallel() + + tests := []struct { + capability, effect string + }{ + {"AUDIT_CONTROL", "allows reconfiguring the kernel's audit subsystem, which is not namespaced"}, + {"BPF", "allows loading BPF programs into the host kernel"}, + {"DAC_READ_SEARCH", "bypasses file read permission checks and allows opening files by handle, outside the container's filesystem"}, + {"MAC_ADMIN", "allows changing the host's mandatory access control policy"}, + {"MAC_OVERRIDE", "bypasses the host's mandatory access control policy"}, + {"PERFMON", "grants access to the kernel's performance monitoring interfaces, which observe the whole host"}, + {"SYSLOG", "allows reading the host kernel's log, which discloses kernel addresses"}, + {"SYS_ADMIN", "grants a broad range of administrative operations, including mounting filesystems"}, + {"SYS_BOOT", "allows rebooting the host"}, + {"SYS_MODULE", "allows loading modules into the host kernel"}, + {"SYS_RAWIO", "allows raw access to the host's I/O ports and memory devices"}, + {"SYS_TIME", "allows setting the clock, which the container shares with the host"}, + } + for _, tt := range tests { + t.Run(tt.capability, func(t *testing.T) { + t.Parallel() + src := `{"capAdd": ["` + tt.capability + `"]}` + assertIssues(t, rules.NoDangerousCapAdd, linter.SeverityWarn, src, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-dangerous-cap-add", + Message: `"capAdd" contains "` + tt.capability + `", which ` + tt.effect}, + }) + }) + } +} + +func TestNoDangerousCapAdd(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want []linter.Issue + }{ + {"no capAdd property", `{"name": "test"}`, nil}, + // SYS_PTRACE and NET_ADMIN are confined to the container's own process and network + // namespaces, so this rule leaves them to no-host-namespace. + {"capAdd with SYS_PTRACE is allowed", `{"capAdd": ["SYS_PTRACE"]}`, nil}, + {"capAdd with NET_ADMIN is allowed", `{"capAdd": ["NET_ADMIN"]}`, nil}, + {"capAdd with ALL is left to no-cap-add-all", `{"capAdd": ["ALL"]}`, nil}, + {"capAdd with SYS_ADMIN", `{"capAdd": ["SYS_ADMIN"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-dangerous-cap-add", + Message: `"capAdd" contains "SYS_ADMIN", which grants a broad range of administrative operations, including mounting filesystems`}, + }}, + {"capAdd with the CAP_ prefix in lower case", `{"capAdd": ["cap_sys_module"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-dangerous-cap-add", + Message: `"capAdd" contains "cap_sys_module", which allows loading modules into the host kernel`}, + }}, + {"every offending entry is reported", `{"capAdd": ["SYS_PTRACE", "SYS_RAWIO", "SYSLOG"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 27, RuleID: "no-dangerous-cap-add", + Message: `"capAdd" contains "SYS_RAWIO", which allows raw access to the host's I/O ports and memory devices`}, + {Path: "devcontainer.json", Line: 1, Col: 40, RuleID: "no-dangerous-cap-add", + Message: `"capAdd" contains "SYSLOG", which allows reading the host kernel's log, which discloses kernel addresses`}, + }}, + {"no runArgs", `{"runArgs": ["--init"]}`, nil}, + {"runArgs with cap-add=SYS_PTRACE", `{"runArgs": ["--cap-add=SYS_PTRACE"]}`, nil}, + {"runArgs with cap-drop is not cap-add", `{"runArgs": ["--cap-drop=SYS_ADMIN"]}`, nil}, + {"runArgs with cap-add=SYS_ADMIN", `{"runArgs": ["--cap-add=SYS_ADMIN"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-dangerous-cap-add", + Message: `"runArgs" contains "--cap-add=SYS_ADMIN", which grants a broad range of administrative operations, including mounting filesystems`}, + }}, + {"runArgs with cap-add DAC_READ_SEARCH two tokens", `{"runArgs": ["--cap-add", "DAC_READ_SEARCH"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 27, RuleID: "no-dangerous-cap-add", + Message: `"runArgs" contains "--cap-add=DAC_READ_SEARCH", which bypasses file read permission checks and allows opening files by handle, outside the container's filesystem`}, + }}, + {"capAdd entry is not a string", `{"capAdd": [123]}`, nil}, + {"runArgs is not an array", `{"runArgs": "--cap-add=SYS_ADMIN"}`, nil}, + // "-e" and "--name" take a value, so the entry after each is that value and names no flag + // (see dockerargs.Flags). In the second case "SYS_ADMIN" is left as the image name, so no + // capability is added there either. + {"runArgs with cap-add in another flag's value position", `{"runArgs": ["-e", "--cap-add=SYS_ADMIN"]}`, nil}, + {"runArgs with cap-add after another flag", `{"runArgs": ["--name", "--cap-add", "SYS_ADMIN"]}`, nil}, + // A bare entry would be the image name to "docker run", which parses no flag after it — but + // it also displaces the image the devcontainer CLI appends, along with the flags it derives + // from the configuration's own security properties. Reading on reports what is written + // rather than going quiet on a file that is broken. + {"runArgs with cap-add after the image name", `{"runArgs": ["myimage", "--cap-add=SYS_ADMIN"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 25, RuleID: "no-dangerous-cap-add", + Message: `"runArgs" contains "--cap-add=SYS_ADMIN", which grants a broad range of administrative operations, including mounting filesystems`}, + }}, + {"runArgs with cap-add after a terminator", `{"runArgs": ["--", "--cap-add=SYS_ADMIN"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 20, RuleID: "no-dangerous-cap-add", Message: `"runArgs" contains "--cap-add=SYS_ADMIN", which grants a broad range of administrative operations, including mounting filesystems`}, + }}, + {"both capAdd and runArgs", `{"capAdd": ["SYS_BOOT"], "runArgs": ["--cap-add=SYS_TIME"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-dangerous-cap-add", + Message: `"capAdd" contains "SYS_BOOT", which allows rebooting the host`}, + {Path: "devcontainer.json", Line: 1, Col: 38, RuleID: "no-dangerous-cap-add", + Message: `"runArgs" contains "--cap-add=SYS_TIME", which allows setting the clock, which the container shares with the host`}, + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assertIssues(t, rules.NoDangerousCapAdd, linter.SeverityWarn, tt.src, tt.want) + }) + } +} + +func TestNoDangerousCapAdd_Feature(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want []linter.Issue + }{ + {"no capAdd property", `{"id": "test", "version": "1.0.0", "name": "test"}`, nil}, + {"capAdd with SYS_PTRACE is allowed", `{"id": "test", "capAdd": ["SYS_PTRACE"]}`, nil}, + {"capAdd with SYS_MODULE", `{"id": "test", "capAdd": ["SYS_MODULE"]}`, []linter.Issue{ + {Path: "devcontainer-feature.json", Line: 1, Col: 27, RuleID: "no-dangerous-cap-add", + Message: `"capAdd" contains "SYS_MODULE", which allows loading modules into the host kernel`}, + }}, + // "runArgs" has no meaning in a Feature, so it's not flagged there. + {"runArgs with cap-add=SYS_ADMIN is ignored", `{"id": "test", "runArgs": ["--cap-add=SYS_ADMIN"]}`, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assertIssuesAt(t, rules.NoDangerousCapAdd, linter.SeverityWarn, "devcontainer-feature.json", linter.Feature, tt.src, tt.want) + }) + } +} diff --git a/rules/no_host_namespace.go b/rules/no_host_namespace.go new file mode 100644 index 0000000..1cc153c --- /dev/null +++ b/rules/no_host_namespace.go @@ -0,0 +1,113 @@ +package rules + +import ( + "fmt" + "slices" + + "github.com/bare-devcontainer/decolint/dockerargs" + "github.com/bare-devcontainer/decolint/linter" +) + +// hostNamespace describes one of the "runArgs" flags that put the container in one of the host's +// namespaces instead of its own. +type hostNamespace struct { + // namespace names what the flag shares with the host. + namespace string + // target reduces the flag's value to the namespace it names. It is nil for a flag whose value is + // the namespace itself, which every one but the network flags is. + target func(value string) string +} + +// hostNamespaces holds those flags, keyed by the canonical long name the engine hands a rule (see +// [dockerargs.Arg]). "--net" is a flag docker/cli registers in its own right rather than a shorthand +// of "--network", so it is its own entry. +// +// The CIS Docker Benchmark has a control per namespace for all of these but the cgroup namespace, +// which postdates the benchmark's controls rather than being held to be safe to share. +var hostNamespaces = map[string]hostNamespace{ + "network": {"network", dockerargs.NetworkTarget}, + "net": {"network", dockerargs.NetworkTarget}, + "pid": {"process", nil}, + "ipc": {"IPC", nil}, + "uts": {"UTS", nil}, + "userns": {"user", nil}, + "cgroupns": {"cgroup", nil}, +} + +// hostNamespacePaths are the rule's Paths: one per flag in [hostNamespaces], so that the two cannot +// drift apart. +var hostNamespacePaths = func() []string { + paths := make([]string, 0, len(hostNamespaces)) + for flag := range hostNamespaces { + paths = append(paths, "/runArgs/--"+flag) + } + slices.Sort(paths) + return paths +}() + +// NoHostNamespace reports a devcontainer.json whose "runArgs" put the container in one of the host's +// namespaces, e.g. "--network=host" or "--pid=host". Each such entry removes the isolation that +// namespace provides. +var NoHostNamespace = &linter.Rule{ + ID: "no-host-namespace", + Description: `disallow sharing one of the host's namespaces with the container via a "--network=host", "--pid=host", "--ipc=host", "--uts=host", "--userns=host", or "--cgroupns=host" entry in "runArgs"`, + LongDescription: `Namespaces are what make a container a container: the process table, network stack, and IPC objects it +sees are its own. A "host" value in "runArgs" hands one of them back, and the container stops being +isolated in that dimension. With "--pid=host" every process on the machine is visible from inside the +container, and a root process there can signal or trace it; with "--network=host" the container reaches +every service bound to the host's loopback interface, including the ones that are only reachable there +because they trust anything local. Put the container on a user-defined Docker network, or forward the +port you need, instead of joining the host's namespace.`, + References: []string{ + `https://containers.dev/implementors/json_reference/#image-specific`, + `https://github.com/docker/docker-bench-security`, + `https://man7.org/linux/man-pages/man7/namespaces.7.html`, + }, + Category: linter.CategorySecurity, + FileTypes: []linter.FileType{linter.Devcontainer}, + Paths: hostNamespacePaths, + Example: linter.Example{ + Bad: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "image": "mcr.microsoft.com/devcontainers/base:ubuntu", + "runArgs": ["--network=host"] +} +`}, + }, + }, + Good: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "image": "mcr.microsoft.com/devcontainers/base:ubuntu", + "runArgs": ["--network=devnet"] +} +`}, + }, + }, + Note: `The good example puts the container on a user-defined Docker network, so it can +reach other containers on it by name while keeping a network namespace of its own.`, + }, + Check: checkNoHostNamespace, +} + +func checkNoHostNamespace(_ *linter.Context, node *linter.Node) []linter.Finding { + ns := hostNamespaces[node.Arg.Flag] + target := node.Arg.Value + if ns.target != nil { + target = ns.target(target) + } + if target != dockerargs.NetworkHost { + return nil + } + // An entry a repeat of the same option supersedes is reported too. Which repeat is the live one + // is a property of the order alone — Docker keeps the first "--network" value and the last of + // every other one here — so an entry that is dead where it stands is live again the moment the + // order changes. The message therefore says what the entry asks for rather than what the + // container it is part of ends up with. + return []linter.Finding{{ + Message: fmt.Sprintf(`"runArgs" contains "--%s=%s", which asks for the host's %s namespace instead of the container's own`, + node.Arg.Flag, node.Arg.Value, ns.namespace), + Offset: node.Value.StartOffset, + }} +} diff --git a/rules/no_host_namespace_test.go b/rules/no_host_namespace_test.go new file mode 100644 index 0000000..a1de0a9 --- /dev/null +++ b/rules/no_host_namespace_test.go @@ -0,0 +1,128 @@ +package rules_test + +import ( + "testing" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" +) + +func TestNoHostNamespace(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want []linter.Issue + }{ + {"no runArgs property", `{"name": "test"}`, nil}, + {"runArgs without a namespace flag", `{"runArgs": ["--init"]}`, nil}, + {"network on a user-defined network", `{"runArgs": ["--network=devnet"]}`, nil}, + {"a host value for another flag", `{"runArgs": ["--add-host=host"]}`, nil}, + {"network=host", `{"runArgs": ["--network=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--network=host", which asks for the host's network namespace instead of the container's own`}, + }}, + {"net=host uses the alias", `{"runArgs": ["--net=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--net=host", which asks for the host's network namespace instead of the container's own`}, + }}, + {"pid host two tokens", `{"runArgs": ["--pid", "host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 23, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--pid=host", which asks for the host's process namespace instead of the container's own`}, + }}, + {"ipc=host", `{"runArgs": ["--ipc=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--ipc=host", which asks for the host's IPC namespace instead of the container's own`}, + }}, + {"uts=host", `{"runArgs": ["--uts=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--uts=host", which asks for the host's UTS namespace instead of the container's own`}, + }}, + {"userns=host", `{"runArgs": ["--userns=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--userns=host", which asks for the host's user namespace instead of the container's own`}, + }}, + {"cgroupns=host", `{"runArgs": ["--cgroupns=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--cgroupns=host", which asks for the host's cgroup namespace instead of the container's own`}, + }}, + {"ipc container reference is not the host", `{"runArgs": ["--ipc=container:other"]}`, nil}, + // Docker reads a "--network" value carrying a field value as a field list, in which "name" + // holds the network, and lower-cases each field. + {"network field list naming the host", `{"runArgs": ["--network=name=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--network=name=host", which asks for the host's network namespace instead of the container's own`}, + }}, + {"network field list is matched case-insensitively", `{"runArgs": ["--net=NAME=HOST"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--net=NAME=HOST", which asks for the host's network namespace instead of the container's own`}, + }}, + {"network field list with the host among other fields", `{"runArgs": ["--network=alias=web,name=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--network=alias=web,name=host", which asks for the host's network namespace instead of the container's own`}, + }}, + {"network field list naming another network", `{"runArgs": ["--network=name=devnet,alias=web"]}`, nil}, + // Docker assigns the network from every "name" field it reads, so a repeated one leaves the + // last. + {"network field list naming the host last", `{"runArgs": ["--network=name=devnet,name=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--network=name=devnet,name=host", which asks for the host's network namespace instead of the container's own`}, + }}, + {"network field list naming the host but not last", `{"runArgs": ["--network=name=host,name=devnet"]}`, nil}, + // Docker trims the space around a field's key and value, so where it falls does not matter. + {"network field list with space around the fields", `{"runArgs": ["--network=alias=web, name = host "]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--network=alias=web, name = host ", which asks for the host's network namespace instead of the container's own`}, + }}, + {"network field list with a field carrying no value", `{"runArgs": ["--network=name=host,web"]}`, nil}, + {"network field list naming no network", `{"runArgs": ["--network=alias=host"]}`, nil}, + // Only "--network" takes a field list; every other namespace flag names its value directly, + // so Docker reads this one as a network named "name=host" rather than as the host. + {"a field list is not accepted for another namespace", `{"runArgs": ["--pid=name=host"]}`, nil}, + {"a host value is matched case-sensitively", `{"runArgs": ["--pid=Host"]}`, nil}, + {"every offending entry is reported", `{"runArgs": ["--network=host", "--pid=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--network=host", which asks for the host's network namespace instead of the container's own`}, + {Path: "devcontainer.json", Line: 1, Col: 32, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--pid=host", which asks for the host's process namespace instead of the container's own`}, + }}, + // "runArgs" holds strings, so an entry that is not one is undefined rather than specified + // to end the parse; reading on keeps a flag behind it from going unreported. + {"non-string entry before the flag", `{"runArgs": [123, "--pid=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 19, RuleID: "no-host-namespace", Message: `"runArgs" contains "--pid=host", which asks for the host's process namespace instead of the container's own`}, + }}, + {"runArgs is not an array", `{"runArgs": "--pid=host"}`, nil}, + // A bare flag reads as a boolean set to true, which is not a namespace, so only a flag with + // a value can name one. + {"a flag with no value left to take", `{"runArgs": ["--pid"]}`, nil}, + {"a flag whose value is not a string", `{"runArgs": ["--pid", 123, "--pid=host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 28, RuleID: "no-host-namespace", Message: `"runArgs" contains "--pid=host", which asks for the host's process namespace instead of the container's own`}, + }}, + // Which repeat of one of these options Docker keeps differs per option: "--network" keeps + // the first value it is given and the rest keep the last. Either way the superseded entry is + // dead only where it stands, so it is reported too. + {"network repeated, the host first", `{"runArgs": ["--network", "host", "--net", "devnet"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 27, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--network=host", which asks for the host's network namespace instead of the container's own`}, + }}, + {"network repeated, the host second", `{"runArgs": ["--network", "devnet", "--net", "host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 46, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--net=host", which asks for the host's network namespace instead of the container's own`}, + }}, + {"pid repeated, the host last", `{"runArgs": ["--pid", "container:x", "--pid", "host"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 47, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--pid=host", which asks for the host's process namespace instead of the container's own`}, + }}, + {"pid repeated, the host first", `{"runArgs": ["--pid", "host", "--pid", "container:x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 23, RuleID: "no-host-namespace", + Message: `"runArgs" contains "--pid=host", which asks for the host's process namespace instead of the container's own`}, + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assertIssues(t, rules.NoHostNamespace, linter.SeverityWarn, tt.src, tt.want) + }) + } +} diff --git a/rules/rules.go b/rules/rules.go index 1af2dd8..f8e79d4 100644 --- a/rules/rules.go +++ b/rules/rules.go @@ -48,9 +48,12 @@ var builtinRuleList = []*linter.Rule{ MissingRequiredProps, MissingWorkspaceMountFolder, NoAppPort, + NoApparmorUnconfined, NoBindMount, NoCapAddAll, + NoDangerousCapAdd, NoDockerSocketMount, + NoHostNamespace, NoHostPortFormat, NoImageLatest, NoPrivilegedContainer,