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

Error: failed to extract tar to ...: no symbolic link allowed between "..." and ... #636

Closed
KristoferHansson opened this issue Nov 6, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@KristoferHansson
Copy link

KristoferHansson commented Nov 6, 2023

Using ORAS CLI 1.1.0 with zot on localhost

I'm pushing a folder with some subdirectories with success, but when pulling it I'm getting the following error:
Error: failed to extract tar to /home/abc/mydir: no symbolic link allowed between "mydir" and "/home/abc/mydir/icu/current/pkgdata.inc"

The error relates to a symbolic link to a symbolic link(folder) specific content(file)

The original icu folder looks like this:
drwxr-xr-x 2 abc abc 4096 Jan 16 20XX 55.1
lrwxrwxrwx 1 abc abc 20 Jan 16 20XX Makefile.inc -> current/Makefile.inc
lrwxrwxrwx 1 abc abc 4 Jan 16 20XX current -> 55.1
lrwxrwxrwx 1 abc abc 19 Jan 16 20XX pkgdata.inc -> current/pkgdata.inc

The resulting folder after the failed pull looks like this:
drwxr-xr-x 2 abc abc 4096 Jan 16 20XX 55.1
lrwxrwxrwx 1 abc abc 20 Nov 6 11:16 Makefile.inc -> current/Makefile.inc
lrwxrwxrwx 1 abc abc 4 Nov 6 11:16 current -> 55.1

For some reason, it succeeds in unpacking the Makefile.inc symbolic link but not the pkgdata.inc.

If I rename the symbolic link pkgdata.inc to Pkgdata.inc before pushing, pull works
drwxr-xr-x 2 abc abc 4096 Jan 16 20XX 55.1
lrwxrwxrwx 1 abc abc 20 Nov 6 15:53 Makefile.inc -> current/Makefile.inc
lrwxrwxrwx 1 abc abc 19 Nov 6 15:53 Pkgdata.inc -> current/pkgdata.inc
lrwxrwxrwx 1 abc abc 4 Nov 6 15:53 current -> 55.1

UPDATE:
Tried some more to dial in the issue:
Changing the name of the symbolic link to different non capital letters did not work
Changing the name of the pointed to file to Pkgdata.inc did not work
Modifying the symbolic link pkgdata to point directly to 55.1/pkgdata.inc works
Modifying the Makefile.inc symbolic link to lowercase gave the same error

Current conclusion:
A symbolic link can for some reason in the scenario of symbolic_link->other_symbolic_link/concrete_file not be starting with a lowercase
If the link instead goes directly to the concrete file then it works as expected

@Wwwsylvia
Copy link
Member

Wwwsylvia commented Nov 7, 2023

Hi @KristoferHansson , would you mind sharing the command you ran and what the directory structure looks like?

@KristoferHansson
Copy link
Author

KristoferHansson commented Nov 7, 2023

@Wwwsylvia

Hi, absolutely.
The folder structure looks exactly as described above "The original icu folder looks like this:"
The push command issued in the folder above: oras push localhost:5000/icu:v1 icu
The pull command issued in another folder: oras pull --plain-http localhost:5000/icu:v1

The original folder structure that I tried to push of course contained a lot more but I was able to reproduce the error with only this minimal contents (which also was the actual thing causing the initial issue, aborting the oras pull command)

@wangxiaoxuan273
Copy link
Contributor

wangxiaoxuan273 commented Nov 8, 2023

Hi @KristoferHansson

I'm trying to reproduce the error. There is one thing I need to confirm:

The error message you provided is the following Error: failed to extract tar to /home/abc/mydir: no symbolic link allowed between "mydir" and "/home/abc/mydir/icu/current/pkgdata.inc"

So does the symlink current in the failed pull link to the original current, or the new pulled current? To put it another way:
image

  • Is red arrow or blue arrow correct? Where does current in the pulled directory link to? And is it your intended behavior?
  • Is 55.1 also a symbolic link, or a concrete directory?

@KristoferHansson
Copy link
Author

KristoferHansson commented Nov 8, 2023

@wangxiaoxuan273

55.1 is a concrete directory
The symbolic link current in the resulting folder(extracted location) after the failed pull points to the 55.1 in the extracted location. Hence, the red arrow is correct.

Basically "The original icu folder looks like this" represents the source folder I'm pushing with oras push, the links are still pointing to within the pushed folder, not outside.

Note that it works well with a few modifications to the source folder before pushing, symbolic link naming with capital letter or removing the intermediate symbolic link "current".

Update:
Reproduced the same issue with a completely new folder with same contents.

mkdir test
cd test
mkdir 55.1
touch 55.1/File.inc
touch 55.1/file2.inc
ln -s 55.1 current
ln -s current/File.inc File.inc
ln -s current/file2.inc file2.inc
cd ..
oras push localhost:5000/test:v1 test

mkdir tmp
oras pull --plain-http 127.0.0.1:5000/test:v1 --verbose -o tmp

Error: failed to extract tar to /home/abc/tmp/test: no symbolic link allowed between "test" and "/home/abc/tmp/test/current/file2.inc"

@wangxiaoxuan273
Copy link
Contributor

wangxiaoxuan273 commented Nov 9, 2023

@KristoferHansson

Thank you for your reply. I was able to reproduce the issue with the code you gave above. I'll update more information after investigation.

@wangxiaoxuan273
Copy link
Contributor

wangxiaoxuan273 commented Nov 9, 2023

Current investigation indicates that the error happens in the dependency oras.go. Likely this code didn't consider the scenario that part of the path may be a symlink. More investigation will follow.

I was able to reproduce the issue with oras-cli 1.1 and an Azure Container Registry, with the following minimal folder content:

drwxr-xr-x 2 root root 4096 Nov  9 17:11 55.1
lrwxrwxrwx 1 root root    4 Nov  9 13:45 current -> 55.1
lrwxrwxrwx 1 root root   17 Nov  9 13:45 file2.inc -> current/file2.inc

I believe the handling of capital letter symlinks (current/File.inc) is also incorrect, even though the outcome looks okay.

@shizhMSFT shizhMSFT added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Nov 14, 2023
@wangxiaoxuan273
Copy link
Contributor

I have finished investigation, and here are the conclusions:

  1. By our design, oras-go does not support part of a symlink being a symlink (./file2 -- > current/file2 is not allowed, since current is a symlink). This is because there may be security risks. For example in the case of ./file2 -- > current/file2, current is a symlink and the link may point to a location outside of the working directory and cause security risks. So the error is the intended behavior.

  2. In the case of capital letter link name not giving the intended error, the behavior is due to the order of pull. oras pull command first pulls the items with capital letter names, then pulls those with small letter names. In the case of small letter link name (i.e. ./file2 --> current/file2), current has been pulled before file2, so oras-go is able to check that current is a symlink and forbids it. In the case of ./File2 --> current/file2, when File2 is pulled, current has not been pulled yet, so oras-go cannot check its nature and it allows the pull.

To prove this, we can rename current to be Current and let the link File2 remain File2. This will let oras to pull Current before File2, and the intended error will appear.

This behavior is also intended, since oras-go ensures security by the time File2 is pulled. But after File2 is pulled, it's the user's responsibility to not introduce anything risky (i.e. a symlink current that may point to anywhere).

  1. oras-go allows symlink to a concrete directory (i.e. ./current --> ./55.1 and ./file2 --> ./55.1/file2), as this is safe. So regarding this issue, I encourage you to use ./file2 --> ./55.1/file2 instead of ./file2 --> current/file2 if possible.

Thanks for opening this issue. @KristoferHansson

@KristoferHansson
Copy link
Author

Hi @wangxiaoxuan273

The primary challenge I am facing is packaging external data/packages, specifically in this case, the ICU package (https://github.com/unicode-org/icu). I cannot modify it due to dependencies on the existing structure. This limitation would require me to fork and patch these packages, which is not an ideal solution. I anticipate encountering similar issues when attempting to package and share other external resources using ORAS.

The crux of the problem is links pointing outside the "working directory". From my understanding, having links-to-links should not inherently cause any issues, provided that the "end" link does not point outside the working directory. It should be possible to verify whether a link directly points within the current working directory or its subdirectories.

With this in mind, would it be possible for you to support links-to-links as long as a single link doesn't terminate outside the current working directory? This feature would greatly benefit the packaging and sharing process for various external resources using ORAS.

/Kristofer

@yizha1 yizha1 added this to the v2.5.0 milestone Nov 22, 2023
@shizhMSFT shizhMSFT modified the milestones: v2.5.0, future Nov 22, 2023
@wangxiaoxuan273
Copy link
Contributor

@KristoferHansson

Thanks for the comment. Enabling links-to-links would require a lot of considerations and effort, and our team currently don't have enough resources for this feature.

Our team has discussed internally and here's our proposal: We have #644 planned for our next milestone. This feature would allow not extracting the tar file after it is downloaded by oras-pull. As the current links-to-links issue happens at the extract stage, a work-around would be using this upcoming feature to pull the tar, and extract it using another tool.

Would this work-around be good for your scenario? Feel free to let us know more about your exact needs.

@KristoferHansson
Copy link
Author

@wangxiaoxuan273

Hi, yes I saw #644 and it can most probably be used as a workaround.

@Wwwsylvia
Copy link
Member

Hi @KristoferHansson, thanks for the update!
We will close this issue then and will be tracking #644 instead. Please let us know if you have other concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants