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

libcontainer: add support for Landlock #3194

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kailun-qin
Copy link
Contributor

This patch introduces Landlock Linux Security Module (LSM) support in
runc, which was landed in Linux kernel 5.13.

This allows unprivileged processes to create safe security sandboxes
that can securely restrict the ambient rights (e.g. global filesystem
access) for themselves.

runtime-spec: opencontainers/runtime-spec#1111

Fixes #2859

Signed-off-by: Kailun Qin [email protected]

@AkihiroSuda AkihiroSuda added this to the 1.2.0 milestone Sep 1, 2021
@@ -211,6 +212,14 @@ func (l *linuxStandardInit) Init() error {
// since been resolved.
// https://github.com/torvalds/linux/blob/v4.9/fs/exec.c#L1290-L1318
_ = unix.Close(l.fifoFd)

// `noNewPrivileges` must be enabled to use Landlock.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use github markdown in comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed markdown.

}

// RulePathBeneath defines the file-hierarchy typed rule that grants the access rights specified by
// `AllowedAccess` to the file hierarchies under the given `Paths` in Landlock.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use github markdown in comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed markdown.

Comment on lines 112 to 126
const (
Execute AccessFS = (1 << 0)
WriteFile AccessFS = (1 << 1)
ReadFile AccessFS = (1 << 2)
ReadDir AccessFS = (1 << 3)
RemoveDir AccessFS = (1 << 4)
RemoveFile AccessFS = (1 << 5)
MakeChar AccessFS = (1 << 6)
MakeDir AccessFS = (1 << 7)
MakeReg AccessFS = (1 << 8)
MakeSock AccessFS = (1 << 9)
MakeFifo AccessFS = (1 << 10)
MakeBlock AccessFS = (1 << 11)
MakeSym AccessFS = (1 << 12)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than a copy/paste from https://github.com/landlock-lsm/go-landlock/blob/main/landlock/syscall/landlock.go maybe it can be imported, and the constants are used directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, use built-in consts from go-landlock directly so remove these.

Comment on lines 9 to 35
var accessFSs = map[string]configs.AccessFS{
"execute": configs.Execute,
"write_file": configs.WriteFile,
"read_file": configs.ReadFile,
"read_dir": configs.ReadDir,
"remove_dir": configs.RemoveDir,
"remove_file": configs.RemoveFile,
"make_char": configs.MakeChar,
"make_dir": configs.MakeDir,
"make_reg": configs.MakeReg,
"make_sock": configs.MakeSock,
"make_fifo": configs.MakeFifo,
"make_block": configs.MakeBlock,
"make_sym": configs.MakeSym,
}

// ConvertStringToAccessFS converts a string into a Landlock access right.
func ConvertStringToAccessFS(in string) (configs.AccessFS, error) {
if access, ok := accessFSs[in]; ok {
return access, nil
}
return 0, fmt.Errorf("string %s is not a valid access right for landlock", in)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have this functionality in https://github.com/landlock-lsm/go-landlock/blob/main/landlock/accessfs.go and use it.

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I got this same concern here. So still keep at this moment and update with more comments.

Copy link

Choose a reason for hiding this comment

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

Feel free to create a PR for go-landlock 😉

Copy link

Choose a reason for hiding this comment

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

As I mentioned also in landlock-lsm/go-landlock#14, please be aware that when this string-to-enum mapping is part of go-landlock, the set of supported values might grow at some point in the future when you update the go-landlock library, and this might change the permitted values in your configuration language implicitly.

If this is the approach you want to take, and since @l0kod seems to also be on board, we could make that mapping part of the library, in the lower-case, underscore-delimited variant ("write_file")

Copy link

Choose a reason for hiding this comment

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

It may help to have an helper to check that an access right is supported by a specific ABI version, or specify the version as an argument. I'm not sure if the string conversion belongs to go-landlock or libcontainer though. Is libcontainer used by other container runtimes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be usable for other container runtimes but I'm not sure here. @kolyshkin @AkihiroSuda @cyphar Any insights? Thanks!

Comment on lines 45 to 49
// Convert Libcontainer AccessFS to go-landlock AccessFSSet.
func getAccess(access configs.AccessFS) landlock.AccessFSSet {
return landlock.AccessFSSet(access)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use landlock.AccessFSSet type directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original intention was to maintain a configs.AccessFS to have a decoupled layer between specs and runc, as well as runc and go-landlock, in case that an AccessFS is supported in one place not the other.

Rethink over this again, I think using landlock.AccessFSSet can achieve the above also, as long as we have explicit control/convert in runc (partially discussed here https://github.com/opencontainers/runc/pull/3194/files#r700925526).

}

// Convert Libcontainer RulePathBeneath to go-landlock PathOpt.
func getPathAccess(rule *configs.RulePathBeneath) landlock.PathOpt {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefixes such as get are discouraged by "effective go", see https://golang.org/doc/effective_go#Getters

IOW getPathAccess -> pathAccess.

Same for other getXxx... functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

Comment on lines 1 to 19
// +build !linux

package landlock

import (
"errors"

"github.com/opencontainers/runc/libcontainer/configs"
)

var ErrLandlockNotSupported = errors.New("land: config provided but Landlock not supported")

// InitLandlock does nothing because Landlock is not supported.
func InitSLandlock(config *configs.Landlock) error {
if config != nil {
return ErrLandlockNotSupported
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

runc is linux-only, I guess you can drop this entirely (as well as _linux suffix from all the other files).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed entirely!

@@ -320,6 +321,14 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
Ambient: spec.Process.Capabilities.Ambient,
}
}
if spec.Process.Landlock != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you have nil check in SetupLandlock already, this one seems redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks!

}

// AccessFS is taken upon ruleset and rule setup in Landlock.
type AccessFS uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use landlock.AccessFS type directly (or, if you're so inclined to have a locally defined type, use something like type AccessFS=landlock.AccessFS so you won't need to convert).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated to use landlock.AccessFSSet directly.

var ErrLandlockNotSupported = errors.New("land: config provided but Landlock not supported")

// InitLandlock does nothing because Landlock is not supported.
func InitSLandlock(config *configs.Landlock) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo? (extra S)

if err := llConfig.RestrictPaths(
getPathAccesses(config.Rules)...,
); err != nil {
return fmt.Errorf("Could not restrict paths: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: error messages should not be Capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

"github.com/opencontainers/runc/libcontainer/configs"
)

var ErrLandlockNotSupported = errors.New("land: config provided but Landlock not supported")
Copy link

Choose a reason for hiding this comment

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

What does "land:" mean?

@kailun-qin
Copy link
Contributor Author

Address comments and add unit tests. PTAL, thanks!

var llConfig landlock.Config

ruleset := getAccess(config.Ruleset.HandledAccessFS)
// Panic on error when constructing the Landlock configuration using invalid config values.
Copy link

Choose a reason for hiding this comment

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

You can use landlock.NewConfig() instead to handle this more gracefully. (It returns the config and an error)

Copy link
Contributor Author

@kailun-qin kailun-qin Sep 9, 2021

Choose a reason for hiding this comment

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

Thanks, updated! More make sense to use NewConfig instead.

llConfig = landlock.MustConfig(ruleset).BestEffort()
}

if err := llConfig.RestrictPaths(
Copy link

@gnoack gnoack Sep 8, 2021

Choose a reason for hiding this comment

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

I have today changed the semantics here slightly, so that the AccessFSSet passed as PathAccess() parameter needs to be a subset of the one configured in MustConfig(). The intention being that it should be forbidden to say "A and B are forbidden, but C is permitted on /tmp". That makes no sense to say, as C was never forbidden, so it's now treated as an error. You might want to catch that case at the configuration level already, otherwise users will get an error from RestrictPaths() if they misconfigure it in their config file.

Commit is here:
landlock-lsm/go-landlock@c567107

See bug landlock-lsm/go-landlock#18 for details.

Sorry about the slight API breakage, I'm hoping this is for the better in the long run, to be strict about the inputs.

This patch introduces Landlock Linux Security Module (LSM) support in
runc, which was landed in Linux kernel 5.13.

This allows unprivileged processes to create safe security sandboxes
that can securely restrict the ambient rights (e.g. global filesystem
access) for themselves.

runtime-spec: opencontainers/runtime-spec#1111

Fixes opencontainers#2859

Signed-off-by: Kailun Qin <[email protected]>
* use landlock.AccessFSSet type directly
* remove non-linux files
* some minor updates

Signed-off-by: Kailun Qin <[email protected]>
@AkihiroSuda
Copy link
Member

@kailun-qin

Could you update this PR with the latest revision of opencontainers/runtime-spec#1111 ?
Then we can confirm the design of the PRs toward merging.

Thanks!

@AkihiroSuda
Copy link
Member

ping @kailun-qin 🙏

@AkihiroSuda AkihiroSuda modified the milestones: 1.2.0, 1.3.0 Jul 17, 2023
@Zheaoli
Copy link
Contributor

Zheaoli commented Nov 12, 2023

@AkihiroSuda May I carry this PR?

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

Grr, I wanted to post on the runtime-spec, sorry 😂. Ignore this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Landlock LSM?
7 participants