8000 seccomp: allow ptrace(2) for 4.8+ kernels by tonistiigi · Pull Request #38137 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

seccomp: allow ptrace(2) for 4.8+ kernels #38137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 5, 2019
Merged

seccomp: allow ptrace(2) for 4.8+ kernels #38137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
seccomp: allow ptrace for 4.8+ kernels 
4.8+ kernels have fixed the ptrace security issues
so we can allow ptrace(2) on the default seccomp
profile if we do the kernel version check.

torvalds/linux@93e35ef

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
  • Loading branch information
@tonistiigi
tonistiigi committed Nov 4, 2018
commit 1124543ca8071074a537a15db251af46a5189907
5 changes: 3 additions & 2 deletions api/types/seccomp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ type Arg struct {

// Filter is used to conditionally apply Seccomp rules
type Filter struct {
Caps []string `json:"caps,omitempty"`
Arches []string `json:"arches,omitempty"`
Caps []string `json:"caps,omitempty"`
Arches []string `json:"arches,omitempty"`
MinKernel string `json:"minKernel,omitempty"`
}

// Syscall is used to match a group of syscalls in Seccomp
Expand Down
12 changes: 12 additions & 0 deletions profiles/seccomp/default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,18 @@
"includes": {},
"excludes": {}
},
{
"names": [
"ptrace"
],
"action": "SCMP_ACT_ALLOW",
"args": null,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using empty array [] instead of null for consistency.

"comment": "",
"includes": {
"minKernel": "4.8.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know generally we tried to avoid kernel version checks; Will this be an issue if distros backport this fix? (i.e., would there be another approach so that the default could manually be overridden at compile time?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a distro backports they will just not get the ptrace support even if it may be secure for their kernel as well. User can still fix it in that case by providing a custom profile or upgrading the kernel. 4.8 is documented the switch point on the man page. Even if I would dynamically detect the behavior of the current system there is no way to mark that condition in the profile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure if there's a cleaner solution. Was just thinking if (e.g.) Red Hat decides to backport, and Docker wants to ship Docker EE packages for RHEL with a correct profile

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ship a different default profile anyway, so I don't think this is an issue.

},
"excludes": {}
},
{
"names": [
"personality"
Expand Down
32 changes: 31 additions & 1 deletion profiles/seccomp/seccomp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import (
"fmt"

"github.com/docker/docker/api/types"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/docker/docker/pkg/parsers/kernel"
specs "github.com/opencontainers/runtime-spec/specs-go"
libseccomp "github.com/seccomp/libseccomp-golang"
)

Expand Down Expand Up @@ -95,6 +96,21 @@ func setupSeccomp(config *types.Seccomp, rs *specs.Spec) (*specs.LinuxSeccomp, e

newConfig.DefaultAction = specs.LinuxSeccompAction(config.DefaultAction)

var currentKernelVersion *kernel.VersionInfo
kernelGreaterEqualThan := func(v string) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I like defining local closures like this, can you just make it a helper function external to this? (yes, Go should allow local functions...)

version, err := kernel.ParseRelease(v)
if err != nil {
return false, err
}
if currentKernelVersion == nil {
currentKernelVersion, err = kernel.GetKernelVersion()
if err != nil {
return false, err
}
}
return kernel.CompareKernelVersion(*version, *currentKernelVersion) <= 0, nil
}

Loop:
// Loop through all syscall blocks and convert them to libcontainer format after filtering them
for _, call := range config.Syscalls {
Expand All @@ -110,6 +126,13 @@ Loop:
}
}
}
if call.Excludes.MinKernel != "" {
if ok, err := kernelGreaterEqualThan(call.Excludes.MinKernel); err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the error case should we not just include the rule anyway? ie fail closed not error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fail only in the case where profile had a version that couldn't be parsed. I think that case is similar to the other validation, eg. the call.Names check.

} else if ok {
continue Loop
}
}
if len(call.Includes.Arches) > 0 {
if !inSlice(call.Includes.Arches, arch) {
continue Loop
Expand All @@ -122,6 +145,13 @@ Loop:
}
}
}
if call.Includes.MinKernel != "" {
if ok, err := kernelGreaterEqualThan(call.Includes.MinKernel); err != nil {
return nil, err
} else if !ok {
continue Loop
}
}

if call.Name != "" && len(call.Names) != 0 {
return nil, errors.New("'name' and 'names' were specified in the seccomp profile, use either 'name' or 'names'")
Expand Down
7 changes: 7 additions & 0 deletions profiles/seccomp/seccomp_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,13 @@ func DefaultProfile() *types.Seccomp {
Action: types.ActAllow,
Args: []*types.Arg{},
},
{
Names: []string{"ptrace"},
Action: types.ActAllow,
Includes: types.Filter{
MinKernel: "4.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the kernel version is 4.8, there is not a 4.8.0 release, ir goes 4.8, 4.8.1...

},
},
{
Names: []string{"personality"},
Action: types.ActAllow,
Expand Down
0