Merge pull request #2739 from RasmusWL/python-modernise-security

Python: modernise Security/ queries

Author: tausbn

python/ql/src/Security/CVE-2018-1281/BindToAllInterfaces.ql

@@ -15,24 +15,24 @@ import python
 Value aSocket() { result.getClass() = Value::named("socket.socket") }
 
 CallNode socketBindCall() {
-  result = aSocket().attr("bind").(CallableValue).getACall() and major_version() = 3
-  or
-  result.getFunction().(AttrNode).getObject("bind").pointsTo(aSocket()) and
-  major_version() = 2
+    result = aSocket().attr("bind").(CallableValue).getACall() and major_version() = 3
+    or
+    result.getFunction().(AttrNode).getObject("bind").pointsTo(aSocket()) and
+    major_version() = 2
 }
 
 string allInterfaces() { result = "0.0.0.0" or result = "" }
 
 Value getTextValue(string address) {
-  result = Value::forUnicode(address) and major_version() = 3
-  or
-  result = Value::forString(address) and major_version() = 2
+    result = Value::forUnicode(address) and major_version() = 3
+    or
+    result = Value::forString(address) and major_version() = 2
 }
 
 from CallNode call, TupleValue args, string address
 where
-  call = socketBindCall() and
-  call.getArg(0).pointsTo(args) and
-  args.getItem(0) = getTextValue(address) and
-  address = allInterfaces()
+    call = socketBindCall() and
+    call.getArg(0).pointsTo(args) and
+    args.getItem(0) = getTextValue(address) and
+    address = allInterfaces()
 select call.getNode(), "'" + address + "' binds a socket to all interfaces."

python/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql

@@ -13,32 +13,28 @@
 import python
 import semmle.python.regex
 
-private string commonTopLevelDomainRegex() {
-    result = "com|org|edu|gov|uk|net|io"
-}
+private string commonTopLevelDomainRegex() { result = "com|org|edu|gov|uk|net|io" }
 
 /**
  * Holds if `pattern` is a regular expression pattern for URLs with a host matched by `hostPart`,
  * and `pattern` contains a subtle mistake that allows it to match unexpected hosts.
  */
 bindingset[pattern]
 predicate isIncompleteHostNameRegExpPattern(string pattern, string hostPart) {
-  hostPart = pattern
-        .regexpCapture("(?i).*" +
-            // an unescaped single `.`
-            "(?<!\\\\)[.]" +
-            // immediately followed by a sequence of subdomains, perhaps with some regex characters mixed in, followed by a known TLD
-            "([():|?a-z0-9-]+(\\\\)?[.](" + commonTopLevelDomainRegex() + "))" + ".*", 1)
+    hostPart = pattern
+                .regexpCapture("(?i).*" +
+                        // an unescaped single `.`
+                        "(?<!\\\\)[.]" +
+                        // immediately followed by a sequence of subdomains, perhaps with some regex characters mixed in, followed by a known TLD
+                        "([():|?a-z0-9-]+(\\\\)?[.](" + commonTopLevelDomainRegex() + "))" + ".*", 1)
 }
 
 from Regex r, string pattern, string hostPart
 where
-  (
-    r.getText() = pattern
-  ) and
-  isIncompleteHostNameRegExpPattern(pattern, hostPart) and
-  // ignore patterns with capture groups after the TLD
-  not pattern.regexpMatch("(?i).*[.](" + commonTopLevelDomainRegex()  + ").*[(][?]:.*[)].*")
+    r.getText() = pattern and
+    isIncompleteHostNameRegExpPattern(pattern, hostPart) and
+    // ignore patterns with capture groups after the TLD
+    not pattern.regexpMatch("(?i).*[.](" + commonTopLevelDomainRegex() + ").*[(][?]:.*[)].*")
 select r,
-  "This regular expression has an unescaped '.' before '" + hostPart +
-    "', so it might match more hosts than expected."
+    "This regular expression has an unescaped '.' before '" + hostPart +
+        "', so it might match more hosts than expected."

python/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql

@@ -10,20 +10,16 @@
  *       external/cwe/cwe-20
  */
 
-
 import python
 import semmle.python.regex
 
-private string commonTopLevelDomainRegex() {
-    result = "com|org|edu|gov|uk|net|io"
-}
+private string commonTopLevelDomainRegex() { result = "com|org|edu|gov|uk|net|io" }
 
 predicate looksLikeUrl(StrConst s) {
-    exists(string text |
-        text = s.getText()
-        |
-        text.regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+(" + 
-            commonTopLevelDomainRegex() +")(:[0-9]+)?/?")
+    exists(string text | text = s.getText() |
+        text
+                .regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+(" + commonTopLevelDomainRegex() +
+                        ")(:[0-9]+)?/?")
         or
         // target is a HTTP URL to a domain on any TLD
         text.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/?")
@@ -42,10 +38,8 @@ predicate incomplete_sanitization(Expr sanitizer, StrConst url) {
 }
 
 predicate unsafe_call_to_startswith(Call sanitizer, StrConst url) {
-    sanitizer.getFunc().(Attribute).getName() = "startswith"
-    and
-    sanitizer.getArg(0) = url
-    and
+    sanitizer.getFunc().(Attribute).getName() = "startswith" and
+    sanitizer.getArg(0) = url and
     not url.getText().regexpMatch("(?i)https?://[\\.a-z0-9-]+/.*")
 }
 

python/ql/src/Security/CWE-022/PathInjection.ql

@@ -18,18 +18,17 @@
 
 import python
 import semmle.python.security.Paths
-
 /* Sources */
 import semmle.python.web.HttpRequest
-
 /* Sinks */
 import semmle.python.security.injection.Path
 
 class PathInjectionConfiguration extends TaintTracking::Configuration {
-
     PathInjectionConfiguration() { this = "Path injection configuration" }
 
-    override predicate isSource(TaintTracking::Source source) { source instanceof HttpRequestTaintSource }
+    override predicate isSource(TaintTracking::Source source) {
+        source instanceof HttpRequestTaintSource
+    }
 
     override predicate isSink(TaintTracking::Sink sink) { sink instanceof OpenNode }
 
@@ -41,10 +40,9 @@ class PathInjectionConfiguration extends TaintTracking::Configuration {
     override predicate isExtension(TaintTracking::Extension extension) {
         extension instanceof AbsPath
     }
-
 }
 
-
 from PathInjectionConfiguration config, TaintedPathSource src, TaintedPathSink sink
 where config.hasFlowPath(src, sink)
-select sink.getSink(), src, sink, "This path depends on $@.", src.getSource(), "a user-provided value"
+select sink.getSink(), src, sink, "This path depends on $@.", src.getSource(),
+    "a user-provided value"

python/ql/src/Security/CWE-022/TarSlip.ql

@@ -3,7 +3,7 @@
  * @description Extracting files from a malicious tar archive without validating that the
  *              destination file path is within the destination directory can cause files outside
  *              the destination directory to be overwritten.
-* @kind path-problem
+ * @kind path-problem
  * @id py/tarslip
  * @problem.severity error
  * @precision medium
@@ -13,76 +13,58 @@
 
 import python
 import semmle.python.security.Paths
-
 import semmle.python.security.TaintTracking
 import semmle.python.security.strings.Basic
 
 /** A TaintKind to represent open tarfile objects. That is, the result of calling `tarfile.open(...)` */
 class OpenTarFile extends TaintKind {
-    OpenTarFile() {
-        this = "tarfile.open"
-    }
+    OpenTarFile() { this = "tarfile.open" }
 
     override TaintKind getTaintOfMethodResult(string name) {
         name = "getmember" and result instanceof TarFileInfo
         or
         name = "getmembers" and result.(SequenceKind).getItem() instanceof TarFileInfo
     }
 
-    override ClassValue getType() {
-        result = Module::named("tarfile").attr("TarFile")
-    }
-
-    override TaintKind getTaintForIteration() {
-        result instanceof TarFileInfo
-    }
+    override ClassValue getType() { result = Value::named("tarfile.TarFile") }
 
+    override TaintKind getTaintForIteration() { result instanceof TarFileInfo }
 }
 
 /** The source of open tarfile objects. That is, any call to `tarfile.open(...)` */
 class TarfileOpen extends TaintSource {
-
     TarfileOpen() {
-        Module::named("tarfile").attr("open").getACall() = this
-        and
-        /* If argument refers to a string object, then it's a hardcoded path and
+        Value::named("tarfile.open").getACall() = this and
+        /*
+         * If argument refers to a string object, then it's a hardcoded path and
          * this tarfile is safe.
          */
-        not this.(CallNode).getAnArg().refersTo(any(StringObject str))
-        and
+
+        not this.(CallNode).getAnArg().pointsTo(any(StringValue str)) and
         /* Ignore opens within the tarfile module itself */
         not this.(ControlFlowNode).getLocation().getFile().getBaseName() = "tarfile.py"
     }
 
-    override predicate isSourceOf(TaintKind kind) {
-        kind instanceof OpenTarFile
-    }
-
+    override predicate isSourceOf(TaintKind kind) { kind instanceof OpenTarFile }
 }
 
 class TarFileInfo extends TaintKind {
+    TarFileInfo() { this = "tarfile.entry" }
 
-    TarFileInfo() {
-        this = "tarfile.entry"
-    }
-
-    override TaintKind getTaintOfMethodResult(string name) {
-        name = "next" and result = this
-    }
+    override TaintKind getTaintOfMethodResult(string name) { name = "next" and result = this }
 
     override TaintKind getTaintOfAttribute(string name) {
         name = "name" and result instanceof TarFileInfo
     }
 }
 
+/*
+ * For efficiency we don't want to track the flow of taint
+ * around the tarfile module.
+ */
 
-/* For efficiency we don't want to track the flow of taint
- * around the tarfile module. */
 class ExcludeTarFilePy extends Sanitizer {
-
-    ExcludeTarFilePy() {
-        this = "Tar sanitizer"
-    }
+    ExcludeTarFilePy() { this = "Tar sanitizer" }
 
     override predicate sanitizingNode(TaintKind taint, ControlFlowNode node) {
         node.getLocation().getFile().getBaseName() = "tarfile.py" and
@@ -94,45 +76,34 @@ class ExcludeTarFilePy extends Sanitizer {
             taint.(SequenceKind).getItem() instanceof TarFileInfo
         )
     }
-
 }
 
 /* Any call to an extractall method */
 class ExtractAllSink extends TaintSink {
-
     CallNode call;
 
     ExtractAllSink() {
         this = call.getFunction().(AttrNode).getObject("extractall") and
         count(call.getAnArg()) = 0
     }
 
-    override predicate sinks(TaintKind kind) {
-        kind instanceof OpenTarFile
-    }
-
+    override predicate sinks(TaintKind kind) { kind instanceof OpenTarFile }
 }
 
 /* Argument to extract method */
 class ExtractSink extends TaintSink {
-
     CallNode call;
 
     ExtractSink() {
         call.getFunction().(AttrNode).getName() = "extract" and
         this = call.getArg(0)
     }
 
-    override predicate sinks(TaintKind kind) {
-        kind instanceof TarFileInfo
-    }
-
+    override predicate sinks(TaintKind kind) { kind instanceof TarFileInfo }
 }
 
-
 /* Members argument to extract method */
 class ExtractMembersSink extends TaintSink {
-
     CallNode call;
 
     ExtractMembersSink() {
@@ -145,21 +116,15 @@ class ExtractMembersSink extends TaintSink {
         or
         kind instanceof OpenTarFile
     }
-
 }
 
 class TarFileInfoSanitizer extends Sanitizer {
-
-    TarFileInfoSanitizer() {
-        this = "TarInfo sanitizer"
-    }
+    TarFileInfoSanitizer() { this = "TarInfo sanitizer" }
 
     override predicate sanitizingEdge(TaintKind taint, PyEdgeRefinement test) {
         path_sanitizing_test(test.getTest()) and
         taint instanceof TarFileInfo
     }
-
-
 }
 
 private predicate path_sanitizing_test(ControlFlowNode test) {
@@ -170,7 +135,6 @@ private predicate path_sanitizing_test(ControlFlowNode test) {
 }
 
 class TarSlipConfiguration extends TaintTracking::Configuration {
-
     TarSlipConfiguration() { this = "TarSlip configuration" }
 
     override predicate isSource(TaintTracking::Source source) { source instanceof TarfileOpen }
@@ -193,16 +157,15 @@ class TarSlipConfiguration extends TaintTracking::Configuration {
             node.asVariable().getDefinition() = def
             or
             node.asCfgNode() = def.getDefiningNode()
-            |
+        |
             def.getScope() = Value::named("tarfile.open").(CallableValue).getScope()
             or
             def.isSelf() and def.getScope().getEnclosingModule().getName() = "tarfile"
         )
     }
 }
 
-
 from TarSlipConfiguration config, TaintedPathSource src, TaintedPathSink sink
 where config.hasFlowPath(src, sink)
-select sink.getSink(), src, sink, "Extraction of tarfile from $@", src.getSource(), "a potentially untrusted source"
-
+select sink.getSink(), src, sink, "Extraction of tarfile from $@", src.getSource(),
+    "a potentially untrusted source"

python/ql/src/Security/CWE-022/examples/tainted_path.py

@@ -11,7 +11,6 @@
 
 def user_picture1(request):
     """A view that is vulnerable to malicious file access."""
-    base_path = '/server/static/images'
     filename = request.GET.get('p')
     # BAD: This could read any file on the file system
     data = open(filename, 'rb').read()

python/ql/src/Security/CWE-078/CommandInjection.ql

@@ -16,18 +16,17 @@
 
 import python
 import semmle.python.security.Paths
-
 /* Sources */
 import semmle.python.web.HttpRequest
-
 /* Sinks */
 import semmle.python.security.injection.Command
 
 class CommandInjectionConfiguration extends TaintTracking::Configuration {
-
     CommandInjectionConfiguration() { this = "Command injection configuration" }
 
-    override predicate isSource(TaintTracking::Source source) { source instanceof HttpRequestTaintSource }
+    override predicate isSource(TaintTracking::Source source) {
+        source instanceof HttpRequestTaintSource
+    }
 
     override predicate isSink(TaintTracking::Sink sink) {
         sink instanceof OsCommandFirstArgument or
@@ -37,9 +36,9 @@ class CommandInjectionConfiguration extends TaintTracking::Configuration {
     override predicate isExtension(TaintTracking::Extension extension) {
         extension instanceof FirstElementFlow
     }
-
 }
 
 from CommandInjectionConfiguration config, TaintedPathSource src, TaintedPathSink sink
 where config.hasFlowPath(src, sink)
-select sink.getSink(), src, sink, "This command depends on $@.", src.getSource(), "a user-provided value"
+select sink.getSink(), src, sink, "This command depends on $@.", src.getSource(),
+    "a user-provided value"