Skip to content

Commit 3840d92

Browse files
ropwareJBropwareJB
committed
Barrier node whereby there is a call that performs valid sanitization, and throws uncaught exception
1 parent 1f79f38 commit 3840d92

File tree

3 files changed

+98
-0
lines changed

3 files changed

+98
-0
lines changed

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,32 @@ private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::Con
204204
}
205205
}
206206

207+
/**
208+
* A Callable that successfully validates a path will resolve under a given directory,
209+
* and if it does not, throws an exception.
210+
*/
211+
private class ValidatingCallableThrowing extends Callable{
212+
Parameter paramFilename;
213+
ValidatingCallableThrowing(){
214+
paramFilename = this.getAParameter() and
215+
// It passes the guard, contraining the function argument to the Guard argument.
216+
exists(ZipSlipGuard g, DataFlow::ParameterNode source, DataFlow::Node sink |
217+
g.getEnclosingCallable() = this and
218+
source = DataFlow::parameterNode(paramFilename) and
219+
sink = DataFlow::exprNode(g.getFilePathArgument()) and
220+
SanitizedGuardTT::flow(source, sink) and
221+
exists(AbstractValues::BooleanValue bv, ThrowStmt throw |
222+
throw.getEnclosingCallable() = this and
223+
forall(TryStmt try | try.getEnclosingCallable() = this | not throw.getParent+() = try) and
224+
// If there exists a control block that guards against misuse
225+
bv.getValue() = false and
226+
g.controlsNode(throw.getAControlFlowNode(), bv)
227+
)
228+
)
229+
}
230+
Parameter paramFilePath() { result = paramFilename }
231+
}
232+
207233
/**
208234
* An AbstractWrapperSanitizerMethod is a Method that
209235
* is a suitable sanitizer for a ZipSlip path that may not have been canonicalized prior.
@@ -361,6 +387,17 @@ class WrapperCheckSanitizer extends Sanitizer {
361387
WrapperCheckSanitizer() { this = DataFlow::BarrierGuard<wrapperCheckGuard/3>::getABarrierNode() }
362388
}
363389

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

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,5 +261,52 @@ static void fp_throw_nested_exception(ZipArchive archive, string root){
261261
}
262262
}
263263

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+
}
264311
}
265312
}

‎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)