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

Likely unnecessary write access to a file #438

Closed
3Hren opened this issue May 13, 2019 · 3 comments · Fixed by #449
Closed

Likely unnecessary write access to a file #438

3Hren opened this issue May 13, 2019 · 3 comments · Fixed by #449
Labels
bug Something isn't working

Comments

@3Hren
Copy link

3Hren commented May 13, 2019

Hi!

While playing with wasmer as an embeddable wasm execution engine I discovered that wasmer-wasi requests more wide file-access than required in the code during opening a file.

For example:

let file = File::open("/proc/uptime")?;

fails without root permissions, while it shouldn't.

Stracing resulted in:

[pid 18721] openat(AT_FDCWD, "./uptime", O_RDWR|O_CLOEXEC = -1 EACCES (Permission denied)

In pure Rust, this opens /proc/uptime in O_RDONLY mode. In the case of wasmer-wasi it also requests write-access (O_RDWR). Here is the suspected code:

let open_options = open_options.read(true).write(true);

Is this a bug or I'm doing something wrong?

PS. I preopened /proc and even changed the working directory to it. However, it is quite strange for me why the full open path is stripped to just ./uptime.

OS: linux 5.0.3, x86_64 x86_64 GNU/Linux
Version = 0.4.1

@3Hren 3Hren added the bug Something isn't working label May 13, 2019
@MarkMcCaskey
Copy link
Contributor

MarkMcCaskey commented May 13, 2019

Thanks for reporting this!

The current file abstraction was never intended to last long-term and it's showing a lot of issues -- things like this and working portably across Unix and Windows. The location of the code where the file is being opened here doesn't know whether the file will need to be written to or not. The reason it's even being opened here is that I was trying to be conservative in regards to the file system changing during program execution but it seems that's not something we can reasonably handle right now.

It seems a completely abstracted WASI file system with security checks on every interaction with the host file system is the way to go to fix things like this

edit: removed the edit that was for a different comment

@MarkMcCaskey
Copy link
Contributor

So it turns out I overestimated the scope of the problem!

We can fix it without updating our abstractions!

Thanks for reporting this!

bors bot added a commit that referenced this issue May 15, 2019
449: avoid opening files when not needed in WASI, check for write permissions r=MarkMcCaskey a=MarkMcCaskey

resolves #438 

Follow up to  #448.  Turns out we don't have to complect things in that way, we can just be more conservative about opening files and granting write permissions

Co-authored-by: Mark McCaskey <mark@wasmer.io>
@bors bors bot closed this as completed in #449 May 15, 2019
@3Hren
Copy link
Author

3Hren commented May 16, 2019

Awesome! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants