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

Don't Canonicalize Paths #920

Closed
Alexhuszagh opened this issue Jul 7, 2022 · 2 comments · Fixed by #942
Closed

Don't Canonicalize Paths #920

Alexhuszagh opened this issue Jul 7, 2022 · 2 comments · Fixed by #942
Assignees
Labels
Milestone

Comments

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jul 7, 2022

Path canonicalization is generally a good idea for security reasons, and we should do it for the uniqueness of our hashes, but it can affects crates that wish to mount volumes that are symlinked and get data specifically using relative paths or absolute paths to the volume, which can differ from the canonical path.

Say my crate does something like:

main.rs

pub fn main() {
    println!("{}", std::fs::read_to_string(std::path::Path::new("/tmp/config.toml")).unwrap());
}

Now, /tmp/config.toml is on macOS, which is actually mounted at /private/tmp/config.toml, so canonicalizing the path will mean /tmp is not mounted, but /private/tmp is. So in this case, we have a path where the parent directory is a symlink and this code will now fail.

@Alexhuszagh Alexhuszagh added the bug label Jul 7, 2022
@Alexhuszagh Alexhuszagh self-assigned this Jul 7, 2022
@Emilgardis
Copy link
Member

Emilgardis commented Jul 7, 2022

I'm not sure I follow the bug description. How is this a bug in cross?

@Emilgardis
Copy link
Member

The issue is that symlinks through canonicalization can destroy assumptions about paths.

Ideally, we'd have to tell the users of cross to always use the env var for mounts, but realistically this breaks on assumptions like the shown example for macOS

@Alexhuszagh Alexhuszagh added this to the v0.3.0 milestone Jul 9, 2022
Alexhuszagh added a commit to Alexhuszagh/cross that referenced this issue Jul 12, 2022
Symlinks and canonical paths can destroy assumptions about paths: say I have a file at `/tmp/path/to/file` I reference in my crate, and ask to mount `/tmp/path/to` on the container. Currently, on macOS, since `/tmp` is a symlink to `/private/tmp`, the code that expects `/tmp/path/to/file` will fail because in the container it will be mounted at `/private/tmp/path/to/file`.

Also fix device_ns parsing with drive letters. Fixes a minor bug where the host root instead of the var names was passed as the envvar.

Closes cross-rs#920.
bors bot added a commit that referenced this issue Jul 14, 2022
947: Resolve symlinks for cargo and xargo home. r=Emilgardis a=Alexhuszagh

Resolve symlinks for the xargo and cargo home (as well as the Nix store) prior to mounting, since they are mounted at a fixed location anyway. This is because podman mounts symlinks as root by default.

Closes #373.
Doesn't interfere with #920 and #942, although #942 might need work to ensure non-canonical paths are mounted with normal permissions.

Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>
@bors bors bot closed this as completed in 59ec530 Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants