SDL: Add option to drop privileges with unveil()/pledge() #1271

bentley · 2019-01-18T10:51:05Z

pledge() and unveil() are two OpenBSD APIs to restrict the capabilities of a program. pledge whitelists the set of syscalls the program can call, while unveil whitelists a view of the filesystem to the program.

pledge can be called repeatedly to reduce the list of allowed syscalls even further. This patch calls pledge twice, initially with a very broad list and finally with "stdio rpath wpath cpath fattr" (for savestates) "drm" (for graphics) "audio" (for audio). The debuggers additionally need "tty" (-d) and "net" (-g). The kernel kills a program that violates its promises. Obviously if that ever happens, the arguments to pledge need to be fixed.

unveil is called once per file or directory to whitelist, with a specified permission (read/write/execute/create), and a final call with null arguments to prevent further whitelisting. Attempts to access anything not unveiled result in ENOENT or EACCES. The kernel’s method of keeping track of these files is somewhat expensive, so typical practice is to call unveil as late as possible with as few files as possible. Here I only pass it the savestates.

Libepoxy is not compatible with these pledges; its deferred dlopen() calls cause the the program to be killed on glDeleteTextures(). I disabled it conditionally in CMake.

I’ve tested normal gameplay, USB gamepad, networked sndio audio, and basic -d usage (read: started program and hit “c”).

Questions:

~~Right now only the base names of the savestates are unveiled, so they only work if the ROM is in $PWD. How do I pass the directory through to the SDL frontend?~~
Any obvious file accesses or syscalls that I missed? Particularly in the debuggers?
Any style issues?

bentley · 2019-01-19T07:46:50Z

New revision, pulling in the savestate path the same way savestate writes do. (Kosher?) unveil works now for a ROM in any location.

endrift · 2019-01-19T08:28:01Z

New commit will fail with other VDir types, like ZIP files.

bentley · 2019-01-19T08:34:44Z

I expected that, but was surprised to find it actually worked for zip files.

endrift · 2019-01-19T08:36:11Z

That's not reliable though, it just happens to be that the struct elements were in the right order for that to work out.

bentley · 2019-01-19T08:44:06Z

Bummer… reverted. So hooking the VDir creation routines will be the next step.

bentley · 2020-05-23T11:04:43Z

Clearly I’m not going to get around to finishing unveil support soon… however, the pledge support works well, and has been part of the OpenBSD package for over a year, including the last three OpenBSD releases. So let’s reduce the scope of this pull request to pledge’s syscall filtering, which is ready to go in. What are your thoughts?

endrift · 2020-05-23T11:06:44Z

I'll take a more in-depth look tomorrow (it's 4 AM here, I shouldn't still be awake) but you should probably start by restricting the status output and checks in CMake to just being on OSes that have it (OpenBSD and...SerenityOS I think?)

bentley · 2020-05-23T11:20:14Z

I think SerenityOS shows up as “Generic” in CMake and isn’t otherwise detectable, but I don’t know for sure. Because of that I only gated pledge() support behind a variable that defaults to off. I can change that to if (CMAKE_SYSTEM_NAME STREQUAL OpenBSD) if you want though.

CMakeLists.txt

endrift · 2020-07-10T06:30:07Z

src/platform/sdl/main.c

@@ -149,6 +154,15 @@ int main(int argc, char** argv) {
 	renderer.player.bindings = &renderer.core->inputMap;
 	mSDLInitBindingsGBA(&renderer.core->inputMap);
 	mSDLInitEvents(&renderer.events);
+
+#ifdef USE_PLEDGE
+	if (!mPledgeBroad(&args)) {


Is there any reason this isn't done immediately after generating the args struct? There's a lot of stuff you're not freeing or deiniting here.

Joystick initialization calls some USB ioctls that are not allowed by any pledge, so this is as early as the pledge call can go.

I gave this another try.

src/platform/sdl/main.c

endrift · 2020-07-10T06:35:00Z

src/platform/sdl/main.c

@@ -264,6 +278,12 @@ int mSDLRun(struct mSDLRenderer* renderer, struct mArguments* args) {
 					state->close(state);
 				}
 			}
+#ifdef USE_PLEDGE
+		if (!mPledgeNarrow(args)) {


What's the purpose of this second, narrower call? To drop further privs after everything is initialized? It looks like all you drop is dns and unix, and I'm not sure what those are used for in the first place. SDL I guess. Note that the runloop will still get called if this fails.

Also the indentation doesn't match.

Yes, that’s the purpose—pledge is intended to ratchet down permissions before entering the main loop.

dns and unix are needed for audio:

unix is needed when the device is a local sndiod(8) sub-device.

inet and dns are needed when the device is a remote sndiod(8) sub-device.

Once no further calls to sio_open() will be made, all these pledge(2) promises may be dropped, except for the audio promise.

Is the right thing here to return didFail right away and let the caller deinit everything?

Edit:

Narrow pledge() failed Segmentation fault (core dumped)

Apparently not.

Everything after the runloop except for mCoreThreadHasCrashed still needs to be called.

src/platform/sdl/main.c

endrift

Pretty close this time. Just some minor stuff and one less minor thing.

endrift · 2020-07-10T07:21:04Z

src/platform/sdl/main.c

@@ -264,6 +278,12 @@ int mSDLRun(struct mSDLRenderer* renderer, struct mArguments* args) {
 					state->close(state);
 				}
 			}
+#ifdef USE_PLEDGE
+		if (!mPledgeNarrow(args)) {


Everything after the runloop except for mCoreThreadHasCrashed still needs to be called.

endrift · 2020-07-10T07:43:21Z

CMakeLists.txt

@@ -489,6 +496,10 @@ find_feature(USE_SQLITE3 "sqlite3")
 find_feature(USE_ELF "libelf")
 find_feature(ENABLE_PYTHON "PythonLibs")

+if(USE_PLEDGE)


You forgot to delete it here.

endrift · 2020-07-10T07:45:05Z

src/platform/sdl/main.c

+		renderer.core->deinit(renderer.core);
+		mSDLDeinitEvents(&renderer.events);
+		mSDLDeinit(&renderer);
+		fputs("Broad pledge() failed\n", stderr);


fputs already appends the \n.

endrift · 2020-07-10T07:47:37Z

src/platform/sdl/main.c

+		mCoreConfigDeinit(&renderer.core->config);
+		mInputMapDeinit(&renderer.core->inputMap);
+		renderer.core->deinit(renderer.core);
+		mSDLDeinitEvents(&renderer.events);


mSDLDeinitEvents is called by mSDLDeinit, you don't need to call it yourself. mCoreConfigFreeOpts should be called though.

endrift · 2020-07-10T07:48:02Z

src/platform/sdl/main.c

+#ifdef USE_PLEDGE
+			if (!mPledgeNarrow(args)) {
+				didFail = true;
+				fputs("Narrow pledge() failed\n", stderr);


Ditto about the \n.

bentley force-pushed the pledge-unveil branch 2 times, most recently from 8ba3c7b to e74e5de Compare January 19, 2019 07:43

bentley force-pushed the pledge-unveil branch from e74e5de to 9a6bab0 Compare January 19, 2019 08:41

bentley force-pushed the pledge-unveil branch from 9a6bab0 to 1bee3c6 Compare May 23, 2020 11:00

bentley force-pushed the pledge-unveil branch from 1bee3c6 to 4015177 Compare May 23, 2020 11:06

endrift requested changes Jul 10, 2020

View reviewed changes

bentley added 5 commits July 10, 2020 01:27

SDL: Add option to drop privileges with pledge()

85baac6

CMake: Hide pledge() support behind an OpenBSD check

c2ef2a6

CMake: Rearrange epoxy disable

7f7ea7c

CMake: Don't print pledge configuration in status output

0387a8e

SDL: Revamp pledge() error handling

585d55a

bentley force-pushed the pledge-unveil branch from cbc0ada to 585d55a Compare July 10, 2020 07:41

endrift requested changes Jul 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDL: Add option to drop privileges with unveil()/pledge() #1271

SDL: Add option to drop privileges with unveil()/pledge() #1271

bentley commented Jan 18, 2019 •

edited

Loading

bentley commented Jan 19, 2019

endrift commented Jan 19, 2019

bentley commented Jan 19, 2019 •

edited

Loading

endrift commented Jan 19, 2019

bentley commented Jan 19, 2019

bentley commented May 23, 2020 •

edited

Loading

endrift commented May 23, 2020

bentley commented May 23, 2020

endrift Jul 10, 2020

bentley Jul 10, 2020

bentley Jul 10, 2020

endrift Jul 10, 2020

bentley Jul 10, 2020

bentley Jul 10, 2020 •

edited

Loading

endrift Jul 10, 2020

endrift left a comment

endrift Jul 10, 2020

endrift Jul 10, 2020

endrift Jul 10, 2020

endrift Jul 10, 2020

endrift Jul 10, 2020

SDL: Add option to drop privileges with unveil()/pledge() #1271

Are you sure you want to change the base?

SDL: Add option to drop privileges with unveil()/pledge() #1271

Conversation

bentley commented Jan 18, 2019 • edited Loading

bentley commented Jan 19, 2019

endrift commented Jan 19, 2019

bentley commented Jan 19, 2019 • edited Loading

endrift commented Jan 19, 2019

bentley commented Jan 19, 2019

bentley commented May 23, 2020 • edited Loading

endrift commented May 23, 2020

bentley commented May 23, 2020

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

bentley Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endrift left a 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

bentley commented Jan 18, 2019 •

edited

Loading

bentley commented Jan 19, 2019 •

edited

Loading

bentley commented May 23, 2020 •

edited

Loading

bentley Jul 10, 2020 •

edited

Loading