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.

Sign up for GitHub

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

Jump to bottom

Support for symlink when untarring an archive #60

Open
jcua opened this issue May 2, 2017 · 15 comments
Open

Support for symlink when untarring an archive #60

jcua opened this issue May 2, 2017 · 15 comments

Comments

@jcua
Copy link

jcua commented May 2, 2017

Operating System:
Ubuntu 16.04

Issue:
When a tarball has symlink inside it, go-getter turns the symlink into zero-length file. One benefit of having go-getter support this would be with respect to Nomad specifically with the artifact stanza https://www.nomadproject.io/docs/job-specification/artifact.html, since some tarball could contain symlink in it.

Repro:

  1. Create a test_dir.tar.gz with the following contents (note: test_ls.symlink)
vagrant@nomad-server01:/tmp$ tree test_dir/
test_dir/
├── a.txt
└── test_ls.symlink -> /bin/ls
  1. On the same directory, execute: python -m SimpleHTTPServer 8000
  2. Run go-getter http://127.0.0.1:8000/test_dir.tar.gz ~/
  3. Check what go-getter untarred (note: test_ls.symlink is 0 bytes)
vagrant@nomad-server01:~$ ls -l ~/test_dir/
total 4
-rw-r--r-- 1 vagrant vagrant 4 May  2 01:19 a.txt
-rwxr-xr-x 1 vagrant vagrant 0 May  2 01:19 test_ls.symlink
@tonyarkles
Copy link

Was just burned by this through Nomad.

@brmzkw
Copy link

brmzkw commented Nov 6, 2018

@tonyarkles same here. Since I'm using the raw_exec driver, I used tar as a workaround to extract the archive. I also extract the archive outside nomad's directory.

    task "mytask" {
      driver = "raw_exec"

      artifact {
        # https://github.com/hashicorp/go-getter/issues/60
        source = "https://url/to/archive/superapi-2.0.0-76-x86_64-xenial.tar.gz"
        options {
          archive = "false"
        }
      }

      config {
        command = "/bin/sh"
        args = [
          "-c",
          "
            mkdir -p /opt/subdir;
            tar -C /opt/subdir/ -xvf local/superapi-2.0.0-76-x86_64-xenial.tar.gz;
            /opt/ocs/superapi/bin/superapi
          "
        ]
      }
   }

@prologic
Copy link
Contributor

This is breaking us in production :/ Is there an ETA for a fix?

@johnzhanghua
Copy link

#37

Looks there is a PR for that, haven't got in.

@prologic
Copy link
Contributor

prologic commented Feb 22, 2019 via email

@prologic
Copy link
Contributor

prologic commented Feb 22, 2019 via email

@prologic
Copy link
Contributor

prologic commented Feb 22, 2019 via email

@prologic
Copy link
Contributor

Before I do; I'd love it if someone from Hashicorp commented on this with some more recent context/update.

@azr
Copy link
Contributor

azr commented Feb 25, 2019 •
edited
Loading

Hey @prologic ! I would love to review such a PR, I don't have much context to add here and also nothing to add on top of the original code review. We just have to make sure it works on all OSes and unit (ci) tests are green on all platforms 🙂.

@prologic
Copy link
Contributor

prologic commented Feb 25, 2019 via email

@cgbaker cgbaker mentioned this issue Mar 1, 2019
Open
@prologic prologic mentioned this issue Mar 4, 2019
Merged
@azr azr closed this as completed in #171 Mar 12, 2019
@schmichael
Copy link
Member

Reopening due to #174 reverting #171. go-getter should implement the same path traversal protections as GNU tar for symlinks. See #175 for details.

@schmichael schmichael reopened this Mar 26, 2019
@prologic
Copy link
Contributor

prologic commented Apr 4, 2019

I needed to support extracting symlinks in Tar archives myself internally at our company with the package manager we're writing to support Nomad. I thought I may as well (at the same time) rethink/resolve what was attempted here and came across the securejoin library. It appears ti on inspection of the code and testing solve exactly what we need to address symlink security issues that were present in #171

I'd love it if someone could also confirm the validity of using this library which itself is slated to go into the Go standard library. (not point reinventing the wheel on this one!) If happy I can resubmit a slightly improved version of #171 with added tests for verifications that we don't escape the chroot directory being extracted to in any way.

@schmichael
Copy link
Member

While it's difficult to determine whether or not it's safe to trust a 3rd party library, the list of projects using SecureJoin gives me quite a bit of confidence (helm, opencontainers, jessfraz, etc). The reasons given in #171 to reject it from the stdlib don't seem to apply to go-getter's use case. If I can attempt to summarize them:

  • Poor Portability - It sounds like it may not be perfect on Windows. It also sounds like it does the best we could do manually, so this isn't a concern for me.
  • Too Subtle - Secure vs Not APIs are always confusing for developers. So I can't blame the Golang maintainers for not wanting to muddy the waters for every developer just wanting to create a path. However, this concern is specific to the stdlib and does not apply to go-getter or Nomad.
  • Imperfect Implementation - Due to kernel limitations it's impossible to guarantee this is safe, but I believe it is the safest possible userspace implementation. I don't see a reason to believe there's a safer option for go-getter or Nomad.

tl;dr - Yes! This should work! I think as long as you include some symlink root path escape tests we can accept tar symlink implementations using this library.

@prologic
Copy link
Contributor

prologic commented Apr 5, 2019

I actually tried to look for the C source to GNU Tar (I found the source) but I wasn't able to find the code that protects against chroot escapeing :/ (I only looked in obvious *.c files) -- For me re-implementing this logic is hard and tricky to get right; so using this library from someone that clearly knows what they're doing here sounds a lot better than re-inventing the wheel :)

@prologic prologic mentioned this issue May 22, 2019
Closed
@prologic prologic mentioned this issue Jul 8, 2019
Closed
@prologic
Copy link
Contributor

prologic commented Jul 9, 2019

@schmichael PR #129 is a new implementation as we discussed in comments above and ensures we don't escape the path we're extracting in to.

@notnoop notnoop mentioned this issue Jul 24, 2020
Closed
@mruz mruz mentioned this issue Sep 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adds support for Symlinks in all Tar decompressors
7 participants
@schmichael @tonyarkles @brmzkw @azr @prologic @jcua @johnzhanghua