[sec] check if symlink points outside of destination directory

[coolaj86]

I was able to confirm that os.Create() will happily follow symlinks.

Although standard tools like tar will not likely allow you to add two files with the same name to an archive file, it's certainly easy enough to do in code. This means that we need to add an additional check before writing to a destination to make sure that it's not a symlink outside of the destination.

The vulnerability only exists if OverwriteExisting is also turned on.

What file is affected?

filecompressor.go:

// DecompressFile reads the source file and decompresses it to destination.
func (fc FileCompressor) DecompressFile(source, destination string) error {
    if fc.Decompressor == nil {
        return fmt.Errorf("no decompressor specified")
    }

    if !fc.OverwriteExisting && fileExists(destination) {
        return fmt.Errorf("file exists: %s", destination)
    }

    // TODO needs check here

    in, err := os.Open(source)
    if err != nil {
        return err
    }
    defer in.Close()

    out, err := os.Create(destination)
    if err != nil {
        return err
    }
    defer out.Close()

    return fc.Decompress(in, out)
}

Possible solution

If the destination exists we should always check if it's a symlink first:

    if fileExists(destination) {
        // TODO check if destination is a symlink with an outside target
        // should be a simple os.Lstat()

        if !fc.OverwriteExisting {
            return fmt.Errorf("file exists: %s", destination)
        }
    }

Also, we could block ALL symilnks that have a target outside of the destination directory.

I think the former approach is the best for now, as some archives may link outside of the destination directory for legitimate reasons.

Please link to any related issues, pull requests, and/or discussion

This is related to

• Fix cve 2019 10743 #203
• Prevent arbitrary file overwrite via path traversal [CVE-2019-10743] #169
• fix: #65#issuecomment-395988244 - zip slip by symlink #70
• Separate CVE for related symlink vulnerability? snyk/zip-slip-vulnerability#41

[coolaj86]

This one is difficult to test because it requires creating a specially crafted double-evil.zip that contains "invalid" double entries.

I'd say it would be sufficient to

• not break existing tests
• add an unsafe option that turns off the protection

I'll open another issue for crafting the bad file. I don't think that can be done with arc currently without adding an unsafe flag because I seem to remember in checking to see if something is already in the archive.

Edit: deleted comments

There were a few back and forth comments about who might take this, but in the end no one did, so I removed them.

[stoiandan]

Hi, I'd like to take this one! :)

[stoiandan]

I understand we need to make sure, the destination path is not a symlink, if it is, I guess we need to exit with an error. Maybe something like?:

destination path is invalid, because it's a symbolic link

Also when you say:

    // TODO check if destination is a symlink with an outside target

What do you mean by outside target?

[tslocum]

@stoiandan I have submitted a fix for this issue as #269.

[mholt]

Thank you for the patch, @tslocum -- however, it's been a long time and v4 no longer has this issue, so, closing. (Could be relevant again if/when v4 gets a command.)