libcontainer: add support for Landlock #3194

kailun-qin · 2021-09-01T09:52:03Z

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 kailun.qin@intel.com

kolyshkin · 2021-09-01T23:29:07Z

libcontainer/standard_init_linux.go

@@ -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.


Please do not use github markdown in comments.

Thanks, removed markdown.

kolyshkin · 2021-09-01T23:30:36Z

libcontainer/configs/config.go

+}
+
+// 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.


Please do not use github markdown in comments.

Thanks, removed markdown.

kolyshkin · 2021-09-01T23:37:00Z

libcontainer/configs/config.go

+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)
+)


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?

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

kolyshkin · 2021-09-01T23:38:52Z

libcontainer/landlock/config.go

+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)
+}


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

Related to landlock-lsm/go-landlock#14

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

Feel free to create a PR for go-landlock 😉

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")

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?

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

kolyshkin · 2021-09-01T23:40:07Z

libcontainer/landlock/landlock_linux.go

+// Convert Libcontainer AccessFS to go-landlock AccessFSSet.
+func getAccess(access configs.AccessFS) landlock.AccessFSSet {
+	return landlock.AccessFSSet(access)
+}
+


Why not use landlock.AccessFSSet type directly?

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).

kolyshkin · 2021-09-01T23:44:34Z

libcontainer/landlock/landlock_linux.go

+}
+
+// Convert Libcontainer RulePathBeneath to go-landlock PathOpt.
+func getPathAccess(rule *configs.RulePathBeneath) landlock.PathOpt {


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

Thanks, updated.

kolyshkin · 2021-09-01T23:45:41Z

libcontainer/landlock/landlock_unsupported.go

+// +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
+}


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

Thanks, removed entirely!

kolyshkin · 2021-09-01T23:47:22Z

libcontainer/specconv/spec_linux.go

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


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

Removed, thanks!

kolyshkin · 2021-09-01T23:49:45Z

libcontainer/configs/config.go

+}
+
+// AccessFS is taken upon ruleset and rule setup in Landlock.
+type AccessFS uint64


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).

Thanks, updated to use landlock.AccessFSSet directly.

kolyshkin · 2021-09-01T23:50:54Z

libcontainer/landlock/landlock_unsupported.go

+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 {


typo? (extra S)

kolyshkin · 2021-09-01T23:52:28Z

libcontainer/landlock/landlock_linux.go

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


nit: error messages should not be Capitalized.

Updated, thanks!

l0kod · 2021-09-02T09:49:10Z

libcontainer/landlock/landlock_unsupported.go

+	"github.com/opencontainers/runc/libcontainer/configs"
+)
+
+var ErrLandlockNotSupported = errors.New("land: config provided but Landlock not supported")


What does "land:" mean?

kailun-qin · 2021-09-03T07:04:19Z

Address comments and add unit tests. PTAL, thanks!

gnoack · 2021-09-08T19:23:45Z

libcontainer/landlock/landlock_linux.go

+	var llConfig landlock.Config
+
+	ruleset := getAccess(config.Ruleset.HandledAccessFS)
+	// Panic on error when constructing the Landlock configuration using invalid config values.


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

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

gnoack · 2021-09-08T19:31:18Z

libcontainer/landlock/landlock_linux.go

+		llConfig = landlock.MustConfig(ruleset).BestEffort()
+	}
+
+	if err := llConfig.RestrictPaths(


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 <kailun.qin@intel.com>

* use landlock.AccessFSSet type directly * remove non-linux files * some minor updates Signed-off-by: Kailun Qin <kailun.qin@intel.com>

Signed-off-by: Kailun Qin <kailun.qin@intel.com>

AkihiroSuda · 2022-09-01T16:15:11Z

@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 · 2023-01-26T03:34:30Z

ping @kailun-qin 🙏

Zheaoli · 2023-11-12T05:51:58Z

@AkihiroSuda May I carry this PR?

rata

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

kailun-qin mentioned this pull request Sep 1, 2021

Support Landlock LSM? #2859

Open

AkihiroSuda added this to the 1.2.0 milestone Sep 1, 2021

kolyshkin reviewed Sep 1, 2021

View reviewed changes

l0kod reviewed Sep 2, 2021

View reviewed changes

gnoack mentioned this pull request Sep 5, 2021

Converting allowedAccess strings into AccessFSSet uint64 landlock-lsm/go-landlock#14

Open

gnoack reviewed Sep 8, 2021

View reviewed changes

kailun-qin force-pushed the add-landlock branch from 0fa5201 to 075542f Compare September 9, 2021 05:55

kailun-qin added 5 commits September 9, 2021 09:44

Address comments

6f64c97

* use landlock.AccessFSSet type directly * remove non-linux files * some minor updates Signed-off-by: Kailun Qin <kailun.qin@intel.com>

Add unit test for SetupLandlock

6a698a6

Signed-off-by: Kailun Qin <kailun.qin@intel.com>

Add check for DisableBestEffort in the unit test

22802a8

Signed-off-by: Kailun Qin <kailun.qin@intel.com>

Update go-landlock and use NewConfig instead

075542f

Signed-off-by: Kailun Qin <kailun.qin@intel.com>

BoardzMaster mentioned this pull request Sep 27, 2021

Support Landlock LSM containerd/containerd#6056

Closed

AkihiroSuda modified the milestones: 1.2.0, 1.3.0 Jul 17, 2023

Zheaoli mentioned this pull request Jan 9, 2024

Carry #1111: specs-go/config: add Landlock LSM support opencontainers/runtime-spec#1241

Open

rata reviewed Mar 14, 2024

View reviewed changes

libcontainer: add support for Landlock #3194

Are you sure you want to change the base?

libcontainer: add support for Landlock #3194

Conversation

kailun-qin commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailun-qin commented Sep 3, 2021

Choose a reason for hiding this comment

kailun-qin Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

gnoack Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda commented Sep 1, 2022

AkihiroSuda commented Jan 26, 2023

Zheaoli commented Nov 12, 2023

rata left a comment • edited Loading

Choose a reason for hiding this comment

kailun-qin Sep 9, 2021 •

edited

Loading

gnoack Sep 8, 2021 •

edited

Loading

rata left a comment •

edited

Loading