From 894ea7d0ac4c49e356a3453405caab7a11650b3d Mon Sep 17 00:00:00 2001 From: Dan Lebrero Date: Mon, 18 Jun 2018 14:01:11 +0200 Subject: [PATCH 1/2] Fixing Zip Slip vulnerability See https://github.com/snyk/zip-slip-vulnerability --- src/me/raynes/fs/compression.clj | 31 +++++++++++++++++--------- test/me/raynes/core_test.clj | 8 ++++++- test/me/raynes/testfiles/zip-slip.tar | Bin 0 -> 10240 bytes test/me/raynes/testfiles/zip-slip.zip | Bin 0 -> 545 bytes 4 files changed, 28 insertions(+), 11 deletions(-) create mode 100644 test/me/raynes/testfiles/zip-slip.tar create mode 100644 test/me/raynes/testfiles/zip-slip.zip diff --git a/src/me/raynes/fs/compression.clj b/src/me/raynes/fs/compression.clj index 0bc9a29..f22d3d3 100644 --- a/src/me/raynes/fs/compression.clj +++ b/src/me/raynes/fs/compression.clj @@ -7,7 +7,14 @@ TarArchiveEntry) (org.apache.commons.compress.compressors bzip2.BZip2CompressorInputStream xz.XZCompressorInputStream) - (java.io ByteArrayOutputStream))) + (java.io ByteArrayOutputStream File))) + +(defn- check-final-path-inside-target-dir! [f target-dir entry] + (when-not (-> f .getCanonicalPath (.startsWith (.getCanonicalPath target-dir))) + (throw (ex-info "Expanding entry would be created outside target dir" + {:entry entry + :entry-final-path f + :target-dir target-dir})))) (defn unzip "Takes the path to a zipfile `source` and unzips it to target-dir." @@ -16,9 +23,11 @@ ([source target-dir] (with-open [zip (ZipFile. (fs/file source))] (let [entries (enumeration-seq (.entries zip)) + target-dir-as-file (fs/file target-dir) target-file #(fs/file target-dir (str %))] (doseq [entry entries :when (not (.isDirectory ^java.util.zip.ZipEntry entry)) - :let [f (target-file entry)]] + :let [^File f (target-file entry)]] + (check-final-path-inside-target-dir! f target-dir-as-file entry) (fs/mkdirs (fs/parent f)) (io/copy (.getInputStream zip entry) f)))) target-dir)) @@ -103,14 +112,16 @@ ([source] (untar source (name source))) ([source target] (with-open [tin (TarArchiveInputStream. (io/input-stream (fs/file source)))] - (doseq [^TarArchiveEntry entry (tar-entries tin) :when (not (.isDirectory entry)) - :let [output-file (fs/file target (.getName entry))]] - (fs/mkdirs (fs/parent output-file)) - (io/copy tin output-file) - (when (.isFile entry) - (fs/chmod (apply str (take-last - 3 (format "%05o" (.getMode entry)))) - (.getPath output-file))))))) + (let [target-dir-as-file (fs/file target)] + (doseq [^TarArchiveEntry entry (tar-entries tin) :when (not (.isDirectory entry)) + :let [output-file (fs/file target (.getName entry))]] + (check-final-path-inside-target-dir! output-file target-dir-as-file entry) + (fs/mkdirs (fs/parent output-file)) + (io/copy tin output-file) + (when (.isFile entry) + (fs/chmod (apply str (take-last + 3 (format "%05o" (.getMode entry)))) + (.getPath output-file)))))))) (defn gunzip "Takes a path to a gzip file `source` and unzips it." diff --git a/test/me/raynes/core_test.clj b/test/me/raynes/core_test.clj index 0efa3d9..715e77f 100644 --- a/test/me/raynes/core_test.clj +++ b/test/me/raynes/core_test.clj @@ -345,7 +345,13 @@ (fact (unxz "xxx.xz" "xxx") (exists? "xxx") => true - (delete "xxx"))) + (delete "xxx")) + + (fact "zip-slip vulnerability" + (unzip "zip-slip.zip" "zip-slip") => (throws Exception "Expanding entry would be created outside target dir") + (untar "zip-slip.tar" "zip-slip") => (throws Exception "Expanding entry would be created outside target dir") + (exists? "/tmp/evil.txt") => false + (delete-dir "zip-slip"))) (let [win-root (when-not unix-root "c:")] (fact diff --git a/test/me/raynes/testfiles/zip-slip.tar b/test/me/raynes/testfiles/zip-slip.tar new file mode 100644 index 0000000000000000000000000000000000000000..264b25064d10164a497faa576c7c8628e9560499 GIT binary patch literal 10240 zcmeH}QEr4F5Qcs3Ddq$^3<|G3fR`{?*P6E2xVGu>J4$Meud8-VenP?kW*9#B9h$DY zq{H`+>{yuDap*4LKE_FYT zuMPd|dR-4L`f(?5bIi1ud1!NZX>X*pqDF55b)kNG=y(y_wgl2F&7R9HG-VrJw{<;x zi_+jx|6^IVEL{AK#d%}INJ8c8VMk-qKhfPQOj}!Q?rw+2I7syZ{6`($KeGC(ecLj009sH0T2KI5C8!X009sH0T2KI5C8!X009sH0T2KI5C8!X0D(1u EA5o@v#sB~S literal 0 HcmV?d00001 diff --git a/test/me/raynes/testfiles/zip-slip.zip b/test/me/raynes/testfiles/zip-slip.zip new file mode 100644 index 0000000000000000000000000000000000000000..38b3f499de0163e62ca15ce18350a9d9a477a51b GIT binary patch literal 545 zcmWIWW@h1H0D=Au{XYEp{-1?`Y!K#PkYPyA&ri`SsVE5z;bdU8U359h4v0%DxEUB( zzA-W|u!sQFm1JZVD*#cV0!Xz&eqJh90MJm76a&LlprHwl)s`S02)6*So}T`Ippx7I z{nWC|9FT|Lj?Pm62|-=W$Rx*%D=;L0E@xl>dYWNLBZ!3v8dgZqpan~SHzSh>Gwx6T jnE?Vz8bg8PfCLE8QsgiR@MdKLxrhk}K_2A>d6oeH^pk5C literal 0 HcmV?d00001 From f64177f93d2707836f219cce70c48fc3fdbb0c01 Mon Sep 17 00:00:00 2001 From: Dan Lebrero Date: Mon, 18 Jun 2018 18:46:59 +0200 Subject: [PATCH 2/2] Adding File/separator to end of target directory --- src/me/raynes/fs/compression.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/me/raynes/fs/compression.clj b/src/me/raynes/fs/compression.clj index f22d3d3..c43f009 100644 --- a/src/me/raynes/fs/compression.clj +++ b/src/me/raynes/fs/compression.clj @@ -10,7 +10,7 @@ (java.io ByteArrayOutputStream File))) (defn- check-final-path-inside-target-dir! [f target-dir entry] - (when-not (-> f .getCanonicalPath (.startsWith (.getCanonicalPath target-dir))) + (when-not (-> f .getCanonicalPath (.startsWith (str (.getCanonicalPath target-dir) File/separator))) (throw (ex-info "Expanding entry would be created outside target dir" {:entry entry :entry-final-path f