Improve the ABI documentation

[l0kod]

Explain more clearly why sticking to a specific Landlock ABI version to infer access rights is highly recommended.

Add links to the landlock-test-tools repository and related CI changes.

[l0kod]

Cc @cd-work @kylewillmon

[cd-work]

I don't see how this would be undefined behavior? The behavior is 100% predictable and consistent based on the rust-landlock version being used, right? There's no dragons to be found anywhere.

Updating landlock can change the behavior of the application, but that's just how updating dependencies works in general. I don't think that's unexpected and might even be intentional? I know that's why I personally used BitFlags::all.

[l0kod]

It may be undefined because when app's code is written, rust-landlock might be at v1.2.0 and when this lib will add support for a new access right it will bump to v1.3.0 which will be automatically used by someone building the app. At this point, folks building the app may know or not what the new access right is, but there is a (big) chance that they don't know. The impact will be that the built app, with the same source code, will have a different behavior and may break if a required kernel feature is then denied because of the BitFlags::ALL. So, at the time of the code was written this behavior couldn't be known, which I translate to "undefined".

If people use BitFlags::ALL their is a good chance that their application will break at some point because of that.

[cd-work]

It may be undefined because when app's code is written, rust-landlock might be at v1.2.0 and when this lib will add support for a new access right it will bump to v1.3.0 which will be automatically used by someone building the app.

Lockfiles should prevent this from ever happening.

[l0kod]

Do you mean Cargo.lock? This shouldn't be used for libraries (that might use rust-landlock), and even apps should not rely on that (in theory) to not break.

[dburgener]

I was chatting about this with @l0kod offline. Here's my two cents on the undefined behavior discussion.

IMHO it is correct that this is "undefined behavior". The discussion seems to dance around exactly what is meant by undefined behavior, but in the sense that "the spec doesn't define it", I think this is undefined. The point you are making (IIUC) is that a future landlock may make unexpected changes. You are not making any guarantee about future access rights being added.

I do think that it would be a stretch to call such behavior "indeterministic". The future behavior will presumably be deterministic (non-deterministic security decisions sure sounds like a security bug in most contexts...).

All of that said, I think the reason "undefined behavior" can feel a little weird here is because in practice you do expect a pretty constrained set of behaviors. Either access will be allowed or denied. Possibly in ways that are difficult to anticipate at this point. "We don't define whether we allow or deny an access" is very different in practice from "We don't define anything, and setting your computer on fire is legal behavior". In some contexts (eg compilers) it can be genuinely difficult to predict the effect of optimizations on undefined behavior. As this article points out, in the presence of optimizations, it's theoretically possible for C compilers to reorder instructions around undefined behavior in ways that cause writes at arbitrary locations in the program's memory space, which very much get outside the scope of things like "crash" that C programmers tend to think that a program with undefined behavior may do.

The reason I bring all that up, is that the landlock scenario is much less scary (I assume) than a compiler that does optimizations such that the behavior could in theory be extremely destructive. It's just not defined what access will be allowed or disallowed in future versions if you rely on future access controls. For that reason, while "undefined behavior" is technically correct, I think it might be helpful to use a less loaded phrase such as "landlock makes no guarantees around access decisions...", which explicitly caveat the scope of the lack of definition. It's both more precise and less scary that way.

[l0kod]

Thanks @dburgener, that's helpful! I agree and I'll update the doc accordingly.

[cd-work]

Calling this undefined behavior seems like a stretch, considering nothing will change unless the landlock dependencies are updated.

I've been mainly thinking about filesystem access, but I suppose the main issue here would be if something like network sandboxing was added. In that scenario people might suddenly not be able to do any networking anymore.

However what I personally was looking for was more something like AccessFs::all (which doesn't exist) rather than BitFlags::all. I think this is much less likely to have unexpected side-effects and at least to me it somewhat makes sense that someone would want to handle every file permission that will ever be supported by landlock?

[l0kod]

Calling this undefined behavior seems like a stretch, considering nothing will change unless the landlock dependencies are updated.

It might be a minor update because only a feature was added (e.g. support a new access right). How would you call that?

I've been mainly thinking about filesystem access, but I suppose the main issue here would be if something like network sandboxing was added. In that scenario people might suddenly not be able to do any networking anymore.

Yes, that is one possibility, but even with filesystem that could happen. For instance, one day we'll be able to deny walking through a file hierarchy. If this BitFlags::ALL is used, the app will broke with a simple cargo update or even cargo install.

However what I personally was looking for was more something like AccessFs::all (which doesn't exist) rather than BitFlags::all. I think this is much less likely to have unexpected side-effects and at least to me it somewhat makes sense that someone would want to handle every file permission that will ever be supported by landlock?

There is AccessFs::from_all(ABI), and again you need to specify an ABI, otherwise you cannot be sure that your app will not break because you don't control the sandboxing.

[cd-work]

It might be a minor update because only a feature was added (e.g. support a new access right). How would you call that?

It's just a breaking change. That's not undefined behavior, the behavior is clearly defined.

Yes, that is one possibility, but even with filesystem that could happen. For instance, one day we'll be able to deny walking through a file hierarchy. If this BitFlags::ALL is used, the app will broke with a simple cargo update or even cargo install.

Hmm, I don't see how that would work (the walking a file hierarchy part), but I suppose I might just be lacking some details about landlocks future plans.

There is AccessFs::from_all(ABI), and again you need to specify an ABI, otherwise you cannot be sure that your app will not break because you don't control the sandboxing.

Yeah that's fair, it's just hard to keep up to date with things. So using from_all(ABI) will inevitably cause new features to not be supported. And recent additions to landlock like FS_TRUNCATE have shown that you really want to support the latest version if possible, because otherwise there'd be major holes for no good reason.

[l0kod]

Yeah that's fair, it's just hard to keep up to date with things. So using from_all(ABI) will inevitably cause new features to not be supported. And recent additions to landlock like FS_TRUNCATE have shown that you really want to support the latest version if possible, because otherwise there'd be major holes for no good reason.

Well, what if your application requires FS_TRUNCATE to properly work? This new access right wasn't a fix but a new feature. Landlock is definitely not complete yet and it will take some years, and maybe more for everyone to update their kernels. 😉

What we really want is to check and test new features, and update the vetted app regularly, not blindly apply new restrictions right?

What is the issue with a global ABI variable incremented from time to time (once tested)?

[cd-work]

Well, what if your application requires FS_TRUNCATE to properly work? This new access right wasn't a fix but a new feature. Landlock is definitely not complete yet and it will take some years, and maybe more for everyone to update their kernels. 😉

Yeah that's fair, my concerns can probably just be summarized as "growing pains" and it probably makes more sense to structure things around the way they should be.

What is the issue with a global ABI variable incremented from time to time (once tested)?

The problem is that it requires a lot of manual intervention which will inevitably fall behind.

[l0kod]

What is the issue with a global ABI variable incremented from time to time (once tested)?

The problem is that it requires a lot of manual intervention which will inevitably fall behind.

Which kind of intervention? It should only be a matter of incrementing a number, testing with this change on a new kernel, and committing this one-line patch.

[cd-work]

I mean you have to realize there's an update first, then see what changed, increment that number, and test it.

That's not insignificant, especially if you're maintaining a large number of other projects at the same time.

[l0kod]

Cc @gnoack

I guess your point-of-view is someone that really care and invest in security, and you'd like something as secure as possible with a low maintenance cost, which I totally get. I'd like as much applications as possible to be sandboxed, and then target developers that aren't willing to invest too much effort in securing their app, and most importantly to keep it sandboxed (at least the same way). For these folks, if using rust-landlock breaks a legitimate use case of their app while they didn't change anything (except cargo update, them or someone else), they will not like that, distrust this crate and stop sandboxing their app, which would be counterproductive. I prefer to not break other's applications than have a hopefully-better-but-maybe-breaking auto sandboxing.

So you're right that this implies a cost to keep sandboxing as best as possible, but if I have to choose, I prefer that there is no cost (nor risk) for developers to update all their dependency crates (including rust-landlock).

If we bump rust-landlock to the next major (potentially breaking) version for each new access right, it would be the same cost for developers to get to increment the version in Cargo.toml than to increment the ABI in a global variable. It would also be much more costly to maintain the rust-landlock crate because of a lot of maintained different major versions. So I don't think this approach is viable.

It might be a minor update because only a feature was added (e.g. support a new access right). How would you call that?

It's just a breaking change. That's not undefined behavior, the behavior is clearly defined.

It would not be a breaking change as Rust defines it. The API will still be compatible, it will build, and it may work fine or break a runtime (e.g. according to a specific user configuration, or environment, or for every user). At the time you're using BitFlags::ALL with an up-to-date rust-landlock library, it's not an undefined behavior. But some time later, someone else picking the same source code and building it might get a different behavior that was observed by the initial developer. I guess a more precise name might be "nondeterministic behavior", but I think calling it "undefined behavior" is not that wrong, well known, and scary enough.

To know what an application needs, developers need to use the rust-landlock API to define these requirements. However, how do we define something for which there is no word (i.e. access right) yet? I don't see any perferct solution here, but I'm open to suggestions. I have a WIP code to manage categories/groups of access rights (e.g. all, read, write, custom). I think that might be a part of the solution.

Another example is the upcoming support for IOCTL access control. This one is tricky because applications might indirectly use some legitimate IOCTL. On the kernel side, we're doing our best to make it consistent, but we never know...

Right now, BitFlags::ALL is allowed, and you're using it at you own risk, which is OK. I think you understand the consequences of this choice, but what do you think a non-security-expert developer should do? And then, what the documentation should encourage or discourage? I'm not convinced yet that keeping BitFlags::ALL would be a good idea, but it is not going away in short term. In the future we could have a part of the API explicitly marked "to use at your own risk", that you could use. What do you think?

Right now, I'd like to improve the documentation for "most developers", hence to avoid pitfalls by default. What do you think about the documentation update?

[cd-work]

It would not be a breaking change as Rust defines it. The API will still be compatible, it will build, and it may work fine or break a runtime (e.g. according to a specific user configuration, or environment, or for every user).

Yeah I think it should be fine as long as it's documented. It's basically comparable to #[non_exhaustive] on an enum, just without explicit compiler support. And that's going to go away if you ever disallow using BitFlags::all.

Right now, BitFlags::ALL is allowed, and you're using it at you own risk, which is OK. I think you understand the consequences of this choice, but what do you think a non-security-expert developer should do?

Just to be clear, I don't think I'm actually going to be using BitFlags::all for the final version of my PR. While I am concerned about others being able to keep track of the landlock version should I mysteriously disappear, there's too much risk in using BitFlags::all if a non-fs feature was introduced (like networking restrictions). Even for something like AccessFs::all, which I'd be more likely to use, it's not predictable enough for me personally.

So getting rid of it is fine by me. If you wanted something to be "use at your own risk" marking it as unsafe could work, but since nobody ever reads documentation and getting a good grasp of the issue takes some time, it's probably better to just remove it.

Right now, I'd like to improve the documentation for "most developers", hence to avoid pitfalls by default. What do you think about the documentation update?

I'm still not a huge fan of calling this undefined behavior, since at least in my mind at least this is reserved for something relatively specific, but generally I think this does improve things.

[l0kod]

I updated the doc, there is no mention of "undefined behavior" anymore. I tried to better explain the impact of using all available bits.

[dburgener]

The new update seems better to me from a technical description perspective. I left a few comments on minor grammatical issues to try to make it a smoother read.

[dburgener]

"because we cannot know what the effect on your application of an unknown restriction would be" seems more grammatically smooth to me.

[dburgener]

Probably "when handling an untested..." is better

[l0kod]

I'm not sure what you mean here, but I updated the doc. Please let me know what you think.

[dburgener]

I had envisioned keeping the order as is and just adding the word "when", ie "Because we cannot know what the effect of an unknown restriction on your application would be when handling an untested landlock access right...".

The way you updated it is fine, but feels a little awkward stylistically to me.