Support symbolic links in archive files

[starpit]

i have a tar.gz file that contains symlinks. when krew (0.3.1) tries to install my archive, i see this error:

failed to install plugin "kubeui": install failed: failed to download and extract: failed to download and verify file: failed to extract file: unable to handle file type 50 for "Kui-darwin-x64/Kui.app/Contents/Frameworks/Mantle.framework/Mantle" in tar

that file is a symlink, and the first symlink in the archive. i don't see anything else special about it that might lead to errors.

[ahmetb]

We should probably do some research ahead of time before allowing this.

Nearly all client software that deals with symlinks in downloaded payload exposes security vulnerabilities down the line based on my observation. Kubectl was subject to this too.

[starpit]

ok. electron client builds will be filled with symlinks.

[ahmetb]

I think we might be ok with evaluating the symlink and see if it goes above the plugin install directory. We do similar checks elsewhere. That might help with the security aspect.

[starpit]

nice, that makes sense

[corneliusweig]

I have strong reservations against allowing symlinks in tarballs.

kubectl used to do this and suffered from several CVEs as a result from that, for example

• CVE-2018-1002100
• CVE-2019-1002101
• CVE-2019-11246
• CVE-2019-11249

These repeated vulnerabilities bound considerable development resources and also posed a threat to the reputation of k8s with regards to security. As a result, there is now a KEP that will almost certainly lead to dropping support for symlinks in kubectl cp.

I don't think that we can reasonably assume that we are going to be smarter than the kubectl core developers when it comes to making symlinks secure. Therefore, we should not do it.

---

@starpit Would it possible to build plugin bundles without symlinks?

[starpit]

@corneliusweig commented:

Would it possible to build plugin bundles without symlinks?

the issue is particularly problematic on macos, where applications are really directories, and chromium uses many symlinks therein:

method	size
tar jcf	57MB
tar zcf	64MB
tar zhcf	181MB
expanded	168MB
expanded zhcf	458MB

keep in mind that all but the last two rows are the compressed sizes.

[corneliusweig]

I see, thanks for checking this out. Would those exploded bundles still work, or does symlink dereferencing even break the application?

Now for kubectl cp it as suggested to use the system tar command instead. Maybe we can work with that. However, this will be a bad UX on Windows, because many people won't have tar installed on their systems.

[starpit]

i believe that the symlink-dereferenced applications work. most of these come from macOS Framework versioning (e.g. Current -> )

i will quantify windows and linux. it is possible those platforms do not suffer from the same egregious symlinking issue.

[starpit]

confirmed: linux and windows suffer not at all from the symlink issue. this seems only to affect macOS application bundles.

$ find dist/electron/Kui-win32-x64 -type l | wc -l
       0
$ find dist/electron/Kui-linux-x64 -type l | wc -l
       0
$ find dist/electron/Kui-darwin-x64 -type l | wc -l
      19

[starpit]

should i take a shot at updating krew to use an external tar (or all tarball extractions?) on macos?

[ahmetb]

I'd actually be more hesitant to maintain two code paths for this and deal with GNU tar vs BSD tar.

I think we have a rather safe(r) handling of symlinks so far.

For example, we let developers choose what plugin binary must be symlinked via the spec.bin field. This field actually does allow relative paths –but it doesn't let you "go above" the plugin install directory.

I think for our tar code, we can consider a similar check to make sure the resulting symlinks still lies within the extraction directory. (That would still work, right?)

[starpit]

it does seem that, with some specific and heavy restrictions, we could have safe symlink support. cornelius raises a good point that rolling our own does mean that the security implications are in our hands. the tar "Security Rules of Thumb" advises:

Extract from an untrusted archive only into an otherwise-empty directory. This directory and its parent should be accessible only to trusted users.

which i think krew does. i will update my PR so that

1. the symlink case block confirms enclosure
   2a) otherwise, emits a warning and skips, but does not fail fast; or
   2b) fail if any escaping symlinks are detected

1 and 2b?

Update: another thought; we could disallow symlinks to any enclosing (or absolute) path. e.g.

ok: foo -> bar/baz
ok: foo -> bar
ok: foo -> ./bar

error: foo -> /path/to/danger
error: foo -> ../bar/baz

the last one is possibly valid, but requires that we maintain code that recognizes correctly where the "top" lies.

[ahmetb]

the last one is possibly valid, but requires that we maintain code that recognizes correctly where the "top" lies.

yeah it's valid. we already have code for this for symlinking the plugins today. we make sure even our link is not escaping where it's supposed to link to (since bin field is technically "user input"). could reuse that.

[ahmetb]

/retitle Support symbolic links in archive files
/kind proposal

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.