Skip to content
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.

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

RFC: libcontainer: remove dependency on libapparmor #1675

Merged
merged 1 commit into from
Dec 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ env:
before_install:
- echo "deb http://archive.ubuntu.com/ubuntu trusty-backports main restricted universe multiverse" | sudo tee -a /etc/apt/sources.list
- sudo apt-get -qq update
- sudo apt-get install -y libapparmor-dev libseccomp-dev/trusty-backports
- sudo apt-get install -y libseccomp-dev/trusty-backports
- go get -u github.com/golang/lint/golint
- go get -u github.com/vbatts/git-validation
- env | grep TRAVIS_
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ make BUILDTAGS='seccomp apparmor'
|-----------|------------------------------------|-------------|
| seccomp | Syscall filtering | libseccomp |
| selinux | selinux process and mount labeling | <none> |
| apparmor | apparmor profile support | libapparmor |
| apparmor | apparmor profile support | <none> |
| ambient | ambient capability support | kernel 4.3 |


Expand Down
37 changes: 26 additions & 11 deletions libcontainer/apparmor/apparmor.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,10 @@

package apparmor

// #cgo LDFLAGS: -lapparmor
// #include <sys/apparmor.h>
// #include <stdlib.h>
import "C"
import (
"fmt"
"io/ioutil"
"os"
"unsafe"
)

// IsEnabled returns true if apparmor is enabled for the host.
Expand All @@ -24,16 +19,36 @@ func IsEnabled() bool {
return false
}

func setprocattr(attr, value string) error {
// Under AppArmor you can only change your own attr, so use /proc/self/
// instead of /proc/<tid>/ like libapparmor does
path := fmt.Sprintf("/proc/self/attr/%s", attr)

f, err := os.OpenFile(path, os.O_WRONLY, 0)
if err != nil {
return err
}
defer f.Close()

_, err = fmt.Fprintf(f, "%s", value)
return err
}

// changeOnExec reimplements aa_change_onexec from libapparmor in Go
func changeOnExec(name string) error {
value := "exec " + name
if err := setprocattr("exec", value); err != nil {
return fmt.Errorf("apparmor failed to apply profile: %s", err)
}
return nil
}

// ApplyProfile will apply the profile with the specified name to the process after
// the next exec.
func ApplyProfile(name string) error {
if name == "" {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Should check !strings.Contains(name, "/")

Copy link
Member

@cyphar cyphar Dec 11, 2017

Choose a reason for hiding this comment

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

Why? Profile names can have / in them IIRC (and they can also not have / in them as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would something like the following be sufficient?

if strings.Contains(name, "/") {
    return fmt.Errorf("apparmor profile name cannot contain '/'")
}

Copy link
Member

Choose a reason for hiding this comment

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

At least ".." should be prohibited to prevent potential filesystem contamination

Copy link
Member

Choose a reason for hiding this comment

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

np, I misread the code

cName := C.CString(name)
defer C.free(unsafe.Pointer(cName))
if _, err := C.aa_change_onexec(cName); err != nil {
return fmt.Errorf("apparmor failed to apply profile: %s", err)
}
return nil

return changeOnExec(name)
}