Skip to content
This repository has been archived by the owner on Feb 20, 2020. It is now read-only.

Bug 1466872: Fix zip slip vulnerability #99

Closed
wants to merge 1 commit into from
Closed

Bug 1466872: Fix zip slip vulnerability #99

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
"log"
Expand Down Expand Up @@ -150,6 +151,11 @@ func Unzip(b []byte, dest string) error {

path := filepath.Join(dest, f.Name)

// Fix for https://snyk.io/research/zip-slip-vulnerability
if !strings.HasPrefix(path, dest) {
Copy link
Member

@petemoore petemoore Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work if dest has not been cleaned (see filepath.Clean).

Also the Unzip function you are patching here is the one that is used to extract zips which are embedded into the worker type definition, which are trusted. Here we are not vulnerable, since if you can modify the content of the zip, you can also modify the absolute location it should be installed to, since both are specified in the same section of the worker type definition, with the same access controls required to modify them.

The code that extracts untrusted zips is in mounts.go which calls github.com/mholt/archiver package. That was already patched in mholt/archiver#65 and since we don't vendor that library, all new builds should pick up the fix. So we should already be safe.

Probably the best is for us to delete this Unzip method, and use package github.com/mholt/archiver everywhere, for simplicity, but again, it seems we are not vulnerable as things stand, so this is just a nice simplification rather than a security enhancement.

Despite this, many thanks for the patch. 😄

return fmt.Errorf("%s: illegal path", f.Name)
}

if f.FileInfo().IsDir() {
os.MkdirAll(path, f.Mode())
} else {
Expand Down