|
| 1 | +/** |
| 2 | + * @name Arbitrary file write during tarfile extraction |
| 3 | + * @description Extracting files from a malicious tar archive without validating that the |
| 4 | + * destination file path is within the destination directory can cause files outside |
| 5 | + * the destination directory to be overwritten. |
| 6 | + * @kind path-problem |
| 7 | + * @id py/tarslip |
| 8 | + * @problem.severity error |
| 9 | + * @security-severity 7.5 |
| 10 | + * @precision high |
| 11 | + * @tags security |
| 12 | + * external/cwe/cwe-022 |
| 13 | + */ |
| 14 | + |
| 15 | +import python |
| 16 | +import semmle.python.dataflow.new.DataFlow |
| 17 | +import semmle.python.dataflow.new.TaintTracking |
| 18 | +import DataFlow::PathGraph |
| 19 | +import semmle.python.ApiGraphs |
| 20 | +import semmle.python.dataflow.new.internal.Attributes |
| 21 | +import semmle.python.dataflow.new.BarrierGuards |
| 22 | +import semmle.python.dataflow.new.RemoteFlowSources |
| 23 | + |
| 24 | +/** |
| 25 | + * Handle those three cases of Tarfile opens: |
| 26 | + * - `tarfile.open()` |
| 27 | + * - `tarfile.TarFile()` |
| 28 | + * - `MKtarfile.Tarfile.open()` |
| 29 | + */ |
| 30 | +API::Node tarfileOpen() { |
| 31 | + result in [ |
| 32 | + API::moduleImport("tarfile").getMember(["open", "TarFile"]), |
| 33 | + API::moduleImport("tarfile").getMember("TarFile").getASubclass().getMember("open") |
| 34 | + ] |
| 35 | +} |
| 36 | + |
| 37 | +/** |
| 38 | + * Handle the previous three cases, plus the use of `closing` in the previous cases |
| 39 | + */ |
| 40 | +class AllTarfileOpens extends API::CallNode { |
| 41 | + AllTarfileOpens() { |
| 42 | + this = tarfileOpen().getACall() |
| 43 | + or |
| 44 | + exists(API::Node closing, Node arg | |
| 45 | + closing = API::moduleImport("contextlib").getMember("closing") and |
| 46 | + this = closing.getACall() and |
| 47 | + arg = this.getArg(0) and |
| 48 | + arg = tarfileOpen().getACall() |
| 49 | + ) |
| 50 | + } |
| 51 | +} |
| 52 | + |
| 53 | +/** |
| 54 | + * A taint-tracking configuration for detecting more "TarSlip" vulnerabilities. |
| 55 | + */ |
| 56 | +class Configuration extends TaintTracking::Configuration { |
| 57 | + Configuration() { this = "TarSlip" } |
| 58 | + |
| 59 | + override predicate isSource(DataFlow::Node source) { source = tarfileOpen().getACall() } |
| 60 | + |
| 61 | + override predicate isSink(DataFlow::Node sink) { |
| 62 | + ( |
| 63 | + // A sink capturing method calls to `extractall` without `members` argument. |
| 64 | + // For a call to `file.extractall` without `members` argument, `file` is considered a sink. |
| 65 | + exists(MethodCallNode call, AllTarfileOpens atfo | |
| 66 | + call = atfo.getReturn().getMember("extractall").getACall() and |
| 67 | + not exists(Node arg | arg = call.getArgByName("members")) and |
| 68 | + sink = call.getObject() |
| 69 | + ) |
| 70 | + or |
| 71 | + // A sink capturing method calls to `extractall` with `members` argument. |
| 72 | + // For a call to `file.extractall` with `members` argument, `file` is considered a sink if not |
| 73 | + // a the `members` argument contains a NameConstant as None, a List or call to the method `getmembers`. |
| 74 | + // Otherwise, the argument of `members` is considered a sink. |
| 75 | + exists(MethodCallNode call, Node arg, AllTarfileOpens atfo | |
| 76 | + call = atfo.getReturn().getMember("extractall").getACall() and |
| 77 | + arg = call.getArgByName("members") and |
| 78 | + if |
| 79 | + arg.asCfgNode() instanceof NameConstantNode or |
| 80 | + arg.asCfgNode() instanceof ListNode |
| 81 | + then sink = call.getObject() |
| 82 | + else |
| 83 | + if arg.(MethodCallNode).getMethodName() = "getmembers" |
| 84 | + then sink = arg.(MethodCallNode).getObject() |
| 85 | + else sink = call.getArgByName("members") |
| 86 | + ) |
| 87 | + or |
| 88 | + // An argument to `extract` is considered a sink. |
| 89 | + exists(AllTarfileOpens atfo | |
| 90 | + sink = atfo.getReturn().getMember("extract").getACall().getArg(0) |
| 91 | + ) |
| 92 | + or |
| 93 | + //An argument to `_extract_member` is considered a sink. |
| 94 | + exists(MethodCallNode call, AllTarfileOpens atfo | |
| 95 | + call = atfo.getReturn().getMember("_extract_member").getACall() and |
| 96 | + call.getArg(1).(AttrRead).accesses(sink, "name") |
| 97 | + ) |
| 98 | + ) and |
| 99 | + not sink.getScope().getLocation().getFile().inStdlib() |
| 100 | + } |
| 101 | + |
| 102 | + override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { |
| 103 | + exists(AttrRead attr, MethodCallNode call | |
| 104 | + attr.accesses(nodeFrom, "getmembers") and |
| 105 | + nodeFrom = call.getObject() and |
| 106 | + nodeFrom instanceof AllTarfileOpens and |
| 107 | + nodeTo = call |
| 108 | + ) |
| 109 | + or |
| 110 | + exists(API::CallNode closing | |
| 111 | + closing = API::moduleImport("contextlib").getMember("closing").getACall() and |
| 112 | + nodeFrom = closing.getArg(0) and |
| 113 | + nodeFrom = tarfileOpen().getReturn().getAValueReachingSink() and |
| 114 | + nodeTo = closing |
| 115 | + ) |
| 116 | + } |
| 117 | +} |
| 118 | + |
| 119 | +from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink |
| 120 | +where config.hasFlowPath(source, sink) |
| 121 | +select sink, source, sink, "Extraction of tarfile from $@ to a potentially untrusted source $@.", |
| 122 | + source.getNode(), source.getNode().toString(), sink.getNode(), sink.getNode().toString() |
0 commit comments