beenull/moby
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Fix daemon panic when starting container with invalid device cgroup rule

Browse source 
This fixes a panic when an invalid "device cgroup rule" is passed, resulting
in an "index out of range".

This bug was introduced in the original implementation in 1756af6faf,
but was not reproducible when using the CLI, because the same commit also added
client-side validation on the flag before making an API request. The following
example, uses an invalid rule (`c *:*  rwm` - two spaces before the permissions);

```console
$ docker run --rm --network=host --device-cgroup-rule='c *:*  rwm' busybox
invalid argument "c *:*  rwm" for "--device-cgroup-rule" flag: invalid device cgroup format 'c *:*  rwm'
```

Doing the same, but using the API results in a daemon panic when starting the container;

Create a container with an invalid device cgroup rule:

```console
curl -v \
  --unix-socket /var/run/docker.sock \
  "http://localhost/v1.41/containers/create?name=foobar" \
  -H "Content-Type: application/json" \
  -d '{"Image":"busybox:latest", "HostConfig":{"DeviceCgroupRules": ["c *:*  rwm"]}}'
```

Start the container:

```console
curl -v \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.41/containers/foobar/start"
```

Observe the daemon logs:

```
2021-01-22 12:53:03.313806 I | http: panic serving @: runtime error: index out of range [0] with length 0
goroutine 571 [running]:
net/http.(*conn).serve.func1(0xc000cb2d20)
	/usr/local/go/src/net/http/server.go:1795 +0x13b
panic(0x2f32380, 0xc000aebfc0)
	/usr/local/go/src/runtime/panic.go:679 +0x1b6
github.com/docker/docker/oci.AppendDevicePermissionsFromCgroupRules(0xc000175c00, 0x8, 0x8, 0xc0000bd380, 0x1, 0x4, 0x0, 0x0, 0xc0000e69c0, 0x0, ...)
	/go/src/github.com/docker/docker/oci/oci.go:34 +0x64f
```

This patch:

- fixes the panic, allowing the daemon to return an error on container start
- adds a unit-test to validate various permutations
- adds a "todo" to verify the regular expression (and handling) of the "a" (all) value

We should also consider performing this validation when _creating_ the container,
so that an error is produced early.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This commit is contained in:
Sebastiaan van Stijn 2021-01-22 14:10:22 +01:00
parent b865beba22
commit 5cc1753f2c
No known key found for this signature in database
GPG key ID: 76698F39D527CE8C
2 changed files with 174 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
oci/oci.go
View file

@ -8,6 +8,11 @@ import (
specs "github.com/opencontainers/runtime-spec/specs-go"
)
// TODO verify if this regex is correct for "a" (all); the docs (https://github.com/torvalds/linux/blob/v5.10/Documentation/admin-guide/cgroup-v1/devices.rst) describe:
// "'all' means it applies to all types and all major and minor numbers", and shows an example
// that *only* passes `a` as value: `echo a > /sys/fs/cgroup/1/devices.allow, which would be
// the "implicit" equivalent of "a *:* rwm". Source-code also looks to confirm this, and returns
// early for "a" (all); https://github.com/torvalds/linux/blob/v5.10/security/device_cgroup.c#L614-L642
// nolint: gosimple
var deviceCgroupRuleRegex = regexp.MustCompile("^([acb]) ([0-9]+|\\*):([0-9]+|\\*) ([rwm]{1,3})$")
@ -31,7 +36,7 @@ func SetCapabilities(s *specs.Spec, caplist []string) error {
func AppendDevicePermissionsFromCgroupRules(devPermissions []specs.LinuxDeviceCgroup, rules []string) ([]specs.LinuxDeviceCgroup, error) {
for _, deviceCgroupRule := range rules {
ss := deviceCgroupRuleRegex.FindAllStringSubmatch(deviceCgroupRule, -1)
if len(ss[0]) != 5 {
if len(ss) == 0 || len(ss[0]) != 5 {
return nil, fmt.Errorf("invalid device cgroup rule format: '%s'", deviceCgroupRule)
}
matches := ss[0]

168
oci/oci_test.go Normal file
View file

@ -0,0 +1,168 @@
package oci
import (
"testing"
"github.com/opencontainers/runtime-spec/specs-go"
"gotest.tools/v3/assert"
)
func TestAppendDevicePermissionsFromCgroupRules(t *testing.T) {
ptr := func(i int64) *int64 { return &i }
tests := []struct {
doc string
rule string
expected specs.LinuxDeviceCgroup
expectedErr string
}{
{
doc: "empty rule",
rule: "",
expectedErr: `invalid device cgroup rule format: ''`,
},
{
doc: "multiple spaces after first column",
rule: "c 1:1 rwm",
expectedErr: `invalid device cgroup rule format: 'c 1:1 rwm'`,
},
{
doc: "multiple spaces after second column",
rule: "c 1:1 rwm",
expectedErr: `invalid device cgroup rule format: 'c 1:1 rwm'`,
},
{
doc: "leading spaces",
rule: " c 1:1 rwm",
expectedErr: `invalid device cgroup rule format: ' c 1:1 rwm'`,
},
{
doc: "trailing spaces",
rule: "c 1:1 rwm ",
expectedErr: `invalid device cgroup rule format: 'c 1:1 rwm '`,
},
{
doc: "unknown device type",
rule: "z 1:1 rwm",
expectedErr: `invalid device cgroup rule format: 'z 1:1 rwm'`,
},
{
doc: "invalid device type",
rule: "zz 1:1 rwm",
expectedErr: `invalid device cgroup rule format: 'zz 1:1 rwm'`,
},
{
doc: "missing colon",
rule: "c 11 rwm",
expectedErr: `invalid device cgroup rule format: 'c 11 rwm'`,
},
{
doc: "invalid device major-minor",
rule: "c a:a rwm",
expectedErr: `invalid device cgroup rule format: 'c a:a rwm'`,
},
{
doc: "negative major device",
rule: "c -1:1 rwm",
expectedErr: `invalid device cgroup rule format: 'c -1:1 rwm'`,
},
{
doc: "negative minor device",
rule: "c 1:-1 rwm",
expectedErr: `invalid device cgroup rule format: 'c 1:-1 rwm'`,
},
{
doc: "missing permissions",
rule: "c 1:1",
expectedErr: `invalid device cgroup rule format: 'c 1:1'`,
},
{
doc: "invalid permissions",
rule: "c 1:1 x",
expectedErr: `invalid device cgroup rule format: 'c 1:1 x'`,
},
{
doc: "too many permissions",
rule: "c 1:1 rwmrwm",
expectedErr: `invalid device cgroup rule format: 'c 1:1 rwmrwm'`,
},
{
doc: "major out of range",
rule: "c 18446744073709551616:1 rwm",
expectedErr: `invalid major value in device cgroup rule format: 'c 18446744073709551616:1 rwm'`,
},
{
doc: "minor out of range",
rule: "c 1:18446744073709551616 rwm",
expectedErr: `invalid minor value in device cgroup rule format: 'c 1:18446744073709551616 rwm'`,
},
{
doc: "all (a) devices",
rule: "a 1:1 rwm",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "a", Major: ptr(1), Minor: ptr(1), Access: "rwm"},
},
{
doc: "char (c) devices",
rule: "c 1:1 rwm",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(1), Minor: ptr(1), Access: "rwm"},
},
{
doc: "block (b) devices",
rule: "b 1:1 rwm",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "b", Major: ptr(1), Minor: ptr(1), Access: "rwm"},
},
{
doc: "char device with rwm permissions",
rule: "c 7:128 rwm",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(7), Minor: ptr(128), Access: "rwm"},
},
{
doc: "wildcard major",
rule: "c *:1 rwm",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(-1), Minor: ptr(1), Access: "rwm"},
},
{
doc: "wildcard minor",
rule: "c 1:* rwm",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(1), Minor: ptr(-1), Access: "rwm"},
},
{
doc: "wildcard major and minor",
rule: "c *:* rwm",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(-1), Minor: ptr(-1), Access: "rwm"},
},
{
doc: "read (r) permission",
rule: "c 1:1 r",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(1), Minor: ptr(1), Access: "r"},
},
{
doc: "write (w) permission",
rule: "c 1:1 w",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(1), Minor: ptr(1), Access: "w"},
},
{
doc: "mknod (m) permission",
rule: "c 1:1 m",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(1), Minor: ptr(1), Access: "m"},
},
{
doc: "mknod (m) and read (r) permission",
rule: "c 1:1 mr",
expected: specs.LinuxDeviceCgroup{Allow: true, Type: "c", Major: ptr(1), Minor: ptr(1), Access: "mr"},
},
}
for _, tc := range tests {
tc := tc
t.Run(tc.doc, func(t *testing.T) {
out, err := AppendDevicePermissionsFromCgroupRules([]specs.LinuxDeviceCgroup{}, []string{tc.rule})
if tc.expectedErr != "" {
assert.Error(t, err, tc.expectedErr)
return
}
assert.NilError(t, err)
assert.DeepEqual(t, out, []specs.LinuxDeviceCgroup{tc.expected})
})
}
}