From f86570f6e7fe55d0a497eb4d935de7c05b85d6e2 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Mon, 24 Jun 2024 03:40:15 +0530 Subject: [PATCH] WIP: Python: CORS Bypass This PR adds a query to detect a Cross Origin Resource Sharing(CORS) policy bypass due to an incorrect check. This PR attempts to detect the vulnerability pattern found in CVE-2022-3457 ```python if request.method in ['POST', 'PUT', 'PATCH', 'DELETE']: origin = request.headers.get('Origin', None) if origin and not origin.startswith(request.base): raise cherrypy.HTTPError(403, 'Unexpected Origin header') ``` In this case, a value obtained from a header is compared using `startswith` call. This comparision is easily bypassed resulting in a CORS bypass. Given that similar bugs have been found in other languages as well, I think this PR would be a great addition to the exisitng python query pack. The databases for CVE-2022-3457 can be downloaded from ``` https://filetransfer.io/data-package/i4Mfepls#link https://file.io/V67T4SSgmExF ``` --- .../experimental/Security/CWE-346/CorsBad.py | 9 ++ .../Security/CWE-346/CorsBypass.qhelp | 28 ++++++ .../Security/CWE-346/CorsBypass.ql | 97 +++++++++++++++++++ .../experimental/Security/CWE-346/CorsGood.py | 9 ++ .../query-tests/Security/CWE-346/Cors.py | 17 ++++ .../Security/CWE-346/CorsBypass.expected | 13 +++ .../Security/CWE-346/CorsBypass.qlref | 1 + 7 files changed, 174 insertions(+) create mode 100644 python/ql/src/experimental/Security/CWE-346/CorsBad.py create mode 100644 python/ql/src/experimental/Security/CWE-346/CorsBypass.qhelp create mode 100644 python/ql/src/experimental/Security/CWE-346/CorsBypass.ql create mode 100644 python/ql/src/experimental/Security/CWE-346/CorsGood.py create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-346/Cors.py create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.expected create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.qlref diff --git a/python/ql/src/experimental/Security/CWE-346/CorsBad.py b/python/ql/src/experimental/Security/CWE-346/CorsBad.py new file mode 100644 index 000000000000..74bd721b8cab --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-346/CorsBad.py @@ -0,0 +1,9 @@ +import cherrypy + +def bad(): + request = cherrypy.request + validCors = "domain.com" + if request.method in ['POST', 'PUT', 'PATCH', 'DELETE']: + origin = request.headers.get('Origin', None) + if origin.startswith(validCors): + print("Origin Valid") \ No newline at end of file diff --git a/python/ql/src/experimental/Security/CWE-346/CorsBypass.qhelp b/python/ql/src/experimental/Security/CWE-346/CorsBypass.qhelp new file mode 100644 index 000000000000..5082fcc23aae --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-346/CorsBypass.qhelp @@ -0,0 +1,28 @@ + + + +

Cross-origin resource sharing policy may be bypassed due to incorrect checks like the string.startswith call.

+ + +

Use a more stronger check to test for CORS policy bypass.

+ + + +

Most Python frameworks provide a mechanism for testing origins and performing CORS checks. + For example, consider the code snippet below, origin is compared using a + startswith call against a list of whitelisted origins. This check can be bypassed + easily by origin like domain.com.baddomain.com +

+ +

This can be prevented by comparing the origin in a manner shown below. +

+ + + + + +
  • PortsSwigger : Cross-origin resource + sharing (CORS)
    • +
  • Related CVE: CVE-2022-3457.
    • +     +     \ No newline at end of file diff --git a/python/ql/src/experimental/Security/CWE-346/CorsBypass.ql b/python/ql/src/experimental/Security/CWE-346/CorsBypass.ql new file mode 100644 index 000000000000..4b79b97ff4a6 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-346/CorsBypass.ql @@ -0,0 +1,97 @@ +/** + * @name Cross Origin Resource Sharing(CORS) Policy Bypass + * @description Checking user supplied origin headers using weak comparators like 'string.startswith' may lead to CORS policy bypass. + * @kind path-problem + * @problem.severity warning + * @id py/cors-bypass + * @tags security + * externa/cwe/CWE-346 + */ + +import python +import semmle.python.ApiGraphs +import semmle.python.dataflow.new.TaintTracking +import semmle.python.Flow +import semmle.python.dataflow.new.RemoteFlowSources + +/** + * Returns true if the control flow node may be useful in the current context. + * + * Ideally for more completeness, we should alert on every `startswith` call and every remote flow source which gets partailly checked. But, as this can lead to lots of FPs, we apply heuristics to filter some calls. This predicate provides logic for this filteration. + */ +private predicate maybeInteresting(ControlFlowNode c) { + // Check if the name of the variable which calls the function matches the heuristic. + // This would typically occur at the sink. + // This should deal with cases like + // `origin.startswith("bla")` + heuristics(c.(CallNode).getFunction().(AttrNode).getObject().(NameNode).getId()) + or + // Check if the name of the variable passed as an argument to the functions matches the heuristic. This would typically occur at the sink. + // This should deal with cases like + // `bla.startswith(origin)` + heuristics(c.(CallNode).getArg(0).(NameNode).getId()) + or + // Check if the value gets written to any interesting variable. This would typically occur at the source. + // This should deal with cases like + // `origin = request.headers.get('My-custom-header')` + exists(Variable v | heuristics(v.getId()) | c.getASuccessor*().getNode() = v.getAStore()) +} + +private class StringStartswithCall extends ControlFlowNode { + StringStartswithCall() { this.(CallNode).getFunction().(AttrNode).getName() = "startswith" } +} + +bindingset[s] +predicate heuristics(string s) { s.matches(["%origin%", "%cors%"]) } + +/** + * A member of the `cherrypy.request` class taken as a `RemoteFlowSource`. + */ +class CherryPyRequest extends RemoteFlowSource::Range { + CherryPyRequest() { + this = + API::moduleImport("cherrypy") + .getMember("request") + .getMember([ + "charset", "content_type", "filename", "fp", "name", "params", "headers", "length", + ]) + .asSource() + } + + override string getSourceType() { result = "cherrypy.request" } +} + +module CorsBypassConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } + + predicate isSink(DataFlow::Node node) { + exists(StringStartswithCall s | + node.asCfgNode() = s.(CallNode).getArg(0) or + node.asCfgNode() = s.(CallNode).getFunction().(AttrNode).getObject() + ) + } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(API::CallNode c, API::Node n | + n = API::moduleImport("cherrypy").getMember("request").getMember("headers") and + c = n.getMember("get").getACall() + | + c.getReturn().asSource() = node2 and n.asSource() = node1 + ) + } +} + +module CorsFlow = TaintTracking::Global; + +import CorsFlow::PathGraph + +from CorsFlow::PathNode source, CorsFlow::PathNode sink +where + CorsFlow::flowPath(source, sink) and + ( + maybeInteresting(source.getNode().asCfgNode()) + or + maybeInteresting(sink.getNode().asCfgNode()) + ) +select sink, source, sink, + "Potentially incorrect string comparison which could lead to a CORS bypass." diff --git a/python/ql/src/experimental/Security/CWE-346/CorsGood.py b/python/ql/src/experimental/Security/CWE-346/CorsGood.py new file mode 100644 index 000000000000..7cd4039c0c39 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-346/CorsGood.py @@ -0,0 +1,9 @@ +import cherrypy + +def good(): + request = cherrypy.request + validOrigin = "domain.com" + if request.method in ['POST', 'PUT', 'PATCH', 'DELETE']: + origin = request.headers.get('Origin', None) + if origin == validOrigin: + print("Origin Valid") \ No newline at end of file diff --git a/python/ql/test/experimental/query-tests/Security/CWE-346/Cors.py b/python/ql/test/experimental/query-tests/Security/CWE-346/Cors.py new file mode 100644 index 000000000000..cc12e1273fbb --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-346/Cors.py @@ -0,0 +1,17 @@ +import cherrypy + +def bad(): + request = cherrypy.request + validCors = "domain.com" + if request.method in ['POST', 'PUT', 'PATCH', 'DELETE']: + origin = request.headers.get('Origin', None) + if origin.startswith(validCors): + print("Origin Valid") + +def good(): + request = cherrypy.request + validOrigin = "domain.com" + if request.method in ['POST', 'PUT', 'PATCH', 'DELETE']: + origin = request.headers.get('Origin', None) + if origin == validOrigin: + print("Origin Valid") \ No newline at end of file diff --git a/python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.expected b/python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.expected new file mode 100644 index 000000000000..32d807c6f6e5 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.expected @@ -0,0 +1,13 @@ +edges +| Cors.py:7:9:7:14 | ControlFlowNode for origin | Cors.py:8:12:8:17 | ControlFlowNode for origin | provenance | | +| Cors.py:7:18:7:32 | ControlFlowNode for Attribute | Cors.py:7:18:7:52 | ControlFlowNode for Attribute() | provenance | Config | +| Cors.py:7:18:7:32 | ControlFlowNode for Attribute | Cors.py:7:18:7:52 | ControlFlowNode for Attribute() | provenance | dict.get | +| Cors.py:7:18:7:52 | ControlFlowNode for Attribute() | Cors.py:7:9:7:14 | ControlFlowNode for origin | provenance | | +nodes +| Cors.py:7:9:7:14 | ControlFlowNode for origin | semmle.label | ControlFlowNode for origin | +| Cors.py:7:18:7:32 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| Cors.py:7:18:7:52 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| Cors.py:8:12:8:17 | ControlFlowNode for origin | semmle.label | ControlFlowNode for origin | +subpaths +#select +| Cors.py:8:12:8:17 | ControlFlowNode for origin | Cors.py:7:18:7:32 | ControlFlowNode for Attribute | Cors.py:8:12:8:17 | ControlFlowNode for origin | Potentially incorrect string comparison which could lead to a CORS bypass. | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.qlref b/python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.qlref new file mode 100644 index 000000000000..b652fd93088b --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-346/CorsBypass.qlref @@ -0,0 +1 @@ +experimental/Security/CWE-346/CorsBypass.ql \ No newline at end of file