From b866f1b21eb89710c69c7460f1890e1e204fa386 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 2 Jul 2021 19:30:33 +0800 Subject: [PATCH 1/8] Add CWE-348 ClientSuppliedIpUsedInSecurityCheck --- .../ClientSuppliedIpUsedInSecurityCheck.py | 35 +++++ .../ClientSuppliedIpUsedInSecurityCheck.qhelp | 35 +++++ .../ClientSuppliedIpUsedInSecurityCheck.ql | 48 +++++++ ...ClientSuppliedIpUsedInSecurityCheckLib.qll | 130 ++++++++++++++++++ ...ientSuppliedIpUsedInSecurityCheck.expected | 11 ++ .../ClientSuppliedIpUsedInSecurityCheck.py | 35 +++++ .../ClientSuppliedIpUsedInSecurityCheck.qlref | 1 + 7 files changed, 295 insertions(+) create mode 100644 python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py create mode 100644 python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp create mode 100644 python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql create mode 100644 python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py new file mode 100644 index 000000000000..5ae8d3dd1abc --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py @@ -0,0 +1,35 @@ +#!/usr/bin/env python +# -*- coding: UTF-8 -*- +""" +@Desc ：ip address spoofing +""" +from flask import Flask +from flask import request + +app = Flask(__name__) + +@app.route('/bad1') +def bad1(): + client_ip = request.headers.get('x-forwarded-for') + if not client_ip.startswith('192.168.'): + raise Exception('ip illegal') + return 'bad1' + +@app.route('/bad2') +def bad2(): + client_ip = request.headers.get('x-forwarded-for') + if not client_ip == '127.0.0.1': + raise Exception('ip illegal') + return 'bad2' + +@app.route('/good1') +def good1(): + client_ip = request.headers.get('x-forwarded-for') + client_ip = client_ip.split(',')[client_ip.split(',').length - 1] + if not client_ip == '127.0.0.1': + raise Exception('ip illegal') + return 'good1' + +if __name__ == '__main__': + app.debug = True + app.run() \ No newline at end of file diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp new file mode 100644 index 000000000000..653c93341c95 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp @@ -0,0 +1,35 @@ + + + +

An original client IP address is retrieved from an http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP +etc.), which is used to ensure security. Attackers can forge the value of these identifiers to +bypass a ban-list, for example.

+ + + + +

Do not trust the values of HTTP headers allegedly identifying the originating IP. If you are aware your application will run behind some reverse proxies then the last entry of a X-Forwarded-For header value may be more trustworthy than the rest of it because some reverse proxies append the IP address they observed to the end of any remote-supplied header.

+ + + + +

The following examples show the bad case and the good case respectively. +In bad1 method and bad2 method, the client ip the X-Forwarded-For is split into comma-separated values, but the less-trustworthy first one is used. Both of these examples could be deceived by providing a forged HTTP header. The method +good1 similarly splits an X-Forwarded-For value, but uses the last, more-trustworthy entry.

+ + + + + + +
  • Dennis Schneider: +Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring +
    • + +
  • Security Rule Zero: A Warning about X-Forwarded-For +
    • + +     +     diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql new file mode 100644 index 000000000000..7866fbd681c6 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql @@ -0,0 +1,48 @@ +/** + * @name IP address spoofing + * @description A remote endpoint identifier is read from an HTTP header. Attackers can modify the value + * of the identifier to forge the client ip. + * @kind path-problem + * @problem.severity error + * @precision high + * @id py/ip-address-spoofing + * @tags security + * external/cwe/cwe-348 + */ + +import python +import semmle.python.dataflow.new.DataFlow +import semmle.python.dataflow.new.TaintTracking +import ClientSuppliedIpUsedInSecurityCheckLib +import DataFlow::PathGraph + +/** + * Taint-tracking configuration tracing flow from obtaining a client ip from an HTTP header to a sensitive use. + */ +class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configuration { + ClientSuppliedIpUsedInSecurityCheckConfig() { this = "ClientSuppliedIpUsedInSecurityCheckConfig" } + + override predicate isSource(DataFlow::Node source) { + source instanceof ClientSuppliedIpUsedInSecurityCheck + } + + override predicate isSink(DataFlow::Node sink) { + sink instanceof ClientSuppliedIpUsedInSecurityCheckSink + } + + override predicate isSanitizer(DataFlow::Node node) { + exists(Subscript ss | + not ss.getIndex().(IntegerLiteral).getText() = "0" and + ss.getObject().(Call).getFunc().(Attribute).getName() = "split" and + ss.getObject().(Call).getArg(0).(StrConst).getText() = "," and + ss.getObject().(Call).getFunc().(Attribute).getObject() = node.asExpr() + ) + } +} + +from + ClientSuppliedIpUsedInSecurityCheckConfig config, DataFlow::PathNode source, + DataFlow::PathNode sink +where config.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "IP address spoofing might include code from $@.", + source.getNode(), "this user input" diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll new file mode 100644 index 000000000000..49af8b2bbf5b --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll @@ -0,0 +1,130 @@ +private import python +private import semmle.python.Concepts +private import semmle.python.ApiGraphs +private import semmle.python.dataflow.new.DataFlow +private import semmle.python.dataflow.new.RemoteFlowSources + +/** + * A data flow source of the client ip obtained according to the remote endpoint identifier specified + * (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header. + * + * For example: `request.headers.get("X-Forwarded-For")`. + */ +abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::CallCfgNode { } + +private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck { + FlaskClientSuppliedIpUsedInSecurityCheck() { + this = + API::moduleImport("flask") + .getMember("request") + .getMember("headers") + .getMember(["get", "get_all", "getlist"]) + .getACall() and + this.getArg(0).asCfgNode().getNode().(StrConst).getText().toLowerCase() = + clientIpParameterName() + } +} + +private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck { + DjangoClientSuppliedIpUsedInSecurityCheck() { + exists(RemoteFlowSource rfs, DataFlow::LocalSourceNode lsn | + rfs.getSourceType() = "django.http.request.HttpRequest" and rfs.asCfgNode() = lsn.asCfgNode() + | + lsn.flowsTo(DataFlow::exprNode(this.getFunction() + .asExpr() + .(Attribute) + .getObject() + .(Attribute) + .getObject())) and + this.getFunction().asExpr().(Attribute).getName() = "get" and + this.getFunction().asExpr().(Attribute).getObject().(Attribute).getName() in [ + "headers", "META" + ] and + this.getArg(0).asCfgNode().getNode().(StrConst).getText().toLowerCase() = + clientIpParameterName() + ) + } +} + +private string clientIpParameterName() { + result in [ + "x-forwarded-for", "x_forwarded_for", "x-real-ip", "x_real_ip", "proxy-client-ip", + "proxy_client_ip", "wl-proxy-client-ip", "wl_proxy_client_ip", "http_x_forwarded_for", + "http-x-forwarded-for", "http_x_forwarded", "http_x_cluster_client_ip", "http_client_ip", + "http_forwarded_for", "http_forwarded", "http_via", "remote_addr" + ] +} + +/** A data flow sink for ip address forgery vulnerabilities. */ +abstract class ClientSuppliedIpUsedInSecurityCheckSink extends DataFlow::Node { } + +/** A data flow sink for sql operation. */ +private class SqlOperationSink extends ClientSuppliedIpUsedInSecurityCheckSink { + SqlOperationSink() { this = any(SqlExecution e).getSql() } +} + +/** + * A data flow sink for remote client ip comparison. + * + * For example: `if not ipAddr.startswith('192.168.') : ...` determine whether the client ip starts + * with `192.168.`, and the program can be deceived by forging the ip address. + */ +private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink { + CompareSink() { + exists(Call call | + call.getFunc().(Attribute).getName() = "startswith" and + call.getArg(0).(StrConst).getText().regexpMatch(getIpAddressRegex()) and + not call.getArg(0).(StrConst).getText() = "0:0:0:0:0:0:0:1" and + call.getFunc().(Attribute).getObject() = this.asExpr() + ) + or + exists(Compare compare | + ( + compare.getOp(0) instanceof Eq or + compare.getOp(0) instanceof NotEq + ) and + ( + compare.getLeft() = this.asExpr() and + compare.getComparator(0).(StrConst).getText() instanceof PrivateHostName and + not compare.getComparator(0).(StrConst).getText() = "0:0:0:0:0:0:0:1" + or + compare.getComparator(0) = this.asExpr() and + compare.getLeft().(StrConst).getText() instanceof PrivateHostName and + not compare.getLeft().(StrConst).getText() = "0:0:0:0:0:0:0:1" + ) + ) + or + exists(Compare compare | + ( + compare.getOp(0) instanceof In or + compare.getOp(0) instanceof NotIn + ) and + ( + compare.getLeft() = this.asExpr() + or + compare.getComparator(0) = this.asExpr() + ) + ) + or + exists(Call call | + call.getFunc().(Attribute).getName() = "add" and + call.getArg(0) = this.asExpr() + ) + } +} + +string getIpAddressRegex() { + result = + "^((10\\.((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?)|(192\\.168\\.)|172\\.(1[6789]|2[0-9]|3[01])\\.)((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)$" +} + +/** + * A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary. + * Several examples are localhost, reserved IPv4 IP addresses including 127.0.0.1, 10.x.x.x, 172.16.x,x, 192.168.x,x, and reserved IPv6 addresses including [0:0:0:0:0:0:0:1] and [::1] + */ +private class PrivateHostName extends string { + bindingset[this] + PrivateHostName() { + this.regexpMatch("(?i)localhost(?:[:/?#].*)?|127\\.0\\.0\\.1(?:[:/?#].*)?|10(?:\\.[0-9]+){3}(?:[:/?#].*)?|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].*)?|192.168(?:\\.[0-9]+){2}(?:[:/?#].*)?|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].*)?|\\[?::1\\]?(?:[:/?#].*)?") + } +} diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected new file mode 100644 index 000000000000..2e6442cc17fa --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected @@ -0,0 +1,11 @@ +edges +| ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | +| ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | +nodes +| ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | +| ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | +#select +| ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | this user input | +| ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | this user input | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py new file mode 100644 index 000000000000..3edb73e575fc --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py @@ -0,0 +1,35 @@ +#!/usr/bin/env python +# -*- coding: UTF-8 -*- +""" +@Desc ：ip address spoofing +""" +from flask import Flask +from flask import request + +app = Flask(__name__) + +@app.route('/bad1') +def bad1(): + client_ip = request.headers.get('x-forwarded-for') + if not client_ip.startswith('192.168.'): + raise Exception('ip illegal') + return 'bad1' + +@app.route('/bad2') +def bad2(): + client_ip = request.headers.get('x-forwarded-for') + if not client_ip == '127.0.0.1': + raise Exception('ip illegal') + return 'bad2' + +@app.route('/good1') +def good1(): + client_ip = request.headers.get('x-forwarded-for') + client_ip = client_ip.split(',')[client_ip.split(',').length - 1] + if not client_ip == '127.0.0.1': + raise Exception('ip illegal') + return 'good1' + +if __name__ == '__main__': + app.debug = True + app.run() diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref new file mode 100644 index 000000000000..2a1775fe06aa --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref @@ -0,0 +1 @@ +experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql \ No newline at end of file From e8d08279168c4dd494acdea4baa2448fb9c25804 Mon Sep 17 00:00:00 2001 From: haby0 Date: Mon, 5 Jul 2021 10:42:15 +0800 Subject: [PATCH 2/8] Add tornado source --- ...ClientSuppliedIpUsedInSecurityCheckLib.qll | 21 +++++++++++ ...ientSuppliedIpUsedInSecurityCheck.expected | 20 ++++++----- ...dIpUsedInSecurityCheck.py => flask_bad.py} | 8 ----- .../Security/CWE-348/flask_good.py | 21 +++++++++++ .../Security/CWE-348/tornado_bad.py | 36 +++++++++++++++++++ 5 files changed, 90 insertions(+), 16 deletions(-) rename python/ql/test/experimental/query-tests/Security/CWE-348/{ClientSuppliedIpUsedInSecurityCheck.py => flask_bad.py} (69%) create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-348/flask_good.py create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-348/tornado_bad.py diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll index 49af8b2bbf5b..8113fd17c9df 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll @@ -46,6 +46,27 @@ private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIp } } +private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck { + TornadoClientSuppliedIpUsedInSecurityCheck() { + exists(RemoteFlowSource rfs, DataFlow::LocalSourceNode lsn | + rfs.getSourceType() = "tornado.web.RequestHandler" and rfs.asCfgNode() = lsn.asCfgNode() + | + lsn.flowsTo(DataFlow::exprNode(this.getFunction() + .asExpr() + .(Attribute) + .getObject() + .(Attribute) + .getObject() + .(Attribute) + .getObject())) and + this.getFunction().asExpr().(Attribute).getName() in ["get", "get_list"] and + this.getFunction().asExpr().(Attribute).getObject().(Attribute).getName() = "headers" and + this.getArg(0).asCfgNode().getNode().(StrConst).getText().toLowerCase() = + clientIpParameterName() + ) + } +} + private string clientIpParameterName() { result in [ "x-forwarded-for", "x_forwarded_for", "x-real-ip", "x_real_ip", "proxy-client-ip", diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected index 2e6442cc17fa..38536f2b7a20 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected @@ -1,11 +1,15 @@ edges -| ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | -| ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | +| flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | +| flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | +| tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | nodes -| ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | -| ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | +| flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | +| flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | +| tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | #select -| ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:14:12:14:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.py:13:17:13:54 | ControlFlowNode for Attribute() | this user input | -| ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | ClientSuppliedIpUsedInSecurityCheck.py:21:12:21:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.py:20:17:20:54 | ControlFlowNode for Attribute() | this user input | +| flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | this user input | +| flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | this user input | +| tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | this user input | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py b/python/ql/test/experimental/query-tests/Security/CWE-348/flask_bad.py similarity index 69% rename from python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py rename to python/ql/test/experimental/query-tests/Security/CWE-348/flask_bad.py index 3edb73e575fc..b357a9316fd0 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/flask_bad.py @@ -22,14 +22,6 @@ def bad2(): raise Exception('ip illegal') return 'bad2' -@app.route('/good1') -def good1(): - client_ip = request.headers.get('x-forwarded-for') - client_ip = client_ip.split(',')[client_ip.split(',').length - 1] - if not client_ip == '127.0.0.1': - raise Exception('ip illegal') - return 'good1' - if __name__ == '__main__': app.debug = True app.run() diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/flask_good.py b/python/ql/test/experimental/query-tests/Security/CWE-348/flask_good.py new file mode 100644 index 000000000000..da42c59724a8 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/flask_good.py @@ -0,0 +1,21 @@ +#!/usr/bin/env python +# -*- coding: UTF-8 -*- +""" +@Desc ：ip address spoofing +""" +from flask import Flask +from flask import request + +app = Flask(__name__) + +@app.route('/good1') +def good1(): + client_ip = request.headers.get('x-forwarded-for') + client_ip = client_ip.split(',')[len(client_ip.split(',')) - 1] + if not client_ip == '127.0.0.1': + raise Exception('ip illegal') + return 'good1' + +if __name__ == '__main__': + app.debug = True + app.run() diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/tornado_bad.py b/python/ql/test/experimental/query-tests/Security/CWE-348/tornado_bad.py new file mode 100644 index 000000000000..23ad29d8b09a --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/tornado_bad.py @@ -0,0 +1,36 @@ +#!/usr/bin/env python +# -*- coding: UTF-8 -*- +""" +@Desc ：ip address spoofing +""" +import tornado.httpserver +import tornado.options +import tornado.web +import tornado.ioloop + +from tornado.options import define, options + +define("port", default=8000, help="run on the given port,default 8000", type=int) + + +class IndexHandler(tornado.web.RequestHandler): + def get(self): + client_ip = self.request.headers.get('x-forwarded-for') + if client_ip: + client_ip = client_ip.split(',')[len(client_ip.split(',')) - 1] + else: + client_ip = self.request.headers.get('REMOTE_ADDR', None) + if not client_ip == '127.0.0.1': + raise Exception('ip illegal') + self.write("hello.") + +handlers = [(r"/", IndexHandler)] + +if __name__ == "__main__": + tornado.options.parse_command_line() + app = tornado.web.Application( + handlers + ) + http_server = tornado.httpserver.HTTPServer(app) + http_server.listen(options.port) + tornado.ioloop.IOLoop.instance().start() From 9e63aa9d8430c214c5c8bb6f7080c291be18a814 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 14 Sep 2021 21:12:49 +0800 Subject: [PATCH 3/8] Update query --- .../CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql | 9 +++++++++ .../CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll | 8 ++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql index 7866fbd681c6..1c96a10d5317 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql @@ -13,6 +13,7 @@ import python import semmle.python.dataflow.new.DataFlow import semmle.python.dataflow.new.TaintTracking +import semmle.python.ApiGraphs import ClientSuppliedIpUsedInSecurityCheckLib import DataFlow::PathGraph @@ -30,6 +31,14 @@ class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configura sink instanceof ClientSuppliedIpUsedInSecurityCheckSink } + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::CallCfgNode ccn | + ccn = API::moduleImport("netaddr").getMember("IPAddress").getACall() and + ccn.getArg(0) = pred and + ccn = succ + ) + } + override predicate isSanitizer(DataFlow::Node node) { exists(Subscript ss | not ss.getIndex().(IntegerLiteral).getText() = "0" and diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll index 8113fd17c9df..b003a188b1a1 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll @@ -123,14 +123,10 @@ private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink { ( compare.getLeft() = this.asExpr() or - compare.getComparator(0) = this.asExpr() + compare.getComparator(0) = this.asExpr() and + not compare.getLeft().(StrConst).getText() in ["%", ","] ) ) - or - exists(Call call | - call.getFunc().(Attribute).getName() = "add" and - call.getArg(0) = this.asExpr() - ) } } From 02776017055491f0e81143e1f2907991689882ea Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 16 Sep 2021 20:59:34 +0800 Subject: [PATCH 4/8] Eliminate false positives caused by `.` --- .../Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll index b003a188b1a1..82da4f5b46f0 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll @@ -124,7 +124,7 @@ private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink { compare.getLeft() = this.asExpr() or compare.getComparator(0) = this.asExpr() and - not compare.getLeft().(StrConst).getText() in ["%", ","] + not compare.getLeft().(StrConst).getText() in ["%", ",", "."] ) ) } From 9b969e15fc82e878ade65c93805792af519ccb04 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 24 Sep 2021 12:56:10 +0800 Subject: [PATCH 5/8] Modify according to @yoff suggestion --- .../ClientSuppliedIpUsedInSecurityCheck.ql | 4 +- ...ClientSuppliedIpUsedInSecurityCheckLib.qll | 85 ++++++++++--------- 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql index 1c96a10d5317..270a0444cfbd 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql @@ -27,9 +27,7 @@ class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configura source instanceof ClientSuppliedIpUsedInSecurityCheck } - override predicate isSink(DataFlow::Node sink) { - sink instanceof ClientSuppliedIpUsedInSecurityCheckSink - } + override predicate isSink(DataFlow::Node sink) { sink instanceof PossibleSecurityCheck } override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { exists(DataFlow::CallCfgNode ccn | diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll index 82da4f5b46f0..97e9bbfd4578 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll @@ -14,55 +14,60 @@ abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::CallCfgNode private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck { FlaskClientSuppliedIpUsedInSecurityCheck() { - this = - API::moduleImport("flask") - .getMember("request") - .getMember("headers") - .getMember(["get", "get_all", "getlist"]) - .getACall() and - this.getArg(0).asCfgNode().getNode().(StrConst).getText().toLowerCase() = - clientIpParameterName() + exists(RemoteFlowSource rfs, DataFlow::AttrRead get | + rfs.getSourceType() = "flask.request" and this.getFunction() = get + | + // `get` is a call to request.headers.get or request.headers.get_all or request.headers.getlist + // request.headers + get.getObject() + .(DataFlow::AttrRead) + // request + .getObject() + .getALocalSource() = rfs and + get.getAttributeName() in ["get", "get_all", "getlist"] and + get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and + this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName() + ) } } private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck { DjangoClientSuppliedIpUsedInSecurityCheck() { - exists(RemoteFlowSource rfs, DataFlow::LocalSourceNode lsn | - rfs.getSourceType() = "django.http.request.HttpRequest" and rfs.asCfgNode() = lsn.asCfgNode() + exists(RemoteFlowSource rfs, DataFlow::AttrRead get | + rfs.getSourceType() = "django.http.request.HttpRequest" and this.getFunction() = get | - lsn.flowsTo(DataFlow::exprNode(this.getFunction() - .asExpr() - .(Attribute) - .getObject() - .(Attribute) - .getObject())) and - this.getFunction().asExpr().(Attribute).getName() = "get" and - this.getFunction().asExpr().(Attribute).getObject().(Attribute).getName() in [ - "headers", "META" - ] and - this.getArg(0).asCfgNode().getNode().(StrConst).getText().toLowerCase() = - clientIpParameterName() + // `get` is a call to request.headers.get or request.META.get + // request.headers + get.getObject() + .(DataFlow::AttrRead) + // request + .getObject() + .getALocalSource() = rfs and + get.getAttributeName() = "get" and + get.getObject().(DataFlow::AttrRead).getAttributeName() in ["headers", "META"] and + this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName() ) } } private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck { TornadoClientSuppliedIpUsedInSecurityCheck() { - exists(RemoteFlowSource rfs, DataFlow::LocalSourceNode lsn | - rfs.getSourceType() = "tornado.web.RequestHandler" and rfs.asCfgNode() = lsn.asCfgNode() + exists(RemoteFlowSource rfs, DataFlow::AttrRead get | + rfs.getSourceType() = "tornado.web.RequestHandler" and this.getFunction() = get | - lsn.flowsTo(DataFlow::exprNode(this.getFunction() - .asExpr() - .(Attribute) - .getObject() - .(Attribute) - .getObject() - .(Attribute) - .getObject())) and - this.getFunction().asExpr().(Attribute).getName() in ["get", "get_list"] and - this.getFunction().asExpr().(Attribute).getObject().(Attribute).getName() = "headers" and - this.getArg(0).asCfgNode().getNode().(StrConst).getText().toLowerCase() = - clientIpParameterName() + // `get` is a call to `rfs`.request.headers.get + // `rfs`.request.headers + get.getObject() + .(DataFlow::AttrRead) + // `rfs`.request + .getObject() + .(DataFlow::AttrRead) + // `rfs` + .getObject() + .getALocalSource() = rfs and + get.getAttributeName() in ["get", "get_list"] and + get.getObject().(DataFlow::AttrRead).getAttributeName() = "headers" and + this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName() ) } } @@ -77,11 +82,11 @@ private string clientIpParameterName() { } /** A data flow sink for ip address forgery vulnerabilities. */ -abstract class ClientSuppliedIpUsedInSecurityCheckSink extends DataFlow::Node { } +abstract class PossibleSecurityCheck extends DataFlow::Node { } /** A data flow sink for sql operation. */ -private class SqlOperationSink extends ClientSuppliedIpUsedInSecurityCheckSink { - SqlOperationSink() { this = any(SqlExecution e).getSql() } +private class SqlOperationAsSecurityCheck extends PossibleSecurityCheck { + SqlOperationAsSecurityCheck() { this = any(SqlExecution e).getSql() } } /** @@ -90,7 +95,7 @@ private class SqlOperationSink extends ClientSuppliedIpUsedInSecurityCheckSink { * For example: `if not ipAddr.startswith('192.168.') : ...` determine whether the client ip starts * with `192.168.`, and the program can be deceived by forging the ip address. */ -private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink { +private class CompareSink extends PossibleSecurityCheck { CompareSink() { exists(Call call | call.getFunc().(Attribute).getName() = "startswith" and From a17b0d4e5c84bcc22e8b97fc2fe3956d67f93fc7 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 5 Oct 2021 17:12:04 +0800 Subject: [PATCH 6/8] Modify Sanitizer --- .../Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql index 270a0444cfbd..9f840132fa3f 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql @@ -39,10 +39,9 @@ class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configura override predicate isSanitizer(DataFlow::Node node) { exists(Subscript ss | - not ss.getIndex().(IntegerLiteral).getText() = "0" and ss.getObject().(Call).getFunc().(Attribute).getName() = "split" and - ss.getObject().(Call).getArg(0).(StrConst).getText() = "," and - ss.getObject().(Call).getFunc().(Attribute).getObject() = node.asExpr() + ss.getObject().(Call).getAnArg().(StrConst).getText() = "," and + ss = node.asExpr() ) } } From 538bf7c3219a00b62f54b4e29d666fd60b519a82 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 7 Oct 2021 19:44:25 +0800 Subject: [PATCH 7/8] Update python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql Co-authored-by: yoff --- .../Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql index 9f840132fa3f..af2d6f8bc162 100644 --- a/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql +++ b/python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql @@ -38,7 +38,9 @@ class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configura } override predicate isSanitizer(DataFlow::Node node) { + // `client_supplied_ip.split(",")[n]` for `n` > 0 exists(Subscript ss | + not ss.getIndex().(IntegerLiteral).getText() = "0" and ss.getObject().(Call).getFunc().(Attribute).getName() = "split" and ss.getObject().(Call).getAnArg().(StrConst).getText() = "," and ss = node.asExpr() From c2d0fcfbe6f6e4a9e69ebdf78f768cdf4939ea61 Mon Sep 17 00:00:00 2001 From: haby0 Date: Mon, 11 Oct 2021 16:46:02 +0800 Subject: [PATCH 8/8] Update python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected Co-authored-by: yoff --- .../CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected index 38536f2b7a20..a432cf5053f9 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected @@ -9,6 +9,7 @@ nodes | flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | | tornado_bad.py:22:25:22:69 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | tornado_bad.py:23:16:23:24 | ControlFlowNode for client_ip | semmle.label | ControlFlowNode for client_ip | +subpaths #select | flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | flask_bad.py:14:12:14:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | flask_bad.py:13:17:13:54 | ControlFlowNode for Attribute() | this user input | | flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | flask_bad.py:21:12:21:20 | ControlFlowNode for client_ip | IP address spoofing might include code from $@. | flask_bad.py:20:17:20:54 | ControlFlowNode for Attribute() | this user input |