Closed starpit closed 4 years ago
ahmetb
commented
5 years ago 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
commented
5 years ago ok. electron client builds will be filled with symlinks.
ahmetb
commented
5 years ago 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
commented
5 years ago nice, that makes sense
corneliusweig
commented
5 years ago 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
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
commented
5 years ago @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
commented
5 years ago 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
commented
5 years ago 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
commented
5 years ago 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
19should i take a shot at updating krew to use an external tar (or all tarball extractions?) on macos?
ahmetb
commented
5 years ago 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
commented
5 years ago 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
commented
5 years ago 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
commented
5 years ago /retitle Support symbolic links in archive files /kind proposal
fejta-bot
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago 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
commented
4 years ago @fejta-bot: Closing this issue.
i have a tar.gz file that contains symlinks. when krew (0.3.1) tries to install my archive, i see this error:
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.