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

package fetching: detect and fail when symlinks point outside the package root #17461

Open
andrewrk opened this issue Oct 9, 2023 · 0 comments
Labels
bug Observed behavior contradicts documented or intended behavior enhancement Solving this issue will likely involve adding new logic or components to the codebase. zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Oct 9, 2023

Extracted from #17392.

This issue is to eliminate this TODO comment:

zig/src/Package/Fetch.zig

Lines 1195 to 1207 in f7bc55c

.sym_link => {
var buf: [fs.MAX_PATH_BYTES]u8 = undefined;
const link_name = try dir.dir.readLink(entry.path, &buf);
// TODO: if this would create a symlink to outside
// the destination directory, fail with an error instead.
tmp_dir.symLink(link_name, entry.path, .{}) catch |err| switch (err) {
error.FileNotFound => {
if (fs.path.dirname(entry.path)) |dirname| try tmp_dir.makePath(dirname);
try tmp_dir.symLink(link_name, entry.path, .{});
},
else => |e| return e,
};
},

I was thinking about this more, and one strategy regarding symbolic links would be to not police what goes in them, since it could be an arbitrary string. These arbitrary strings do not necessarily reach outside of the package contents; they could be interpreted in any way. It could be considered the responsibility of other software, such as the compiler or build system - which would include user logic - to make sure that files within a package don't reach outside themselves into the file system.

However, I think that it may be better to look for relative paths inside symbolic links that reach outside their containing package and treat those as file system paths, and make those become errors, for these reasons:

  • Symbolic link support is different per operating system, and potentially file system. They are certainly intended to semantically correspond to file system paths, and the ability to represent arbitrary strings may be an extra feature that is not available on some targets. Limiting the kinds of file system shenanigans that are allowed in a Zig package serves to make them more portable.
  • Relying on user logic to prevent symlinks from reaching outside the package paths is not really a good idea. It's barely even a good idea to rely on the Zig compiler being correct about it.

So I think the TODO comment is correct. Package fetching code should detect when a symlink would point to outside the package, and make the fetch fail with an error.

The linked code points to tarball unpacking, but in order to close this issue, the same protection needs to be implemented in the other kinds of unpacking as well:

  • Recursive directory copying
  • Tarballs
  • git

All future kinds of unpacking, such as #17408, will need the same logic.

Finally, this needs to integrate with #17460. Any bad symlinks that get excluded by the inclusion rules should not trigger a fetch failure, since they are not included in the package after all.

@andrewrk andrewrk added bug Observed behavior contradicts documented or intended behavior enhancement Solving this issue will likely involve adding new logic or components to the codebase. zig build system std.Build, the build runner, `zig build` subcommand, package management labels Oct 9, 2023
@andrewrk andrewrk added this to the 0.13.0 milestone Oct 9, 2023
@andrewrk andrewrk mentioned this issue Oct 9, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior enhancement Solving this issue will likely involve adding new logic or components to the codebase. zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

No branches or pull requests

1 participant