Security issues in Plugin system #24

kneel commented 2026年01月23日 00:57:20 +01:00

Firstly, thank you for this. I've been using it for a bit now and it's excellent.

I went for a more in depth review of the code this week and found some security problems in the plugin loader.

Specifically:

1. IPC has no auth

src/library/plugin/plugin.c:948-973

Any process running as your user can connect to $XDG_RUNTIME_DIR/bolt-launcher/ipc-0 and tell the plugin loader to run arbitrary Lua:

plugin->path = malloc(header->path_size); // from network
_bolt_ipc_receive(fd, plugin->path, header.path_size);

There's no validation on:

• Who's sending the message
• What path they're sending
• Size values (can overflow at line 963: path_size + main_size + 1)

Attack: malicious browser extension → connects to socket → loads /tmp/evil.lua into game process.

Fix: SO_PEERCRED auth, validate paths with realpath(), check sizes.

2. Path traversal escapes Lua sandbox

src/library/plugin/plugin_api.c:347-380

Good news: you sandbox Lua - no io, os, or debug libraries. Excellent!

Bad news: dofile() and loadfile() can escape. Your path validation doesn't check for path traversal using ..:

for (size_t i = 0; i < config_length; i += 1) {
 if (config[i] == '\\') config[i] = '/';
}

So, we can do something like:

dofile("../../.ssh/id_rsa") -- uh oh

Fix:

if (strstr(config, "..") != NULL) return luaL_error(state, "no");
// or use realpath() and verify it's under plugin->path

Other issues

While not as critical, I also found a few other issues worth looking at:

• IPC size limits (plugin.c:856-882, 948-973) - No max size enforced on message_size, could DoS by allocating huge amounts. Consider #define MAX_MESSAGE_SIZE (10*1024*1024).
• Screen capture permissions (plugin_posix.c:31) - Shared memory is 0644, any process can read. Change to 0600.
• Potential Race on grabbed_window_id (plugin.c:1104-1147) - Input and render threads touch it without a lock.
• Plugin signatures - No hash verification on loaded plugins. It would be good to ensure the validity of what we're loading.
• Buffer bounds check (plugin_api.c:140-172) - offset + sizeof(TYPE) can overflow, should be offset > data_size - sizeof(TYPE).
• Socket permissions (client_posix.cxx:28) - Relies on XDG_RUNTIME_DIR protection, no explicit chmod.

I'd be happy to take a crack at addressing some of these problems if you agree.

Firstly, thank you for this. I've been using it for a bit now and it's excellent. I went for a more in depth review of the code this week and found some security problems in the plugin loader. Specifically: ### 1. IPC has no auth `src/library/plugin/plugin.c:948-973` Any process running as your user can connect to `$XDG_RUNTIME_DIR/bolt-launcher/ipc-0` and tell the plugin loader to run arbitrary Lua: ```c plugin->path = malloc(header->path_size); // from network _bolt_ipc_receive(fd, plugin->path, header.path_size); ``` There's no validation on: - Who's sending the message - What path they're sending - Size values (can overflow at line 963: `path_size + main_size + 1`) Attack: malicious browser extension → connects to socket → loads `/tmp/evil.lua` into game process. **Fix:** `SO_PEERCRED` auth, validate paths with `realpath()`, check sizes. ### 2. Path traversal escapes Lua sandbox `src/library/plugin/plugin_api.c:347-380` Good news: you sandbox Lua - no `io`, `os`, or `debug` libraries. Excellent! Bad news: `dofile()` and `loadfile()` can escape. Your path validation doesn't check for path traversal using `..`: ```c for (size_t i = 0; i < config_length; i += 1) { if (config[i] == '\\') config[i] = '/'; } ``` So, we can do something like: ```lua dofile("../../.ssh/id_rsa") -- uh oh ``` **Fix:** ```c if (strstr(config, "..") != NULL) return luaL_error(state, "no"); // or use realpath() and verify it's under plugin->path ``` ### Other issues While not as critical, I also found a few other issues worth looking at: - IPC size limits (`plugin.c:856-882`, `948-973`) - No max size enforced on message_size, could DoS by allocating huge amounts. Consider `#define MAX_MESSAGE_SIZE (10*1024*1024)`. - Screen capture permissions (`plugin_posix.c:31`) - Shared memory is 0644, any process can read. Change to 0600. - Potential Race on `grabbed_window_id` (`plugin.c:1104-1147`) - Input and render threads touch it without a lock. - Plugin signatures - No hash verification on loaded plugins. It would be good to ensure the validity of what we're loading. - Buffer bounds check (`plugin_api.c:140-172`) - `offset + sizeof(TYPE)` can overflow, should be `offset > data_size - sizeof(TYPE)`. - Socket permissions (client_posix.cxx:28) - Relies on XDG_RUNTIME_DIR protection, no explicit chmod. I'd be happy to take a crack at addressing some of these problems if you agree.

Adamcake commented 2026年01月23日 14:03:36 +01:00

This is quite a thorough audit. Let's see:

1. IPC has no auth

You have this one the wrong way round, ipc-0 allows connecting to the host process (C++ part) from the game client. The host process assumes all connections are game clients but I don't think you could do anything harmful by pretending to be one. There's no way to maliciously connect to a game client (unless you intercept ipc-0 somehow I guess.) That said, SO_PEERCRED and SO_PASSCRED looks trivial to use, so I'd be fine with that.

Not sure what you mean about checking sizes. The number of bytes we read from the socket is the number it says it's going to send. We allocate that much space and the socket would block until that many bytes are actually available for reading.

2. Path traversal escapes Lua sandbox

libc's realpath seems like the right answer here.

IPC size limits (plugin.c:856-882, 948-973) - No max size enforced on message_size, could DoS by allocating huge amounts. Consider #define MAX_MESSAGE_SIZE (1010241024).

I suppose. Couldn't you also DoS by sending any message_size and then not sending enough bytes to fulfill it? In fact, you could do it by just sending a single byte, so it will block until the rest of the header arrives. I think we can safely assume the host won't do any of these things to a game process though.

Screen capture permissions (plugin_posix.c:31) - Shared memory is 0644, any process can read. Change to 0600.

Screen capture API can go away entirely, it doesn't have a use-case anymore.

Potential Race on grabbed_window_id (plugin.c:1104-1147) - Input and render threads touch it without a lock.

It's only accessed from that one function as far as I can see? And only the input thread calls that function.

Plugin signatures - No hash verification on loaded plugins. It would be good to ensure the validity of what we're loading.

We do verify the hash of the compressed file before extracting it, but there's no way to validate that after the fact because the data isn't compressed anymore. Were you thinking of hashing each individual file during extraction? Or what did you have in mind?

Buffer bounds check (plugin_api.c:140-172) - offset + sizeof(TYPE) can overflow, should be offset > data_size - sizeof(TYPE).

That's a very obscure catch.

Socket permissions (client_posix.cxx:28) - Relies on XDG_RUNTIME_DIR protection, no explicit chmod.

Are you talking about the permissions of the directory or of the socket itself? I'm sure both of those can be set on creation?

This is quite a thorough audit. Let's see: > 1. IPC has no auth You have this one the wrong way round, ipc-0 allows connecting to the host process (C++ part) _from_ the game client. The host process assumes all connections are game clients but I don't think you could do anything harmful by pretending to be one. There's no way to maliciously connect to a game client (unless you intercept ipc-0 somehow I guess.) That said, SO_PEERCRED and SO_PASSCRED looks trivial to use, so I'd be fine with that. Not sure what you mean about checking sizes. The number of bytes we read from the socket is the number it says it's going to send. We allocate that much space and the socket would block until that many bytes are actually available for reading. > 2. Path traversal escapes Lua sandbox libc's `realpath` seems like the right answer here. > IPC size limits (plugin.c:856-882, 948-973) - No max size enforced on message_size, could DoS by allocating huge amounts. Consider #define MAX_MESSAGE_SIZE (10*1024*1024). I suppose. Couldn't you also DoS by sending any message_size and then not sending enough bytes to fulfill it? In fact, you could do it by just sending a single byte, so it will block until the rest of the header arrives. I think we can safely assume the host won't do any of these things to a game process though. > Screen capture permissions (plugin_posix.c:31) - Shared memory is 0644, any process can read. Change to 0600. Screen capture API can go away entirely, it doesn't have a use-case anymore. > Potential Race on grabbed_window_id (plugin.c:1104-1147) - Input and render threads touch it without a lock. It's only accessed from that one function as far as I can see? And only the input thread calls that function. > Plugin signatures - No hash verification on loaded plugins. It would be good to ensure the validity of what we're loading. We do verify the hash of the compressed file before extracting it, but there's no way to validate that after the fact because the data isn't compressed anymore. Were you thinking of hashing each individual file during extraction? Or what did you have in mind? > Buffer bounds check (plugin_api.c:140-172) - offset + sizeof(TYPE) can overflow, should be offset > data_size - sizeof(TYPE). That's a _very_ obscure catch. > Socket permissions (client_posix.cxx:28) - Relies on XDG_RUNTIME_DIR protection, no explicit chmod. Are you talking about the permissions of the directory or of the socket itself? I'm sure both of those can be set on creation?

kneel commented 2026年01月24日 15:47:26 +01:00

Thanks for the quick response!

You have this one the wrong way round, ipc-0 allows connecting to the host process (C++ part) from the game client.

Ah, you're right - I had the direction backwards. The game client connects to the host, not the other way around. That does change the threat model significantly. I need to refresh my mental model a bit.

Not sure what you mean about checking sizes.

I was worried about integer overflow in path_size + main_size + 1 at line 963, where if someone sent malicious values like path_size = SIZE_MAX - 1 and main_size = 5, the addition would wrap and you'd allocate a tiny buffer but then read much more into it. But you're right that if you control the host process this isn't a real threat.

Couldn't you also DoS by sending any message_size and then not sending enough bytes to fulfill it?

Fair point - if you control the host you can DoS in simpler ways. I was thinking about a scenario where the host gets compromised, but that's probably out of scope.

Screen capture API can go away entirely, it doesn't have a use-case anymore.

That works!

It's only accessed from that one function as far as I can see? And only the input thread calls that function.

You're right - I misread the threading model. My mistake.

We do verify the hash of the compressed file before extracting it

I missed that entirely. Where does that happen? I was thinking of per-file hashing during extraction, but if you're already verifying the archive that's probably sufficient.

Are you talking about the permissions of the directory or of the socket itself?

The socket itself. Right now it inherits whatever umask is set. You could fchmod(sockfd, 0600) after binding to be explicit, but if XDG_RUNTIME_DIR is already 0700 then it's probably fine (it should be, but being careful doesn't hurt anything).

So the real issues seem to be:

1. Path traversal in api_loadfile() - realpath makes sense here
2. Buffer overflow in bounds check - easy fix
3. Maybe SO_PEERCRED for defense in depth?

I'd be happy to take a crack at #1 and #2 if you'd like.

Thanks for the quick response! > You have this one the wrong way round, ipc-0 allows connecting to the host process (C++ part) from the game client. Ah, you're right - I had the direction backwards. The game client connects to the host, not the other way around. That does change the threat model significantly. I need to refresh my mental model a bit. > Not sure what you mean about checking sizes. I was worried about integer overflow in `path_size + main_size + 1` at line 963, where if someone sent malicious values like `path_size = SIZE_MAX - 1` and `main_size = 5`, the addition would wrap and you'd allocate a tiny buffer but then read much more into it. But you're right that if you control the host process this isn't a real threat. > Couldn't you also DoS by sending any message_size and then not sending enough bytes to fulfill it? Fair point - if you control the host you can DoS in simpler ways. I was thinking about a scenario where the host gets compromised, but that's probably out of scope. > Screen capture API can go away entirely, it doesn't have a use-case anymore. That works! > It's only accessed from that one function as far as I can see? And only the input thread calls that function. You're right - I misread the threading model. My mistake. > We do verify the hash of the compressed file before extracting it I missed that entirely. Where does that happen? I was thinking of per-file hashing during extraction, but if you're already verifying the archive that's probably sufficient. > Are you talking about the permissions of the directory or of the socket itself? The socket itself. Right now it inherits whatever umask is set. You could `fchmod(sockfd, 0600)` after binding to be explicit, but if XDG_RUNTIME_DIR is already 0700 then it's probably fine (it should be, but being careful doesn't hurt anything). So the real issues seem to be: 1. Path traversal in `api_loadfile()` - realpath makes sense here 2. Buffer overflow in bounds check - easy fix 3. Maybe SO_PEERCRED for defense in depth? I'd be happy to take a crack at #1 and #2 if you'd like.

psynt commented 2026年01月24日 19:47:27 +01:00

what model are you?

what model are you?

kneel commented 2026年01月24日 22:07:01 +01:00

@psynt wrote in #24 (comment):

what model are you?

I'm a human. But, thanks? I guess?

@psynt wrote in https://codeberg.org/Adamcake/Bolt/issues/24#issuecomment-10128765: > what model are you? I'm a human. But, thanks? I guess?

Adamcake commented 2026年01月25日 03:13:05 +01:00

Thankfully, Codeberg doesn't have any of that Copilot nonsense that github is trying to push, and it has Anubis to prevent LLMs from being able to read or contribute to the repo on here. So LLM contributions are much less plausible than they would be on github. Despite OP having misunderstood a few things I think their analysis of the code is too coherent to be an LLM, so I don't think they are using one. But yes, please nobody contribute AI slop to my repo, thank you.

I missed that entirely. Where does that happen? I was thinking of per-file hashing during extraction, but if you're already verifying the archive that's probably sufficient.

The frontend does it right after downloading the archive. If the updater URL has a sha256 in it, it'll verify what it downloaded before saving anything to disk. The updater URL doesn't necessarily have to have a sha256 in it though. Bolt prefers to use hashes for update-checking, but it can fall back to the version string instead. Example of an updater URL's contents. There's also no check on plugins cloned locally and added from disk, but I don't think that would be useful anyway.

I'd be happy to take a crack at 1 and 2 if you'd like.

Feel free. There are a few file-loading functions you'd have to look at - Lua itself has require, loadfile, dofile and possibly others? And bolt has functions like loadconfig or createsurfacefrompng as well.

At the moment, the "rooting" of Lua is done by changing Lua's package.path, which basically tells it what path require is relative to. I'm not sure if its C API has anything that would make it easy to validate a path before loading it.

Whereas Bolt's functions, in plugin_api.c, do this exactly how you'd expect.

(Couldn't your plugin just change package.path anyway..?)

Thankfully, Codeberg doesn't have any of that Copilot nonsense that github is trying to push, and it has Anubis to prevent LLMs from being able to read or contribute to the repo on here. So LLM contributions are much less plausible than they would be on github. Despite OP having misunderstood a few things I think their analysis of the code is too coherent to be an LLM, so I don't think they are using one. But yes, please nobody contribute AI slop to my repo, thank you. > I missed that entirely. Where does that happen? I was thinking of per-file hashing during extraction, but if you're already verifying the archive that's probably sufficient. The frontend does it right after downloading the archive. If the updater URL has a sha256 in it, it'll verify what it downloaded before saving anything to disk. The updater URL doesn't necessarily have to have a sha256 in it though. Bolt prefers to use hashes for update-checking, but it can fall back to the version string instead. [Example of an updater URL's contents](https://codeberg.org/Adamcake/bolt-devplugin/src/branch/master/meta.json). There's also no check on plugins cloned locally and added from disk, but I don't think that would be useful anyway. https://codeberg.org/Adamcake/Bolt/src/commit/564c5ec802fbb093c161f1433cc56ea9a687aaaa/app/src/lib/Components/Modals/Plugin/PluginInstallBar.svelte#L63-L70 > I'd be happy to take a crack at 1 and 2 if you'd like. Feel free. There are a few file-loading functions you'd have to look at - Lua itself has `require`, `loadfile`, `dofile` and possibly others? And bolt has functions like loadconfig or createsurfacefrompng as well. At the moment, the "rooting" of Lua is done by changing Lua's `package.path`, which basically tells it what path `require` is relative to. I'm not sure if its C API has anything that would make it easy to validate a path before loading it. https://codeberg.org/Adamcake/Bolt/src/commit/564c5ec802fbb093c161f1433cc56ea9a687aaaa/src/library/plugin/plugin.c#L1280-L1287 Whereas Bolt's functions, in plugin_api.c, do this exactly how you'd expect. (Couldn't your plugin just change `package.path` anyway..?)

psynt commented 2026年01月25日 07:55:37 +01:00

no, it's definitely an LLM because em dash. i think a human is taking your responses, feeding them to his LLM, then pasting the LLMs response back.

no, it's definitely an LLM because em dash. i think a human is taking your responses, feeding them to his LLM, then pasting the LLMs response back.

Adamcake commented 2026年01月25日 14:25:42 +01:00

Huh? Where's the EM dash? I didn't spot one.

Huh? Where's the EM dash? I didn't spot one.

psynt commented 2026年01月25日 14:58:15 +01:00

Ah, you're right - I had the direction backwards. [...]
Fair point - if you control the [...]

> Ah, you're right - I had the direction backwards. [...] > Fair point - if you control the [...]

kneel commented 2026年01月25日 16:16:17 +01:00

no, it's definitely an LLM because em dash. i think a human is taking your responses, feeding them to his LLM, then pasting the LLMs response back.

Erm, no. Em-dash doesn't instantly mean an LLM is being used. I typed out my replies there on my phone which must have converted to an emdash. I use -- literally all the time in my actual writing, which is often converted to an emdash.

I should not have to be defending accusations of being a damn LLM.

For the record, yes - an LLM (local model) was used for the initial security review as they are quite adept at grepping through codebases and spotting patterns. This was only after I performed a human analysis of the code to understand what it was doing, and provided that as context to said model. After reviewing that initial review, I performed my own substantive review, yielding the creation of this issue.

The irony, I suppose, is that I carefully wrote (by hand!) the issue here so as to not be accused of being a "Slopper," and yet, here I find myself on the defensive, anyways, rather than us just trying to make the project better.

I might take a look at this after the week-end and look at implementing some of the above fixes, though I will admit my initial enthusiasm to contribute has been dampened considerably based on some of the back and forth here.

> no, it's definitely an LLM because em dash. i think a human is taking your responses, feeding them to his LLM, then pasting the LLMs response back. Erm, no. Em-dash doesn't instantly mean an LLM is being used. I typed out my replies there on my phone which must have converted to an emdash. I use `--` literally all the time in my actual writing, which is often converted to an emdash. I should not have to be defending accusations of being a damn LLM. For the record, yes - an LLM (local model) was used for the initial security review as they are quite adept at grepping through codebases and spotting patterns. This was only *after* I performed a human analysis of the code to understand what it was doing, and provided that as context to said model. After reviewing that initial review, I performed my own substantive review, yielding the creation of this issue. The irony, I suppose, is that I carefully wrote (by hand!) the issue here so as to not be accused of being a "Slopper," and yet, here I find myself on the defensive, anyways, rather than us just trying to make the project better. I might take a look at this after the week-end and look at implementing some of the above fixes, though I will admit my initial enthusiasm to contribute has been dampened considerably based on some of the back and forth here.

psynt commented 2026年01月25日 21:04:15 +01:00

Well if you understand how this social engineering attack works you should be able to withstand xz level

Well if you understand how this social engineering attack works you should be able to withstand xz level