Improve configuration of compatibility

[l0kod]

For now, the configuration is close to the Rust library, but we need to make it simpler to use and easier to understand. See landlock-lsm/rust-landlock#58

[l0kod]

Here is a new proposal replacing the current best_effort, soft_requirement, and hard_requirement compatLevel with tailored errorIfAnyUnsupported, cancelSandboxingIfAnyUnsupported, and (a new) errorIfAnyUnusablePath:

{
	"ruleset": [
		{
			"handledAccessFs": [ "v6.all" ]
		},
		{
			"handledAccessFs": [ "refer" ],
			"cancelSandboxingIfAnyUnsupported": true
		}
	],
	"pathBeneath": [
		{
			"allowedAccess": [ "execute", "read_file", "read_dir", "refer" ],
			"parentFd": [ ".", "/usr", "/etc", "/proc", "/tmp" ],
			"errorIfAnyUnusablePath": true
		},
		{
			"allowedAccess": [ "write_file", "remove_dir", "remove_file", "make_char", "make_dir", "make_reg", "make_sock", "make_fifo", "make_block", "make_sym", "refer", "truncate" ],
			"parentFd": [ "/tmp" ]
		}
	]
}

With this first configuration:

• cancelSandboxingIfAnyUnsupported: No restriction at all would be enforced if the running kernel does not support refer. In this case, this should be used when the sandboxed app must be able to create hardlinks (which is only possible with the refer access right).
• errorIfAnyUnusablePath: The sandboxing will failed with an error if one of the [ ".", "/usr", "/etc", "/proc", "/tmp" ] paths do not exist. This should be used when one of these paths should exist for the sandboxed app to correctly work.

---

	"ruleset": [
		{
			"handledAccessFs": [ "v6.all" ]
		}
	],
	"errorIfAnyUnsupported": true,
	"pathBeneath": [
		{
			"allowedAccess": [ "v6.read_execute" ],
			"parentFd": [ ".", "/usr", "/etc", "/proc", "/tmp" ]
		},
		{
			"allowedAccess": [ "v6.read_write" ],
			"parentFd": [ "/tmp" ]
		}
	]

With this second configuration:

• errorIfAnyUnsupported: The sandboxing will failed with an error if any of the handled access right are not supported by the running kernel. This should be used to for test environment to make sure the full sandboxing is tested, or in tailored systems were we know the running kernel should support these features.

Any opinion @gnoack, @rata, @Foxboron, @rgacogne?

[Foxboron]

I might be missing something, but a potential issue I see is that these are three flags where two are generic, but one is specific to one part of landlock. errorIfAnyUnusablePath only cares about file paths, but should we then also have specific restrictions for things like network and IPC?

Would this be more complicated then the current compat levels which are all generic?

[l0kod]

I might be missing something, but a potential issue I see is that these are three flags where two are generic,

That's right, the two generic flags are errorIfAnyUnsupported and cancelSandboxingIfAnyUnsupported. Any better name?

but one is specific to one part of landlock. errorIfAnyUnusablePath only cares about file paths, but should we then also have specific restrictions for things like network and IPC?

In some cases we might want to know if a file is missing, hence the specific errorIfAnyUnusablePath flag. Thinking about that again, I guess we should remove this specific flag. If there is a real need for that, it will be easy to extend the schema. The important point is, by default, to not error out if a path doesn't exist, otherwise it would make configuration writing and maintenance very unpractical. Testing for existence of a required file should be done by the sandboxed app/container anyway.

Would this be more complicated then the current compat levels which are all generic?

This proposal indeed brings narrower configuration bits. The current compat levels looks too generic and might be confusing. Soft and hard requirements are not clear enough: opencontainers/runtime-spec#1241 (comment)
See this discussion about the two use cases landlock-lsm/rust-landlock#12 (comment) and the FOSDEM talk.

This new approach should be easier to understand and meet the requirements for tests and compatibility.

[rata]

This might be personal, but cancelSandboxingIfAnyUnsupported gave me the wrong impression by only looking at the name: I thought sandboxing was aborted (cancelled). What something like ignoreUnsupported? But this might be just me, feel free to ignore me if you like the current name :)

Also, If I'm getting the right meaning for all of this, it seems to me that all the flags are basically the same: return an error if <the appropiate thing for this context> fails. So, in my mind, this could be the same flag. I'm bad with names, but like: enforce_policy: <string>.? For example:

{
	"ruleset": [
		{
			"handledAccessFs": [ "v6.all" ]
		},
		{
			"handledAccessFs": [ "refer" ],
			"enforce_policy": "FailOnUnknownAccess"
		}
	],
	"pathBeneath": [
		{
			"allowedAccess": [ "execute", "read_file", "read_dir", "refer" ],
			"parentFd": [ ".", "/usr", "/etc", "/proc", "/tmp" ],
			"enforce_policy": "FailOnMissingDir"
		},
		{
			"allowedAccess": [ "write_file", "remove_dir", "remove_file", "make_char", "make_dir", "make_reg", "make_sock", "make_fifo", "make_block", "make_sym", "refer", "truncate" ],
			"parentFd": [ "/tmp" ]
		}
	]
}

And errorIfAnyUnsupported can just be a default enforce_policy, like:

	"ruleset": [
		{
			"handledAccessFs": [ "v6.all" ]
		}
	],
	"pathBeneath": 
           "enforce_policy": "FailOnUnsupported"
           "rules": [
		{
			"allowedAccess": [ "v6.read_execute" ],
			"parentFd": [ ".", "/usr", "/etc", "/proc", "/tmp" ]
		},
		{
			"allowedAccess": [ "v6.read_write" ],
			"parentFd": [ "/tmp" ]
		}
	]

This would be a default policy that each rule can override with a "more local" policy like in the previous example. But if you want the behavior of having unchanged rules between prod and testing and have a single thing to set to enforce them all (like the global errorIfAnyUnsupported), maybe pathBeneath can also have a "override_rules_policy: FailOnError"

And maybe the value foe the policy can always be FailOnError and that means any errors to apply that will create a failure. Maybe we don't need to be so specific in the values? I think I prefer FailOnError for all too, it seems simpler to me.

That wouldn't work if you want to differentiate errors of missing dirs vs missing paths in the pathBeneath section, but I think we can start with FailOnError and add FailOnMissingPath in the future, if needed.

What do you think?

[l0kod]

This might be personal, but cancelSandboxingIfAnyUnsupported gave me the wrong impression by only looking at the name: I thought sandboxing was aborted (cancelled).

Well, that is exactly the meaning. 😄

This kind of toggle might be needed in the case of a sandboxed program really needs some capabilities which are denied by earlier version of Landlock. For instance, if a program really needs to be able to do hardlinks, the refer access right would need to be supported by the running kernel (which may not be know to the person writing this configuration file). Other always-denied-for-now capabilities could similarly be allowed by future Landlock version (e.g. ptrace a set of processes outside the sandbox, create specific mount points) with new access rights, and program might just not work without these new supported capabilities. That's why we need a way to not create a sandbox if some features are not supported, because by default we follow a best-effort approach.

I was thinking about interruptOnUnsupported, but interrupt might not be clear enough.

What something like ignoreUnsupported? But this might be just me, feel free to ignore me if you like the current name :)

The default behavior is best-effort, which would be ignoreUnsupported, so no need for a specific toggle.

Also, If I'm getting the right meaning for all of this, it seems to me that all the flags are basically the same: return an error if <the appropiate thing for this context> fails.

That wasn't my intention. The goal of the cancel* (or interrupt*) toggles are to not create a sandbox and not return an error.

However, the error* toggles return an error/failure because these are unattended things.

So, in my mind, this could be the same flag. I'm bad with names, but like: enforce_policy: <string>.? For example:

{
	"ruleset": [
		{
			"handledAccessFs": [ "v6.all" ]
		},
		{
			"handledAccessFs": [ "refer" ],
			"enforce_policy": "FailOnUnknownAccess"
		}
	],
	"pathBeneath": [
		{
			"allowedAccess": [ "execute", "read_file", "read_dir", "refer" ],
			"parentFd": [ ".", "/usr", "/etc", "/proc", "/tmp" ],
			"enforce_policy": "FailOnMissingDir"
		},
		{
			"allowedAccess": [ "write_file", "remove_dir", "remove_file", "make_char", "make_dir", "make_reg", "make_sock", "make_fifo", "make_block", "make_sym", "refer", "truncate" ],
			"parentFd": [ "/tmp" ]
		}
	]
}

FailOnUnknownAccess contains "Access", but other ruleset properties might not be access rights, for instance the scoped flags to restrict signals and abstract UNIX socket are not strictly access right because they is not a related rule type to add exceptions.

What about FailOnUnknown`

And errorIfAnyUnsupported can just be a default enforce_policy, like:

	...
	"pathBeneath": 
           "enforce_policy": "FailOnUnsupported"
           ...
	]

This would be a default policy that each rule can override with a "more local" policy like in the previous example. But if you want the behavior of having unchanged rules between prod and testing and have a single thing to set to enforce them all (like the global errorIfAnyUnsupported), maybe pathBeneath can also have a "override_rules_policy: FailOnError"

And maybe the value foe the policy can always be FailOnError and that means any errors to apply that will create a failure. Maybe we don't need to be so specific in the values? I think I prefer FailOnError for all too, it seems simpler to me.

That wouldn't work if you want to differentiate errors of missing dirs vs missing paths in the pathBeneath section, but I think we can start with FailOnError and add FailOnMissingPath in the future, if needed.

What do you think?

FailOnError may be difficult to understand because "fail" and "error" can mean the same thing, and "error on error" doesn't make sense.

Actually, I don't think errorIfAnyUnusablePath (or any error*) would be useful. We might want to get warnings but that should be in the API that parses the config, not the config itself.

Summary

The error* toggles should be moved from the configuration file to the library API. By default all non-Landlock errors will be ignored but still available in the form of warnings through the library. For instance, a file path that doesn't exist will just (safely) be ignored.

The cancelSandboxing prefix correctly reflects its meaning, so we should probably keep it. We can keep cancelSandboxingIfAnyUnsupported as-is or replace it with something like:

• cancelSandboxingIfUnsupported
• cancelSandboxingOnUnsupported
• cancelSandboxingIfNotFullySupported

Any though?

Use of this toggle should be rare, so for now I'll remove all compatibility toggles and introduce this one later in the config format.

[rata]

Ohh, okay, I was missing some context, thanks! I'm not a fan of so many toggles and I think there should be a way to reduce them and be simpler too.

Other ideas to explore might be to express a min ABI required and what to do if it isn't.

The default behavior is best-effort, which would be ignoreUnsupported, so no need for a specific toggle.

Sorry I wasn't clear, the point is being able to set it to false. It could be ignoreErrors (bool) or whatever.

[l0kod]

Other ideas to explore might be to express a min ABI required and what to do if it isn't.

That's a simplest approach for this library development, but a more complex one for users because they need to figure out what those ABIs mean, and map them to a set of features. At the end, they also need to define if some features must be supported or not with a toggle (e.g. cancelSandboxingIf...), so using explicit ABIs adds another layer of complexity (that this library can handle) and it doesn't solve anything.

[l0kod]

The default behavior is best-effort, which would be ignoreUnsupported, so no need for a specific toggle.

Sorry I wasn't clear, the point is being able to set it to false. It could be ignoreErrors (bool) or whatever.

OK, but as explained above, explicit error should be handled by the library's API, not the configuration file, so we don't need such toggle in the JSON configuration.

However, we should still be able to explicitly set if a specific feature is required for the program to work as intended. We discussed about the naming for the Rust library case, and in this issue as well, and I think cancelSandboxOnUnsupported = true makes the most sense. If set and if a specific set of access rights are not supported, then the landlockconfig_parse_*() will return NULL instead of -errno (see #18), and the program/container will be launched without sandbox.

[rata]

Thanks for the links, now I see why the refer thing is important :)

I'm kind of lost of what is the proposal now: remove the compatibility levels, we will use best-effort for now, and in the future maybe add cancelSandboxOnUnsupported (or some similar name but something that has this effect)?

[l0kod]

I'm kind of lost of what is the proposal now: remove the compatibility levels, we will use best-effort for now, and in the future maybe add cancelSandboxOnUnsupported (or some similar name but something that has this effect)?

Yes, best-effort will be the only supported mode, no explicit configuration should generate any error, only cancelSandboxOnUnsupported = true or a fully unsupported access rights may not return a ruleset and then not sandbox at all.

I'll probably add this toggle in a few weeks, but this will not be an important change. It should be there for the stable schema though, so we should validate this design until then.