C++ifying `Core::System::pledge()` and `Core::System::unveil()`

[bugreport0]

Background

It's a joy to see LibMain and TRY() greatly simplifying the main function of SerenityOS components like in 6b862d506359e5eff6cc155ab18350c62c7ab06c. One thing that stands out though is the barebones C-syscall wrapping of Core::System::pledge() and Core::System::unveil().

Given the Core::System namespace you'd expect these functions to offer a typed C++ interface to the underlying system calls. Such a 'proper' C++ interface would allow for compile-time checking, finding exact argument usage and automated code indexing and refactoring.

Maybe someone's already on this, but here's a few ideas that cross my mind.

1. pledge() symbols

Make the Core::System namespace define specific enums or macros for each possible pledge promise. Advantages:

• compile-time checking of promises
• exact users of each pledge promise can be found instantly (imagine finding all users of the stdio pledge in the codebase)

This would make code look something like:

// enumeration style
TRY(Core::System::pledge(kPromise_STDIO | kPromise_EXEC));

// macro style
TRY(Core::System::pledge(PROMISE_STDIO | PROMISE_EXEC));

// struct style
TRY(Core::System::pledge({ .stdio = true, .exec = true }));

2. unveil() symbols

Make the Core::System namespace define specific enums or macros for each possible unveil permission. Advantages:

• compile-time checking of permissions
• exact users of each unveil permission can be found instantly (imagine finding all c permission uses in the codebase)

This would make code look something like:

// enumeration style
TRY(Core::System::unveil("/tmp/tmpfile", kUnveil_Read | kUnveil_Write));

// macro style
TRY(Core::System::unveil("/tmp/tmpfile", UNVEIL_READ | UNVEIL_WRITE));

// struct style
TRY(Core::System::unveil("/tmp/tmpfile", { .read = true, .write = true }));

3. unveil() auto-commit

Instead of forcing unveil(nullptr, nullptr) into every program, serenity_main could commit permissions automatically if any Core::System::unveil() call was made in serenity_main. Programs that want to unveil() at a later moment can then still unveil later.

4. unveil_commit() function

In a similar vein, forcing programmers to unveil(nullptr, nullptr) at all seems silly. Why not introduce a Core::System::unveilCommit() function that commits unveil permissions without having to pass nullptrs.

[hikilaka]

I like the idea behind this issue, and I agree that this functionality is just a wrapper behind the "C" APIs.

[RobertBendun]

Maybe silly idea but something like builder pattern maybe will be nice (due to for example . exploration of available options in IDE) For example:

auto p1 = pladge
  .with_stdio()
  .with_exec()
  .commit();
 
auto p2 = p1
   .without_exec()
   .commit();

Maybe even some DSL with operator overloading, but this sounds too silly for this simple feature.

[bgianfo]

The string API for pledge was a conscious choice by the OpenBSD folks AFAIK. I think the goal was that on every subsequent pledge, you explicitly state the remaining pledges, so it’s almost impossible to get it wrong. Using enums, or other API designs tend to hide stuff behind abstraction and thus it’s not as in your face as you might want for a security feature?

Not saying there is nothing we can do to make them more c++ friendly, but I think there is some good reasoning for the current design.

[bugreport0]

On pledge()

The string API for pledge was a conscious choice by the OpenBSD folks AFAIK. I think the goal was that on every subsequent pledge, you explicitly state the remaining pledges, so it’s almost impossible to get it wrong. Using enums, or other API designs tend to hide stuff behind abstraction and thus it’s not as in your face as you might want for a security feature?

For a C API pledge() makes tons of sense. A very brief and dense syntax for handing over of a set of permission groups. It's great! The perfect building block for a richer 'Core' API on top of it to expand the semantic value of SerenityOS code using it.

A Core API should not hide pledge()s simplicity, but build on top of it to enforce compile-time checking of parameters, keeping the syscall tried and tested. A richer API could also uncover run-time problems at a more semantic and granular level.

Consider a two-function pledge() and revoke() API like:

Core::System::pledge( { PROMISE_IOCTL, PROMISE_EXEC } );
Core::System::revoke( { PROMISE_STDIO } );

You can't compile with a typo in a promise, leading to richer source code. And the revoke() function makes it clearer to see which privilege you're dropping instead of hunting down two lists of strings. revoke() can also give a descriptive error like Trying to drop non-pledge()d PROMISE_STDIO at runtime. A backtrace would pinpoint exactly which promise you're revoke()ing and where.

revoke() could return the new number of pledges to ensure run-time conformance to expected promises by assert()ing the promise count:

Core::System::pledge( { PROMISE_STDIO, PROMISE_IOCTL, PROMISE_EXEC } );
//
//  ... code using ioctl() here ...
//
assert(2 == Core::System::revoke( { PROMISE_IOCTL } ));

This would make promise management DRY and make sure no superfluous promises leak through.

Promise count expectation asserts could even be forced into a revokeExpect() function while keeping the unchecked API:

// expected behavior
Core::System::revokeExpect( 2, { PROMISE_IOCTL } );

// run-time error: new promise count (2) differs from expected count (3)
Core::System::revokeExpect( 3, { PROMISE_IOCTL } );

On unveil()

My initial comment about auto-commit for unveil() both makes sense and doesn't. As serenity_main() never returns until program termination my suggested mechanism doesn't work.

Which leads to the question: should LibMain query applications and set up initial unveils?

Taking it further

Should an application have a plaintext, source-controlled 'manifest' / Info.plist with initial promises and unveiled paths that LibMain can pick up on?

[bgianfo]

A richer API could also uncover run-time problems at a more semantic and granular level.

Sure, they all sound like potentially interesting ideas. Folks normally just send out PRs for this kind of stuff (new API + a few users of the API, and maybe some tests) in Serenity.

I think coding up some minimal prototype and sending that out for feedback will:

• Tell us how this would really work and if we like it.
• Give folks a strawman that we can tweak and iterate on.
• Provide you with more concrete feedback on your designs.

Taking it further Should an application have a plaintext, source-controlled 'manifest' / Info.plist with initial promises and unveiled paths that LibMain can pick up on?

Personally, I'm not as excited about having LibMain setup unveils or the info.plist idea... IMHO code should be in code, in your face, not hidden away.

[bugreport0]

Working on a draft API I found a redundant pledge() in passwd (#11207).

This leads me to believe that a proper Core::System::pledge() API would allow only a single pledge() call to set up initial promises and subsequently only allow Core::System::revoke() with a non-empty set of revoked promises.

But if we head into that territory, the initial Core::System::pledge() promises could easily be supplied to, and be set by, LibMain.

[alimpfard]

The reason we currently don't have the equivalent of the revoke() you're proposing is to make reasoning about the current state of pledges trivial (last pledge is the current state, vs all pledges from start). I remember at least one attempt at a similar API to revoke() (pledge("-foo -bar")), which is where this line of reasoning came to be (I think)

Furthermore, I really don't see why having LibMain set pledges up is desirable.

[bugreport0]

@alimpfard: noted. I just created #11209 before reading your comment to illustrate what this would look like in actual code. Feel free to close everything (#11140 and #11209) if this is a fruitless avenue. I believe #11207 is still relevant.

[alimpfard]

I'm personally not particularly averse to that, I was just pointing out a reason to keep all active pledges available at once as opposed to having incremental changes to that set - I'm not going to just close it without further discussion :P

[bugreport0]

As for LibMain setting up promises: on a conceptual level (for me) it makes more sense for the operating system to enforce metadata-level security properties of processes instead of having the process govern itself.

If an application can externally indicate which promises cover its maximum scope of operation even before being executed, that would allow the OS to restrict its capabilities even earlier and further than we do now.

This could even prevent you from running binaries without any pre-set promises, or gate those behind a security dialog akin to newer operating systems: 'You are about to run an application without security restrictions. This application can potentially read, change, transmit and remove all your files. Are you sure?'.

[alimpfard]

Is that even a useful thing? all applications can potentially read, change, transmit and remove all files regardless of pledge status, pledge is a defensive mechanism for a program against a malicious attacker, not a mechanism for the user against a malicious program.

[bugreport0]

Regarding the idea of the API internals needing to be simple and auditable, I agree. I believe a higher level API can be implemented in a provably safe and secure way, especially with the TRY() pattern being available for out-of-memory situations. Here's a bit of an attempt.

For 'new' Core::System::pledge():

• if Core::System::pledge() was already called: 'Error: pledge() can only be called once per process'
• store that pledge() has been called for the process
• loop through provided promises in the Span
• if individual promise is already in active set of promises: 'Error: promise X was already pledge()d'
• else add promise to set of active promises
• build a promises String and syscall pledge()
• if out of memory: 'Error: out of memory while adding a pledge() promise'

For Core::System::revoke():

• if Core::System::pledge() wasn't called yet: 'Error: pledge() wasn't called for this process yet'
• if empty Span of promises: 'Error: revoke() must be called with at least one promise'
• loop through provided promises in the Span
• if individual promise is not in active set of promises: 'Error: trying to revoke() non-pledge()d promise X'
• else remove promise from active set of promises
• build a promises String and syscall pledge()
• if out of memory: 'Error: out of memory while revoke()ing a promise'

[bugreport0]

Is that even a useful thing? all applications can potentially read, change, transmit and remove all files regardless of pledge status, pledge is a defensive mechanism for a program against a malicious attacker, not a mechanism for the user against a malicious program.

Okay, I didn't know! I (naively) thought that a process without unix inet couldn't create sockets to transmit data. And with unveil() I believed you could for instance wall off your home folder so your user files couldn't be read.

I'm pretty sure macOS' recent sandboxing and entitlements do similar things in the security department, but that's somewhat of an even higher-level framework.

[alimpfard]

Indeed a process without inet cannot create a socket, but a malicious program can simply just say "I want inet" in its pledge set - that's entirely up to the program.

[bugreport0]

Indeed a process without inet cannot create a socket, but a malicious program can simply just say "I want inet" in its pledge set - that's entirely up to the program.

Alright, thanks for the feedback. I completely get where you are coming from. And I'm getting way ahead of what pledge and unveil can provide or assure at a higher level. But it can't hurt to keep future design goals in mind when talking about the building blocks.

So in your example you might get a warning the first time you fire up a passwd binary that has inet. But then you go down the rabbit hole of having to digitally sign or acknowledge some binaries to have more permissions by default and before you know it, exec() needs to starts doing DNS lookups and signature validation stalling actual program execution if a validation server goes down.

Let's focus on pledge and unveil first 😄

[bugreport0]

Strong opinion: after #11207 and #11211 I consider the repeated pledge() idiom actively harmful for security audits. I've removed duplicate (redundant) pledges from three programs now, but I have no clue if these point to bugs in the security or that they're merely copy-and-paste errors.

If applications would pledge() once and then revoke() it signals clear programmer intent.

[bugreport0]

Updated my proof of concept in https://github.com/SerenityOS/serenity/pull/11209#issuecomment-991941182 to a fully pledge()-less design, leaving applications with just Core::System::retract().

[bugreport0]

After #11234 I have some more opinionated thoughts: https://github.com/SerenityOS/serenity/pull/11209#issuecomment-992344502

[bugreport0]

One-time ping to @jntrnr and @awesomekling — In idea 1, compiler poison, of https://github.com/SerenityOS/serenity/pull/11209#issuecomment-992344502 I mused how a custom SerenityOS compiler could automate pledge() enforcement. Would it make sense to add automatic runtime/promise safety to the Jakt language and/or API?