From f19eb783be66edff2b75a5f8932577a374177eb9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 29 Aug 2022 10:49:54 +0200 Subject: [PATCH] Generalize file/path taint steps This is needed by PathSanitizer but also helps simplify ZipSlip.ql --- .../code/java/dataflow/ExternalFlow.qll | 14 ------- .../lib/semmle/code/java/security/Files.qll | 25 ++++++++++++ java/ql/src/Security/CWE/CWE-022/ZipSlip.ql | 39 ------------------- .../CWE-022/semmle/tests/ZipSlip.expected | 3 -- 4 files changed, 25 insertions(+), 56 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 5dada4f8532..a235f43362d 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -361,19 +361,7 @@ private class SummaryModelCsvBase extends SummaryModelCsv { "java.net;URI;false;toURL;;;Argument[-1];ReturnValue;taint;manual", "java.net;URI;false;toString;;;Argument[-1];ReturnValue;taint;manual", "java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint;manual", "java.nio;ByteBuffer;false;array;();;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint;manual", - "java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Path;true;toUri;;;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint;manual", "java.io;BufferedReader;true;readLine;;;Argument[-1];ReturnValue;taint;manual", "java.io;Reader;true;read;();;Argument[-1];ReturnValue;taint;manual", // arg to return @@ -400,8 +388,6 @@ private class SummaryModelCsvBase extends SummaryModelCsv { // arg to arg "java.lang;System;false;arraycopy;;;Argument[0];Argument[2];taint;manual", // constructor flow - "java.io;File;false;File;;;Argument[0];Argument[-1];taint;manual", - "java.io;File;false;File;;;Argument[1];Argument[-1];taint;manual", "java.net;URI;false;URI;(String);;Argument[0];Argument[-1];taint;manual", "java.net;URL;false;URL;(String);;Argument[0];Argument[-1];taint;manual", "javax.xml.transform.stream;StreamSource;false;StreamSource;;;Argument[0];Argument[-1];taint;manual", diff --git a/java/ql/lib/semmle/code/java/security/Files.qll b/java/ql/lib/semmle/code/java/security/Files.qll index 3d783b93d30..d4326a6faad 100644 --- a/java/ql/lib/semmle/code/java/security/Files.qll +++ b/java/ql/lib/semmle/code/java/security/Files.qll @@ -70,3 +70,28 @@ private class WriteFileSinkModels extends SinkModelCsv { ] } } + +private class FileSummaryModels extends SummaryModelCsv { + override predicate row(string row) { + row = + [ + "java.io;File;false;File;;;Argument[0];Argument[-1];taint;manual", + "java.io;File;false;File;;;Argument[1];Argument[-1];taint;manual", + "java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint;manual", + "java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint;manual", + "java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint;manual", + "java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint;manual", + "java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint;manual", + "java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint;manual", + "java.nio.file;Path;true;toAbsolutePath;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;true;toUri;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint;manual", + "java.nio.file;FileSystem;true;getPath;;;Argument[0];ReturnValue;taint;manual", + "java.nio.file;FileSystem;true;getRootDirectories;;;Argument[0];ReturnValue;taint;manual" + ] + } +} diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql index dc382c9678c..3f63123cfb6 100644 --- a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql +++ b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql @@ -36,41 +36,6 @@ class ArchiveEntryNameMethod extends Method { } } -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `String`, - * `File`, and `Path`. - */ -predicate filePathStep(ExprNode n1, ExprNode n2) { - exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeFile | - n1.asExpr() = cc.getAnArgument() and - n2.asExpr() = cc - ) - or - exists(MethodAccess ma, Method m | - ma.getMethod() = m and - n1.asExpr() = ma.getQualifier() and - n2.asExpr() = ma - | - m.getDeclaringType() instanceof TypeFile and m.hasName("toPath") - or - m.getDeclaringType() instanceof TypePath and m.hasName("toAbsolutePath") - or - m.getDeclaringType() instanceof TypePath and m.hasName("toFile") - ) -} - -predicate fileTaintStep(ExprNode n1, ExprNode n2) { - exists(MethodAccess ma, Method m | - n1.asExpr() = ma.getQualifier() or - n1.asExpr() = ma.getAnArgument() - | - n2.asExpr() = ma and - ma.getMethod() = m and - m.getDeclaringType() instanceof TypePath and - m.hasName("resolve") - ) -} - class ZipSlipConfiguration extends TaintTracking::Configuration { ZipSlipConfiguration() { this = "ZipSlip" } @@ -80,10 +45,6 @@ class ZipSlipConfiguration extends TaintTracking::Configuration { override predicate isSink(Node sink) { sink instanceof FileCreationSink } - override predicate isAdditionalTaintStep(Node n1, Node n2) { - filePathStep(n1, n2) or fileTaintStep(n1, n2) - } - override predicate isSanitizer(Node node) { node instanceof PathInjectionSanitizer } } diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected index 48c54030679..7c6a5bffc2e 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected @@ -1,8 +1,5 @@ edges | ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:8:31:8:34 | name : String | -| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:9:48:9:51 | file | -| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:10:49:10:52 | file | -| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:11:36:11:39 | file | | ZipTest.java:8:17:8:35 | new File(...) : File | ZipTest.java:9:48:9:51 | file | | ZipTest.java:8:17:8:35 | new File(...) : File | ZipTest.java:10:49:10:52 | file | | ZipTest.java:8:17:8:35 | new File(...) : File | ZipTest.java:11:36:11:39 | file |