Unveil: Accept relative paths #1474
Conversation
|
I'm not sure it makes sense for the |
muscar
force-pushed
the
dek20-unveil-accept-relative-paths
branch
from
59e9c21
to
c5aefa4
Compare
|
@awesomekling that was just me being slightly too trigger happy with the changes. Agreed |
|
@awesomekling It actually doesn't :) The check for an absolute path is still in place, and it comes before the call to resolve the path. It's just my PR description that is misleading. Will update. |
muscar
force-pushed
the
dek20-unveil-accept-relative-paths
branch
from
c5aefa4
to
d277d57
Compare
| @@ -4772,6 +4772,14 @@ int Process::sys$unveil(const Syscall::SC_unveil_params* user_params) | |||
| if (path.value().is_empty() || path.value().characters()[0] != '/') | |||
| return -EINVAL; | |||
|
|
|||
| auto custody_or_error = VFS::the().resolve_path_without_veil(path.value(), root_directory()); | |||
There was a problem hiding this comment.
Hmm, I'm unsure what to think of this change.
With this change, we will now follow symbolic links when unveiling a path.
Previously, we would capture paths verbatim and stick with that.
The current implementation already has various other issues wrt symlinks, but what is the rationale for supporting symlinks here?
There was a problem hiding this comment.
@awesomekling That's what OpenBSD does AFAICT (by looking at the source code and using a quick test program).
If I unveil a file I would expect a symlink to it to be unveiled as well.
Here's the test program I used in OpenBSD: https://clbin.com/DAS0M.
muscar
force-pushed
the
dek20-unveil-accept-relative-paths
branch
from
d277d57
to
26a6e63
Compare
There was a problem hiding this comment.
Hmm, I don't understand how this addresses #1260
That bug is specifically about unveiling e.g "/etc/passwd" and then still being unable to open something like "../../etc/passwd" from "/home/anon"
| if (custody_or_error.is_error()) | ||
| // FIXME Should this be EINVAL? | ||
| return custody_or_error.error(); |
There was a problem hiding this comment.
Please use { and } whenever the contents of a block spans multiple lines, even if some of them are comments.
|
@awesomekling Using this test program (very similar to the OpenBSD one) https://clbin.com/dt1pp it works exactly as you described. It works by moving the path check from before resolving it to after resolving it to an absolute path. |
|
|
|
Ah, I misunderstood how the logic would flow after this patch. Thanks for explaining with a picture and everything :) |
This splits
VFS::resolve_path()into a helper (resolve_path_without_unveil()) which does the actual lookup without checking the unveiled paths, and a wrapperresolve_path()that calls the helper, and then checks the unveiled paths.(Addresses #1260)