Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preventing path traversal by plugins? #3194

Closed
cbiffle opened this issue Feb 12, 2024 · 10 comments · Fixed by #3195
Closed

Preventing path traversal by plugins? #3194

cbiffle opened this issue Feb 12, 2024 · 10 comments · Fixed by #3195
Labels
⚙️ WASM WASM plugins

Comments

@cbiffle
Copy link

cbiffle commented Feb 12, 2024

Hi! sqlc looks neat and I'm playing around with it.

The docs make this statement:

WASM plugins are fully sandboxed; they do not have access to the network, filesystem, or environment variables.

This caused me to look into how filesystem writes are performed. Am I misinterpreting the code, or does the use of path.Join allow for path traversal attacks? e.g. if a plugin emits a path beginning with ../../../. I don't speak any Go, but my read of the Clean function (which appears to be applied automatically to the result of Join) is that it only normalizes .. inside a path and preserves it at the front.

Apologies in advance if I've missed the defense against this.

@kyleconroy kyleconroy added the ⚙️ WASM WASM plugins label Feb 12, 2024
@kyleconroy
Copy link
Collaborator

Excellent question! As the documentation states, WASM plugins do not have any filesystem access. The output of a plugin is specified in the out parameter. I need to investigate if plugin output can escape that directory using "..".

@jgallagher
Copy link

jgallagher commented Feb 12, 2024

@cbiffle and I were chatting about this before he filed the issue, and I was able to confirm they can (as he was writing it up!). I applied this diff to the Rust plugin demo:

--- a/src/main.rs
+++ b/src/main.rs
@@ -21,7 +21,7 @@ pub fn serialize_codegen_response(resp: &plugin::CodeGenResponse) -> Vec<u8> {

 pub fn create_codegen_response() -> plugin::CodeGenResponse {
     let mut file = plugin::File::default();
-    file.name = "hello.txt".to_string();
+    file.name = "../../../../../../../../../tmp/escaped.txt".to_string();
     file.contents = "Hello World".as_bytes().to_vec();

     let mut resp = plugin::CodeGenResponse::default();

and after running it:

% cat /tmp/escaped.txt
Hello World

@cbiffle
Copy link
Author

cbiffle commented Feb 12, 2024

WASM plugins do not have any filesystem access

It appears that they don't have filesystem access in the WASI sense, but that's a narrower statement. FWIW I also can't work out how to read the filesystem from a plugin, only write it, so, that's good.

@kyleconroy
Copy link
Collaborator

@jgallagher that looks conclusive to me. I'll get a PR up to fix the behavior. This could be a breaking change, so I'll need to decide if it can write to locations next to and under sqlc.json or the specified out directory.

Are you using sqlc at Oxide?

@cbiffle
Copy link
Author

cbiffle commented Feb 12, 2024

Assuming this gets fixed, I might suggest rephrasing that statement to "plugins can only write files into their output directory," since "no access to the filesystem" suggests to me that they couldn't, say, deposit malware to it or fill it up -- and to be useful plugins the writing part is important!

Are you using sqlc at Oxide?

Not currently, but John and I both think it's a really neat idea.

@kyleconroy
Copy link
Collaborator

Yeah, it's a bit confusing to say that the plugins can write files, as they can't due to the sandbox. sqlc takes the output from the plugin and uses that to write the output files. I'm sure we can figure out some language that makes it clear what's happening.

@cbiffle
Copy link
Author

cbiffle commented Feb 12, 2024

nod Something like "plugins can send commands to sqlc to have files created in the output directory, but can't directly access the filesystem"

@kyleconroy
Copy link
Collaborator

This doesn't include the necessary tests yet, but I believe #3195 should fix the issue.

@kyleconroy
Copy link
Collaborator

Alright, this should be fixed on main. Can you build sqlc from main and confirm your plugin now generates an error?

@jgallagher
Copy link

Confirmed:

% sqlc-dev generate
# package sqlc-gen-rust
error generating code: invalid file output path: /tmp/escaped.txt

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ WASM WASM plugins
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants