Skip to content

Commit 1a8c9ab

Browse files
Sim4n6Sim4n6
committed
Incorporate Sink & Source as steps from TarSlipQry
1 parent 7079def commit 1a8c9ab

File tree

2 files changed

+161
-3
lines changed

2 files changed

+161
-3
lines changed

‎python/ql/src/experimental/Security/UnsafeUnpackQuery.qll

Lines changed: 84 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,35 @@ import semmle.python.dataflow.new.TaintTracking
1010
import semmle.python.frameworks.Stdlib
1111
import semmle.python.dataflow.new.RemoteFlowSources
1212

13+
/**
14+
* Handle those three cases of Tarfile opens:
15+
* - `tarfile.open()`
16+
* - `tarfile.TarFile()`
17+
* - `MKtarfile.Tarfile.open()`
18+
*/
19+
API::Node tarfileOpen() {
20+
result in [
21+
API::moduleImport("tarfile").getMember(["open", "TarFile"]),
22+
API::moduleImport("tarfile").getMember("TarFile").getASubclass().getMember("open")
23+
]
24+
}
25+
26+
/**
27+
* Handle the previous three cases, plus the use of `closing` in the previous cases
28+
*/
29+
class AllTarfileOpens extends API::CallNode {
30+
AllTarfileOpens() {
31+
this = tarfileOpen().getACall()
32+
or
33+
exists(API::Node closing, Node arg |
34+
closing = API::moduleImport("contextlib").getMember("closing") and
35+
this = closing.getACall() and
36+
arg = this.getArg(0) and
37+
arg = tarfileOpen().getACall()
38+
)
39+
}
40+
}
41+
1342
class UnsafeUnpackingConfig extends TaintTracking::Configuration {
1443
UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" }
1544

@@ -68,8 +97,47 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration {
6897
}
6998

7099
override predicate isSink(DataFlow::Node sink) {
71-
// A sink capturing method calls to `unpack_archive`.
72-
sink = API::moduleImport("shutil").getMember("unpack_archive").getACall().getArg(0)
100+
(
101+
// A sink capturing method calls to `unpack_archive`.
102+
sink = API::moduleImport("shutil").getMember("unpack_archive").getACall().getArg(0)
103+
or
104+
// A sink capturing method calls to `extractall` without `members` argument.
105+
// For a call to `file.extractall` without `members` argument, `file` is considered a sink.
106+
exists(MethodCallNode call, AllTarfileOpens atfo |
107+
call = atfo.getReturn().getMember("extractall").getACall() and
108+
not exists(Node arg | arg = call.getArgByName("members")) and
109+
sink = call.getObject()
110+
)
111+
or
112+
// A sink capturing method calls to `extractall` with `members` argument.
113+
// For a call to `file.extractall` with `members` argument, `file` is considered a sink if not
114+
// a the `members` argument contains a NameConstant as None, a List or call to the method `getmembers`.
115+
// Otherwise, the argument of `members` is considered a sink.
116+
exists(MethodCallNode call, Node arg, AllTarfileOpens atfo |
117+
call = atfo.getReturn().getMember("extractall").getACall() and
118+
arg = call.getArgByName("members") and
119+
if
120+
arg.asCfgNode() instanceof NameConstantNode or
121+
arg.asCfgNode() instanceof ListNode
122+
then sink = call.getObject()
123+
else
124+
if arg.(MethodCallNode).getMethodName() = "getmembers"
125+
then sink = arg.(MethodCallNode).getObject()
126+
else sink = call.getArgByName("members")
127+
)
128+
or
129+
// An argument to `extract` is considered a sink.
130+
exists(AllTarfileOpens atfo |
131+
sink = atfo.getReturn().getMember("extract").getACall().getArg(0)
132+
)
133+
or
134+
//An argument to `_extract_member` is considered a sink.
135+
exists(MethodCallNode call, AllTarfileOpens atfo |
136+
call = atfo.getReturn().getMember("_extract_member").getACall() and
137+
call.getArg(1).(AttrRead).accesses(sink, "name")
138+
)
139+
) and
140+
not sink.getScope().getLocation().getFile().inStdlib()
73141
}
74142

75143
override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
@@ -119,5 +187,19 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration {
119187
// Join the base_dir to the filename
120188
nodeTo = API::moduleImport("os").getMember("path").getMember("join").getACall() and
121189
nodeFrom = nodeTo.(API::CallNode).getArg(1)
190+
or
191+
// Go through an Open for a Tarfile
192+
nodeTo = tarfileOpen().getACall() and nodeFrom = nodeTo.(MethodCallNode).getArg(0)
193+
or
194+
// Handle the case where the getmembers is used.
195+
nodeTo.(MethodCallNode).calls(nodeFrom, "getmembers") and
196+
nodeFrom instanceof AllTarfileOpens
197+
or
198+
// To handle the case of `with closing(tarfile.open()) as file:`
199+
// we add a step from the first argument of `closing` to the call to `closing`,
200+
// whenever that first argument is a return of `tarfile.open()`.
201+
nodeTo = API::moduleImport("contextlib").getMember("closing").getACall() and
202+
nodeFrom = nodeTo.(API::CallNode).getArg(0) and
203+
nodeFrom = tarfileOpen().getReturn().getAValueReachableFromSource()
122204
}
123205
}

‎python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,4 +122,80 @@ def simple_upload(request):
122122
return render(request, 'simple_upload.html')
123123

124124
elif request.method == 'GET':
125-
return render(request, 'simple_upload.html')
125+
return render(request, 'simple_upload.html')
126+
127+
128+
import shutil
129+
import os
130+
import tarfile
131+
import tempfile
132+
import argparse
133+
134+
parser = argparse.ArgumentParser(description='Process some integers.')
135+
parser.add_argument('integers', metavar='N', type=int, nargs='+',
136+
help='an integer for the accumulator')
137+
parser.add_argument('filename', help='filename to be provided')
138+
139+
args = parser.parse_args()
140+
unsafe_filename_tar = args.filename
141+
with tarfile.TarFile(unsafe_filename_tar, mode="r") as tar:
142+
tar.extractall(path="/tmp/unpack/", members=tar) # $result=BAD
143+
tar = tarfile.open(unsafe_filename_tar)
144+
145+
146+
from django.shortcuts import render
147+
from django.core.files.storage import FileSystemStorage
148+
import shutil
149+
150+
def simple_upload(request):
151+
152+
base_dir = "/tmp/baase_dir"
153+
if request.method == 'POST':
154+
# Read uploaded files by chunks of data
155+
# see chunks(): https://docs.djangoproject.com/en/4.1/ref/files/uploads/#django.core.files.uploadedfile.UploadedFile.chunks
156+
savepath = os.path.join(base_dir, "tarball_compressed.tar.gz")
157+
with open(savepath, 'wb+') as wfile:
158+
for chunk in request.FILES["ufile1"].chunks():
159+
wfile.write(chunk)
160+
161+
tar = tarfile.open(savepath)
162+
result = []
163+
for member in tar:
164+
if member.issym():
165+
raise ValueError("But it is a symlink")
166+
result.append(member)
167+
tar.extractall(path=tempfile.mkdtemp(), members=result) # $result=BAD
168+
tar.close()
169+
170+
171+
response = requests.get(url_filename, stream=True)
172+
tarpath = "/tmp/tmp456/tarball.tar.gz"
173+
with open(tarpath, "wb") as f:
174+
f.write(response.raw.read())
175+
target_dir = "/tmp/unpack"
176+
tarfile.TarFile(tarpath, mode="r").extractall(path=target_dir) # $result=BAD
177+
178+
179+
from pathlib import Path
180+
import tempfile
181+
import boto3
182+
183+
def default_session() -> boto3.Session:
184+
_SESSION = None
185+
if _SESSION is None:
186+
_SESSION = boto3.Session()
187+
return _SESSION
188+
189+
cache = False
190+
cache_dir = "/tmp/artifacts"
191+
object_path = "/objects/obj1"
192+
s3 = default_session().client("s3")
193+
with tempfile.NamedTemporaryFile(suffix=".tar.gz") as tmp:
194+
s3.download_fileobj(bucket_name, object_path, tmp)
195+
tmp.seek(0)
196+
if cache:
197+
cache_dir.mkdir(exist_ok=True, parents=True)
198+
target = cache_dir
199+
else:
200+
target = Path(tempfile.mkdtemp())
201+
shutil.unpack_archive(tmp.name, target) # $result=BAD

0 commit comments

Comments
 (0)