extractFilesFromArchive handles '..' in entry paths in a potentially insecure manner
Closed this issue · 7 comments
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.
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.
what about deprecating addEntryToArchive
and adding securelyAddEntryToArchive
with the appropriate type for errors?
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.
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