Skip to content

Commit bf7ebcc

Browse files
ropwareJBropwareJB
authored
Merge pull request #275 from microsoft/jb1/AB#13038-fp
False Positive - `cs/zipslip`
2 parents 97170ee + 9b6eddb commit bf7ebcc

File tree

3 files changed

+154
-18
lines changed

3 files changed

+154
-18
lines changed

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll‎

Lines changed: 63 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,26 @@ private module GetFullPathToQualifierTaintTrackingConfiguration implements DataF
5151
}
5252
}
5353

54+
class ZipArchiveEntryClass extends Class{
55+
ZipArchiveEntryClass(){
56+
this.hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry")
57+
}
58+
}
59+
60+
/**
61+
* The `FullName` property of `System.IO.Compression.ZipArchiveEntry`.
62+
*/
63+
class ZipArchiveEntryFullNameAccess extends Property{
64+
ZipArchiveEntryFullNameAccess(){
65+
this.getDeclaringType() instanceof ZipArchiveEntryClass and
66+
this.getName() = "FullName"
67+
}
68+
}
69+
5470
/** An access to the `FullName` property of a `ZipArchiveEntry`. */
5571
class ArchiveFullNameSource extends Source {
5672
ArchiveFullNameSource() {
57-
exists(PropertyAccess pa | this.asExpr() = pa |
58-
pa.getTarget().getDeclaringType().hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry") and
59-
pa.getTarget().getName() = "FullName"
60-
)
73+
exists(ZipArchiveEntryFullNameAccess pa | pa.getAnAccess() = this.asExpr())
6174
}
6275
}
6376

@@ -125,10 +138,9 @@ private predicate safeCombineGetFullPathSequence(MethodCallGetFullPath mcGetFull
125138
class RootSanitizerMethodCall extends SanitizerMethodCall {
126139
RootSanitizerMethodCall() {
127140
exists(MethodSystemStringStartsWith sm | this.getTarget() = sm) and
128-
exists(Expr q, AbstractValue v |
141+
exists(Expr q, MethodCallGetFullPath mcGetFullPath |
129142
this.getQualifier() = q and
130-
v.(AbstractValues::BooleanValue).getValue() = true and
131-
exists(MethodCallGetFullPath mcGetFullPath | safeCombineGetFullPathSequence(mcGetFullPath, q))
143+
safeCombineGetFullPathSequence(mcGetFullPath, q)
132144
)
133145
}
134146

@@ -179,13 +191,42 @@ private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::Con
179191
}
180192

181193
predicate isSink(DataFlow::Node sink) {
182-
exists(RootSanitizerMethodCall smc |
183-
smc.getAnArgument() = sink.asExpr() or
184-
smc.getQualifier() = sink.asExpr()
194+
exists(RootSanitizerMethodCall smc, Expr e |
195+
e = sink.asExpr() and
196+
e = [
197+
smc.getAnArgument(),
198+
smc.getQualifier()
199+
]
185200
)
186201
}
187202
}
188203

204+
/**
205+
* A Callable that successfully validates a path will resolve under a given directory,
206+
* and if it does not, throws an exception.
207+
*/
208+
private class ValidatingCallableThrowing extends Callable{
209+
Parameter paramFilename;
210+
ValidatingCallableThrowing(){
211+
paramFilename = this.getAParameter() and
212+
// It passes the guard, contraining the function argument to the Guard argument.
213+
exists(ZipSlipGuard g, DataFlow::ParameterNode source, DataFlow::Node sink |
214+
g.getEnclosingCallable() = this and
215+
source = DataFlow::parameterNode(paramFilename) and
216+
sink = DataFlow::exprNode(g.getFilePathArgument()) and
217+
SanitizedGuardTT::flow(source, sink) and
218+
exists(AbstractValues::BooleanValue bv, ThrowStmt throw |
219+
throw.getEnclosingCallable() = this and
220+
forall(TryStmt try | try.getEnclosingCallable() = this | not throw.getParent+() = try) and
221+
// If there exists a control block that guards against misuse
222+
bv.getValue() = false and
223+
g.controlsNode(throw.getAControlFlowNode(), bv)
224+
)
225+
)
226+
}
227+
Parameter paramFilePath() { result = paramFilename }
228+
}
229+
189230
/**
190231
* An AbstractWrapperSanitizerMethod is a Method that
191232
* is a suitable sanitizer for a ZipSlip path that may not have been canonicalized prior.
@@ -199,19 +240,12 @@ abstract private class AbstractWrapperSanitizerMethod extends AbstractSanitizerM
199240

200241
AbstractWrapperSanitizerMethod() {
201242
this.getReturnType() instanceof BoolType and
202-
this.getAParameter() = paramFilename
243+
paramFilename = this.getAParameter()
203244
}
204245

205246
Parameter paramFilePath() { result = paramFilename }
206247
}
207248

208-
/* predicate aaaa(ZipSlipGuard g, DataFlow::ParameterNode source){
209-
exists(DataFlow::Node sink |
210-
sink = DataFlow::exprNode(g.getFilePathArgument()) and
211-
SanitizedGuardTT::flow(source, sink) and
212-
)
213-
} */
214-
215249
/**
216250
* A DirectWrapperSantizierMethod is a Method where
217251
* The function can /only/ returns true when passes through the RootSanitizerGuard
@@ -350,6 +384,17 @@ class WrapperCheckSanitizer extends Sanitizer {
350384
WrapperCheckSanitizer() { this = DataFlow::BarrierGuard<wrapperCheckGuard/3>::getABarrierNode() }
351385
}
352386

387+
/**
388+
* A Call to `ValidatingCallableThrowing` which acts as a barrier in a DataFlow
389+
*/
390+
class ValidatingCallableThrowingSanitizer extends Sanitizer {
391+
ValidatingCallableThrowingSanitizer(){
392+
exists(ValidatingCallableThrowing validator, Call validatorCall | validatorCall = validator.getACall() |
393+
this = DataFlow::exprNode(validatorCall.getAnArgument())
394+
)
395+
}
396+
}
397+
353398
/**
354399
* A data flow source for unsafe zip extraction.
355400
*/

‎csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,22 @@ public static bool ContainsPath(string? fullPath, string? path, bool excludeSame
196196
}
197197
}
198198

199+
/* Test that the given `fullPath` exists within the given `path` directory.
200+
* If it does not, throw an exception to terminate the request.
201+
*/
202+
public static void ContainsPathValidationThrowing(string? fullPath, string? path)
203+
{
204+
fullPath = Path.GetFullPath(fullPath);
205+
path = Path.GetFullPath(path);
206+
207+
fullPath = AddBackslashIfNotPresent(fullPath);
208+
path = AddBackslashIfNotPresent(path);
209+
210+
if (!fullPath!.StartsWith(path, StringComparison.OrdinalIgnoreCase)) {
211+
throw new Exception("Attempting path traversal");
212+
}
213+
}
214+
199215
static void Main(string[] args)
200216
{
201217
string zipFileName;
@@ -231,5 +247,66 @@ static void fp_throw(ZipArchive archive, string root){
231247
entry.ExtractToFile(destinationOnDisk, true);
232248
}
233249
}
250+
251+
/**
252+
* Negative - dangerous path terminates early due to exception thrown by guarded condition in descendent call.
253+
*/
254+
static void fp_throw_nested_exception(ZipArchive archive, string root){
255+
foreach (var entry in archive.Entries){
256+
string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName));
257+
string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
258+
ContainsPathValidationThrowing(destinationOnDisk, fullRoot);
259+
// NEGATIVE, above exception short circuits by exception on invalid input by path traversal.
260+
entry.ExtractToFile(destinationOnDisk, true);
261+
}
262+
}
263+
264+
/**
265+
* Negative - no extraction, only sanitization
266+
*/
267+
static void fp_throw_sanitizer_valid(string file, string root){
268+
string destinationOnDisk = Path.GetFullPath(file);
269+
string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
270+
if (!destinationOnDisk.StartsWith(fullRoot)){
271+
throw new Exception("Entry is outside of target directory. There may have been some directory traversal sequences in filename.");
272+
}
273+
}
274+
275+
/**
276+
* Negative - dangerous path terminates early due to throw in fp_throw_sanitizer_valid
277+
*/
278+
static void fp_throw_nested_exception_uncaught(ZipArchive archive, string root){
279+
foreach (var entry in archive.Entries){
280+
string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName));
281+
string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
282+
fp_throw_sanitizer_valid(destinationOnDisk, fullRoot);
283+
entry.ExtractToFile(destinationOnDisk, true);
284+
}
285+
}
286+
287+
/**
288+
* Negative - no extraction, only sanitization
289+
*/
290+
static void fp_throw_sanitizer_invalid(string file, string root){
291+
try{
292+
string destinationOnDisk = Path.GetFullPath(file);
293+
string fullRoot = Path.GetFullPath(root);
294+
if (!destinationOnDisk.StartsWith(fullRoot)){
295+
throw new Exception("Entry is outside of target directory. There may have been some directory traversal sequences in filename.");
296+
}
297+
}catch(Exception e){}
298+
}
299+
300+
/**
301+
* Positive - dangerous path does not terminate early due to try block in fp_throw_sanitizer_invalid
302+
*/
303+
static void tp_throw_nested_exception_caught(ZipArchive archive, string root){
304+
foreach (var entry in archive.Entries){
305+
string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName));
306+
string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
307+
fp_throw_sanitizer_invalid(destinationOnDisk, fullRoot);
308+
entry.ExtractToFile(destinationOnDisk, true);
309+
}
310+
}
234311
}
235312
}

‎csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
| ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | file system operation |
99
| ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | file system operation |
1010
| ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | file system operation |
11+
| ZipSlip.cs:305:80:305:93 | access to property FullName | ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | file system operation |
1112
| ZipSlipBad.cs:9:59:9:72 | access to property FullName | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | file system operation |
1213
edges
1314
| ZipSlip.cs:15:24:15:40 | access to local variable fullPath_relative : String | ZipSlip.cs:30:71:30:87 | access to local variable fullPath_relative : String | provenance | |
@@ -47,6 +48,13 @@ edges
4748
| ZipSlip.cs:121:71:121:82 | access to local variable destFilePath : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | provenance | |
4849
| ZipSlip.cs:121:71:121:82 | access to local variable destFilePath : String | ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | provenance | |
4950
| ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | provenance | |
51+
| ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | provenance | |
52+
| ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | provenance | |
53+
| ZipSlip.cs:305:61:305:94 | call to method Combine : String | ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | provenance | Config |
54+
| ZipSlip.cs:305:61:305:94 | call to method Combine : String | ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | provenance | MaD:2 |
55+
| ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:305:61:305:94 | call to method Combine : String | provenance | Config |
56+
| ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:305:61:305:94 | call to method Combine : String | provenance | MaD:1 |
57+
| ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | provenance | |
5058
| ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | provenance | |
5159
| ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | provenance | |
5260
| ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | provenance | Config |
@@ -91,6 +99,12 @@ nodes
9199
| ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | semmle.label | access to local variable destFilePath |
92100
| ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | semmle.label | access to local variable destFilePath : String |
93101
| ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | semmle.label | access to local variable destFilePath |
102+
| ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | semmle.label | access to local variable destinationOnDisk : String |
103+
| ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | semmle.label | call to method GetFullPath : String |
104+
| ZipSlip.cs:305:61:305:94 | call to method Combine : String | semmle.label | call to method Combine : String |
105+
| ZipSlip.cs:305:80:305:93 | access to property FullName : String | semmle.label | access to property FullName : String |
106+
| ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | semmle.label | access to local variable destinationOnDisk : String |
107+
| ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | semmle.label | access to local variable destinationOnDisk |
94108
| ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | semmle.label | access to local variable destFileName : String |
95109
| ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | semmle.label | call to method Combine : String |
96110
| ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | semmle.label | access to property FullName : String |

0 commit comments

Comments
 (0)