diff --git a/runsc/cmd/exec.go b/runsc/cmd/exec.go index e575dc163..a4dada10e 100644 --- a/runsc/cmd/exec.go +++ b/runsc/cmd/exec.go @@ -114,39 +114,26 @@ func (ex *Exec) SetFlags(f *flag.FlagSet) { // already created container. func (ex *Exec) Execute(_ context.Context, f *flag.FlagSet, args ...any) subcommands.ExitStatus { conf := args[0].(*config.Config) - e, id, err := ex.parseArgs(f, conf.EnableRaw) - if err != nil { - util.Fatalf("parsing process spec: %v", err) - } waitStatus := args[1].(*unix.WaitStatus) + if f.NArg() < 1 { + f.Usage() + util.Fatalf("a container-id is required") + } + id := f.Arg(0) c, err := container.Load(conf.RootDir, container.FullID{ContainerID: id}, container.LoadOpts{}) if err != nil { util.Fatalf("loading sandbox: %v", err) } + e, err := ex.parseArgs(f, c.Spec.Process, conf.EnableRaw) + if err != nil { + util.Fatalf("parsing process spec: %v", err) + } + log.Debugf("Exec arguments: %+v", e) log.Debugf("Exec capabilities: %+v", e.Capabilities) - // Replace empty settings with defaults from container. - if e.WorkingDirectory == "" { - e.WorkingDirectory = c.Spec.Process.Cwd - } - if e.Envv == nil { - e.Envv, err = specutils.ResolveEnvs(c.Spec.Process.Env, ex.env) - if err != nil { - util.Fatalf("getting environment variables: %v", err) - } - } - - if e.Capabilities == nil { - e.Capabilities, err = specutils.Capabilities(conf.EnableRaw, c.Spec.Process.Capabilities) - if err != nil { - util.Fatalf("creating capabilities: %v", err) - } - log.Infof("Using exec capabilities from container: %+v", e.Capabilities) - } - // Create the file descriptor map for the process in the container. fdMap := map[int]*os.File{ 0: os.Stdin, @@ -326,29 +313,30 @@ func (ex *Exec) execChildAndWait(waitStatus *unix.WaitStatus) subcommands.ExitSt } // parseArgs parses exec information from the command line or a JSON file -// depending on whether the --process flag was used. Returns an ExecArgs and -// the ID of the container to be used. -func (ex *Exec) parseArgs(f *flag.FlagSet, enableRaw bool) (*control.ExecArgs, string, error) { +// depending on whether the --process flag was used. +func (ex *Exec) parseArgs(f *flag.FlagSet, p *specs.Process, enableRaw bool) (*control.ExecArgs, error) { if ex.processPath == "" { // Requires at least a container ID and command. if f.NArg() < 2 { f.Usage() - return nil, "", fmt.Errorf("both a container-id and command are required") + return nil, fmt.Errorf("both a container-id and command are required") } - e, err := ex.argsFromCLI(f.Args()[1:], enableRaw) - return e, f.Arg(0), err + return ex.argsFromCLI(p, f.Args()[1:], enableRaw) } // Requires only the container ID. if f.NArg() != 1 { f.Usage() - return nil, "", fmt.Errorf("a container-id is required") + return nil, fmt.Errorf("only the container-id is required") } - e, err := ex.argsFromProcessFile(enableRaw) - return e, f.Arg(0), err + e, err := ex.argsFromProcessFile(p, enableRaw) + return e, err } -func (ex *Exec) argsFromCLI(argv []string, enableRaw bool) (*control.ExecArgs, error) { - extraKGIDs := make([]auth.KGID, 0, len(ex.extraKGIDs)) +func (ex *Exec) argsFromCLI(p *specs.Process, argv []string, enableRaw bool) (*control.ExecArgs, error) { + extraKGIDs := make([]auth.KGID, 0, len(p.User.AdditionalGids)+len(ex.extraKGIDs)) + for _, kgid := range p.User.AdditionalGids { + extraKGIDs = append(extraKGIDs, auth.KGID(kgid)) + } for _, s := range ex.extraKGIDs { kgid, err := strconv.Atoi(s) if err != nil { @@ -357,20 +345,34 @@ func (ex *Exec) argsFromCLI(argv []string, enableRaw bool) (*control.ExecArgs, e extraKGIDs = append(extraKGIDs, auth.KGID(kgid)) } - var caps *auth.TaskCapabilities - if len(ex.caps) > 0 { - var err error - caps, err = capabilities(ex.caps, enableRaw) - if err != nil { - return nil, fmt.Errorf("capabilities error: %v", err) - } + caps, err := capabilities(p, ex.caps, enableRaw) + if err != nil { + return nil, fmt.Errorf("capabilities error: %v", err) + } + + cwd := p.Cwd + if ex.cwd != "" { + cwd = ex.cwd + } + + envv := append(p.Env, ex.env...) + + kuid := auth.KUID(p.User.UID) + if ex.user.kuidSet { + kuid = ex.user.kuid + } + + kgid := auth.KGID(p.User.GID) + if ex.user.kgidSet { + kgid = ex.user.kgid } return &control.ExecArgs{ Argv: argv, - WorkingDirectory: ex.cwd, - KUID: ex.user.kuid, - KGID: ex.user.kgid, + Envv: envv, + WorkingDirectory: cwd, + KUID: kuid, + KGID: kgid, ExtraKGIDs: extraKGIDs, Capabilities: caps, StdioIsPty: ex.consoleSocket != "" || console.IsPty(os.Stdin.Fd()), @@ -382,7 +384,7 @@ func (ex *Exec) argsFromCLI(argv []string, enableRaw bool) (*control.ExecArgs, e }, nil } -func (ex *Exec) argsFromProcessFile(enableRaw bool) (*control.ExecArgs, error) { +func (ex *Exec) argsFromProcessFile(specProc *specs.Process, enableRaw bool) (*control.ExecArgs, error) { f, err := os.Open(ex.processPath) if err != nil { return nil, fmt.Errorf("error opening process file: %s, %v", ex.processPath, err) @@ -392,24 +394,42 @@ func (ex *Exec) argsFromProcessFile(enableRaw bool) (*control.ExecArgs, error) { if err := json.NewDecoder(f).Decode(&p); err != nil { return nil, fmt.Errorf("error parsing process file: %s, %v", ex.processPath, err) } - return argsFromProcess(&p, enableRaw) + if validateProcessSpec(&p) != nil { + return nil, fmt.Errorf("invalid process spec: %w", err) + } + return argsFromProcess(specProc, &p, enableRaw) +} + +func validateProcessSpec(p *specs.Process) error { + if p.Cwd == "" { + return fmt.Errorf("cwd must not be empty") + } + if !filepath.IsAbs(p.Cwd) { + return fmt.Errorf("cwd %q must be an absolute path", p.Cwd) + } + if len(p.Args) == 0 { + return fmt.Errorf("args must not be empty") + } + return nil } // argsFromProcess performs all the non-IO conversion from the Process struct // to ExecArgs. -func argsFromProcess(p *specs.Process, enableRaw bool) (*control.ExecArgs, error) { +func argsFromProcess(specProc *specs.Process, p *specs.Process, enableRaw bool) (*control.ExecArgs, error) { // Create capabilities. - var caps *auth.TaskCapabilities - if p.Capabilities != nil { - var err error - // Starting from Docker 19, capabilities are explicitly set for exec (instead - // of nil like before). So we can't distinguish 'exec' from - // 'exec --privileged', as both specify CAP_NET_RAW. Therefore, filter - // CAP_NET_RAW in the same way as container start. - caps, err = specutils.Capabilities(enableRaw, p.Capabilities) - if err != nil { - return nil, fmt.Errorf("error creating capabilities: %v", err) - } + procCaps := p.Capabilities + if procCaps == nil { + // If p doesn't have capabilities specified, fallback to the capabilities + // specified in the container spec. + procCaps = specProc.Capabilities + } + // Starting from Docker 19, capabilities are explicitly set for exec (instead + // of nil like before). So we can't distinguish 'exec' from + // 'exec --privileged', as both specify CAP_NET_RAW. Therefore, filter + // CAP_NET_RAW in the same way as container start. + caps, err := specutils.Capabilities(enableRaw, procCaps) + if err != nil { + return nil, fmt.Errorf("error creating capabilities: %v", err) } // Convert the spec's additional GIDs to KGIDs. @@ -438,14 +458,17 @@ func argsFromProcess(p *specs.Process, enableRaw bool) (*control.ExecArgs, error // capabilities takes a list of capabilities as strings and returns an // auth.TaskCapabilities struct with those capabilities in every capability set. // This mimics runc's behavior. -func capabilities(cs []string, enableRaw bool) (*auth.TaskCapabilities, error) { - var specCaps specs.LinuxCapabilities +func capabilities(p *specs.Process, cs []string, enableRaw bool) (*auth.TaskCapabilities, error) { + specCaps := *p.Capabilities for _, cap := range cs { - specCaps.Ambient = append(specCaps.Ambient, cap) specCaps.Bounding = append(specCaps.Bounding, cap) specCaps.Effective = append(specCaps.Effective, cap) - specCaps.Inheritable = append(specCaps.Inheritable, cap) specCaps.Permitted = append(specCaps.Permitted, cap) + // Consistent with runc, don't set inheritable. Only set ambient if we + // already have some inheritable bits set from spec. + if specCaps.Inheritable != nil { + specCaps.Ambient = append(specCaps.Ambient, cap) + } } // Starting from Docker 19, capabilities are explicitly set for exec (instead // of nil like before). So we can't distinguish 'exec' from @@ -479,8 +502,10 @@ func (ss *stringSlice) Set(s string) error { // user allows -user to convey a UID and, optionally, a GID separated by a // colon. type user struct { - kuid auth.KUID - kgid auth.KGID + kuid auth.KUID + kuidSet bool + kgid auth.KGID + kgidSet bool } // String implements flag.Value.String. @@ -501,12 +526,14 @@ func (u *user) Set(s string) error { return fmt.Errorf("couldn't parse UID: %s", parts[0]) } u.kuid = auth.KUID(kuid) + u.kuidSet = true if len(parts) > 1 { kgid, err := strconv.Atoi(parts[1]) if err != nil { return fmt.Errorf("couldn't parse GID: %s", parts[1]) } u.kgid = auth.KGID(kgid) + u.kgidSet = true } return nil } diff --git a/runsc/cmd/exec_test.go b/runsc/cmd/exec_test.go index 20c804d97..d0361920e 100644 --- a/runsc/cmd/exec_test.go +++ b/runsc/cmd/exec_test.go @@ -32,10 +32,10 @@ func TestUser(t *testing.T) { want user wantErr bool }{ - {input: "0", want: user{kuid: 0, kgid: 0}}, - {input: "7", want: user{kuid: 7, kgid: 0}}, - {input: "49:343", want: user{kuid: 49, kgid: 343}}, - {input: "0:2401", want: user{kuid: 0, kgid: 2401}}, + {input: "0", want: user{kuid: 0, kuidSet: true, kgid: 0, kgidSet: false}}, + {input: "7", want: user{kuid: 7, kuidSet: true, kgid: 0, kgidSet: false}}, + {input: "49:343", want: user{kuid: 49, kuidSet: true, kgid: 343, kgidSet: true}}, + {input: "0:2401", want: user{kuid: 0, kuidSet: true, kgid: 2401, kgidSet: true}}, {input: "", wantErr: true}, {input: "foo", wantErr: true}, {input: ":123", wantErr: true}, @@ -59,58 +59,105 @@ func TestUser(t *testing.T) { func TestCLIArgs(t *testing.T) { testCases := []struct { + name string ex Exec + spec specs.Process argv []string expected control.ExecArgs }{ { - ex: Exec{ - cwd: "/foo/bar", - user: user{kuid: 0, kgid: 0}, - extraKGIDs: []string{"1", "2", "3"}, - caps: []string{"CAP_DAC_OVERRIDE"}, - processPath: "", + name: "spec used by default", + ex: Exec{}, + spec: specs.Process{ + User: specs.User{UID: 2, GID: 2, AdditionalGids: []uint32{1, 2, 3}}, + Capabilities: &specs.LinuxCapabilities{Bounding: []string{"CAP_DAC_OVERRIDE"}, Inheritable: []string{"CAP_DAC_OVERRIDE"}}, + Cwd: "/foo/bar", + Env: []string{"FOO=bar"}, }, argv: []string{"ls", "/"}, expected: control.ExecArgs{ Argv: []string{"ls", "/"}, + Envv: []string{"FOO=bar"}, WorkingDirectory: "/foo/bar", FilePayload: control.NewFilePayload(map[int]*os.File{ 0: os.Stdin, 1: os.Stdout, 2: os.Stderr, }, nil), - KUID: 0, - KGID: 0, + KUID: 2, + KGID: 2, ExtraKGIDs: []auth.KGID{1, 2, 3}, Capabilities: &auth.TaskCapabilities{ BoundingCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), - EffectiveCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), InheritableCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), - PermittedCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), + }, + }, + }, + { + name: "spec overridden by CLI", + ex: Exec{ + cwd: "/baz", + user: user{kuid: 4, kuidSet: true, kgid: 4, kgidSet: true}, + extraKGIDs: []string{"4", "5", "6"}, + caps: []string{"CAP_DAC_READ_SEARCH"}, + env: []string{"BAZ=new"}, + }, + spec: specs.Process{ + User: specs.User{UID: 2, GID: 2, AdditionalGids: []uint32{1, 2, 3}}, + Capabilities: &specs.LinuxCapabilities{Bounding: []string{"CAP_DAC_OVERRIDE"}, Inheritable: []string{"CAP_DAC_OVERRIDE"}}, + Cwd: "/foo/bar", + Env: []string{"FOO=bar"}, + }, + argv: []string{"ls", "/"}, + expected: control.ExecArgs{ + Argv: []string{"ls", "/"}, + Envv: []string{"FOO=bar", "BAZ=new"}, + WorkingDirectory: "/baz", + FilePayload: control.NewFilePayload(map[int]*os.File{ + 0: os.Stdin, + 1: os.Stdout, + 2: os.Stderr, + }, nil), + KUID: 4, + KGID: 4, + ExtraKGIDs: []auth.KGID{1, 2, 3, 4, 5, 6}, + Capabilities: &auth.TaskCapabilities{ + BoundingCaps: auth.CapabilitySetOfMany([]linux.Capability{linux.CAP_DAC_OVERRIDE, linux.CAP_DAC_READ_SEARCH}), + EffectiveCaps: auth.CapabilitySetOfMany([]linux.Capability{linux.CAP_DAC_READ_SEARCH}), + PermittedCaps: auth.CapabilitySetOfMany([]linux.Capability{linux.CAP_DAC_READ_SEARCH}), + InheritableCaps: auth.CapabilitySetOfMany([]linux.Capability{linux.CAP_DAC_OVERRIDE}), + // TODO(gvisor.dev/issue/3166): Once ambient capabilities is + // supported, AmbientCaps should be CAP_DAC_READ_SEARCH. + AmbientCaps: 0, }, }, }, } for _, tc := range testCases { - e, err := tc.ex.argsFromCLI(tc.argv, true) - if err != nil { - t.Errorf("argsFromCLI(%+v): got error: %+v", tc.ex, err) - } else if !cmp.Equal(*e, tc.expected, cmpopts.IgnoreUnexported(os.File{})) { - t.Errorf("argsFromCLI(%+v): got %+v, but expected %+v", tc.ex, *e, tc.expected) - } + t.Run(tc.name, func(t *testing.T) { + e, err := tc.ex.argsFromCLI(&tc.spec, tc.argv, true) + if err != nil { + t.Errorf("argsFromCLI(%+v): got error: %+v", tc.ex, err) + return + } + if diff := cmp.Diff(*e, tc.expected, cmpopts.IgnoreUnexported(os.File{})); diff != "" { + t.Errorf("argsFromCLI(%+v): diff (+want -got):\n%s", tc.ex, diff) + } + }) } } func TestJSONArgs(t *testing.T) { testCases := []struct { - // ex is provided to make sure it is overridden by p. + name string ex Exec + spec specs.Process p specs.Process expected control.ExecArgs }{ { + name: "flags overridden by process file", ex: Exec{ cwd: "/baz/quux", user: user{kuid: 1, kgid: 1}, @@ -118,6 +165,9 @@ func TestJSONArgs(t *testing.T) { caps: []string{"CAP_SETGID"}, processPath: "/bin/foo", }, + spec: specs.Process{ + Capabilities: &specs.LinuxCapabilities{Bounding: []string{"CAP_DAC_READ_SEARCH"}}, + }, p: specs.Process{ User: specs.User{UID: 0, GID: 0, AdditionalGids: []uint32{1, 2, 3}}, Args: []string{"ls", "/"}, @@ -148,14 +198,47 @@ func TestJSONArgs(t *testing.T) { }, }, }, + { + name: "capabilities fallback to spec", + ex: Exec{}, + spec: specs.Process{ + Capabilities: &specs.LinuxCapabilities{ + Bounding: []string{"CAP_DAC_READ_SEARCH"}}, + }, + p: specs.Process{ + User: specs.User{UID: 0, GID: 0}, + Args: []string{"ls", "/"}, + Cwd: "/foo/bar", + // Does not specify capabilities. + }, + expected: control.ExecArgs{ + Argv: []string{"ls", "/"}, + WorkingDirectory: "/foo/bar", + FilePayload: control.NewFilePayload(map[int]*os.File{ + 0: os.Stdin, + 1: os.Stdout, + 2: os.Stderr, + }, nil), + KUID: 0, + KGID: 0, + ExtraKGIDs: []auth.KGID{}, + Capabilities: &auth.TaskCapabilities{ + BoundingCaps: auth.CapabilitySetOf(linux.CAP_DAC_READ_SEARCH), + }, + }, + }, } for _, tc := range testCases { - e, err := argsFromProcess(&tc.p, true) - if err != nil { - t.Errorf("argsFromProcess(%+v): got error: %+v", tc.p, err) - } else if !cmp.Equal(*e, tc.expected, cmpopts.IgnoreUnexported(os.File{})) { - t.Errorf("argsFromProcess(%+v): got %+v, but expected %+v", tc.p, *e, tc.expected) - } + t.Run(tc.name, func(t *testing.T) { + e, err := argsFromProcess(&tc.spec, &tc.p, true) + if err != nil { + t.Errorf("argsFromProcess(%+v): got error: %+v", tc.p, err) + return + } + if diff := cmp.Diff(*e, tc.expected, cmpopts.IgnoreUnexported(os.File{})); diff != "" { + t.Errorf("argsFromProcess(%+v): diff (+want -got):\n%s", tc.p, diff) + } + }) } }