extractFilesFromArchive handles '..' in entry paths in a potentially insecure manner

[athas]

Consider this program, which creates and then unpacks a zip file evil.zip that contains a file with filepath "../evil":

module Main (main) where

import qualified Codec.Archive.Zip as Zip
import qualified Data.ByteString.Lazy as LBS

main :: IO ()
main = writeIt >> readIt
  where writeIt = do
          let entry = Zip.toEntry "../evil" 0 mempty
              archive = Zip.addEntryToArchive entry Zip.emptyArchive
          LBS.writeFile "evil.zip" $ Zip.fromArchive archive

        readIt = do
          archive <- Zip.toArchive <$> LBS.readFile "evil.zip"
          Zip.extractFilesFromArchive [Zip.OptVerbose] archive

When run, this program will indeed create an empty file at "../evil". This means that extractFilesFromArchive is dangerous to use on untrusted zip files. The unzip tool on *nix has some form of safety mechanism here:

$ unzip evil.zip 
Archive:  evil.zip
warning:  skipped "../" path component(s) in ../evil
 extracting: evil

I think the best solution is to fail in a noisy way when such dangerous entries are encountered.

[jgm]

Troels Henriksen <notifications@github.com> writes:

Consider this program, which creates and then unpacks a zip file `evil.zip` that contains a file with filepath `"../evil"`: ``` module Main (main) where import qualified Codec.Archive.Zip as Zip import qualified Data.ByteString.Lazy as LBS main :: IO () main = writeIt >> readIt where writeIt = do let entry = Zip.toEntry "../evil" 0 mempty archive = Zip.addEntryToArchive entry Zip.emptyArchive LBS.writeFile "evil.zip" $ Zip.fromArchive archive readIt = do archive <- Zip.toArchive <$> LBS.readFile "evil.zip" Zip.extractFilesFromArchive [Zip.OptVerbose] archive ``` When run, this program will indeed create an empty file at `"../evil"`. This means that `extractFilesFromArchive` is dangerous to use on untrusted zip files. The `unzip` tool on *nix has some form of safety mechanism here: ``` $ unzip evil.zip Archive: evil.zip warning: skipped "../" path component(s) in ../evil extracting: evil ``` I think the best solution is to fail in a noisy way when such dangerous entries are encountered.

Thanks for noticing this. I suppose one question is whether the failure should occur in the library function or just in the Zip program. The problem is that addEntryToArchive doesn't have a type that can really return a failure. Best, John

[athas]

One possibility is to do what unzip does, and just ignore the '..'s (possibly logging that). Another is to crash with 'error' like I assume is done for other kinds of invalid filepaths (like 'nul' on Windows or something that contains a NUL byte). I don't think it's important to do something particularly nice, since you're already quite far into the weeds and dealing with malformed data if this occurs.

[jgm]

Troels Henriksen <notifications@github.com> writes:

One possibility is to do what `unzip` does, and just ignore the '..'s (possibly logging that).

I think I'd be okay with ignoring them, but note that there's currently no way to log it, without changing the API (unless we just add 'Debug.trace', which feels wrong).

Another is to crash with 'error' like I assume is done for other kinds of invalid filepaths (like 'nul' on Windows or something that contains a NUL byte). I don't think it's important to do something particularly nice, since you're already quite far into the weeds and dealing with malformed data if this occurs.

Yes, I suppose we could regard it as a programmer error if someone tries to zip up something in ../, so maybe error wouldn't be inappropriate. I'm not sure which approach is best.

[tmspzz]

what about deprecating addEntryToArchive and adding securelyAddEntryToArchive with the appropriate type for errors?

[athas]

The problem is not addEntryToArchive, but extractFilesFromArchive. An attacker can always craft a malicious zip archive.

I think the best solution is for extractFilesFromArchive to crash with error when unsafe paths are found. This is also what it will do for other malformed paths (like empty ones). The second best solution is to rewrite the paths to be safe (as unzip(1) does) and print a notice if OptVerbose is used.

[jgm]

Actually the root of it is writeEntry. Note that `writeEntry` already throws a CRC32Mismatch exception in certain cases. That is an instance of ZipException, which we define in this module. So, I propose that we add UnsafePath FilePath to ZipException, and throw that exception for unsafe paths. This is better than 'error' because it can be caught. The only remaining question is how we test for unsafe paths? One approach would be to check that (a) the path is relative and (b) there are no ".." path components. Would that suffice?

[tmspzz]

ZipException, and throw that exception for unsafe paths. This is better than 'error' because it can be caught.

Yes.

The only remaining question is how we test for unsafe paths?

Make the path absolute (resolving symlinks as well) and check that it does not leave the root directory of the archive.

I use https://github.com/blender/Rome/blob/faed9ce50884338e1dbbe1863bba6e571374bb53/src/Utils.hs#L392