Skip to content

Commit c064b1f

Browse files
hvitvedhvitved
authored
Merge pull request #103 from lukecartey/csharp/zipslip-update
C#: ZipSlip - Refine sanitizers
2 parents 8d8148d + 1a90f7d commit c064b1f

File tree

3 files changed

+61
-16
lines changed

3 files changed

+61
-16
lines changed

‎csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll‎

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
import csharp
55

66
module ZipSlip {
7+
import semmle.code.csharp.controlflow.Guards
8+
79
/**
810
* A data flow source for unsafe zip extraction.
911
*/
@@ -96,31 +98,55 @@ module ZipSlip {
9698
}
9799

98100
/**
99-
* An argument to `GetFileName`.
101+
* A call to `GetFileName`.
100102
*
101103
* This is considered a sanitizer because it extracts just the file name, not the full path.
102104
*/
103105
class GetFileNameSanitizer extends Sanitizer {
104106
GetFileNameSanitizer() {
105107
exists(MethodCall mc |
106108
mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName") |
107-
this.asExpr() = mc.getAnArgument()
109+
this.asExpr() = mc
108110
)
109111
}
110112
}
111113

112114
/**
113-
* A qualifier in a call to `StartsWith` or `Substring` string method.
115+
* A call to `Substring`.
114116
*
115-
* A call to a String method such as `StartsWith` or `Substring` can indicate a check for a
116-
* relative path, or a check against the destination folder for whitelisted/target path, etc.
117+
* This is considered a sanitizer because `Substring` may be used to extract a single component
118+
* of a path to avoid ZipSlip.
117119
*/
118-
class StringCheckSanitizer extends Sanitizer {
119-
StringCheckSanitizer() {
120+
class SubstringSanitizer extends Sanitizer {
121+
SubstringSanitizer() {
120122
exists(MethodCall mc |
121-
mc.getTarget().hasQualifiedName("System.String", "StartsWith") or
122123
mc.getTarget().hasQualifiedName("System.String", "Substring") |
123-
this.asExpr() = mc.getQualifier()
124+
this.asExpr() = mc
125+
)
126+
}
127+
}
128+
129+
/**
130+
* An expression which is guarded by a call to `String.StartsWith`.
131+
*
132+
* A call to the method `String.StartsWith` can indicate the the tainted path value is being
133+
* validated to ensure that it occurs within a permitted output path.
134+
*/
135+
class StringCheckSanitizer extends Sanitizer {
136+
StringCheckSanitizer() {
137+
exists(GuardedExpr ge, MethodCall mc, Expr startsWithQualifier |
138+
ge = this.asExpr() and
139+
ge.isGuardedBy(mc, startsWithQualifier, true) |
140+
mc.getTarget().hasQualifiedName("System.String", "StartsWith") and
141+
mc.getQualifier() = startsWithQualifier and
142+
/*
143+
* A StartsWith check against Path.Combine is not sufficient, because the ".." elements have
144+
* not yet been resolved.
145+
*/
146+
not exists(MethodCall combineCall |
147+
combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and
148+
DataFlow::localFlow(DataFlow::exprNode(combineCall), DataFlow::exprNode(startsWithQualifier))
149+
)
124150
)
125151
}
126152
}

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

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,15 @@ public static void UnzipFileByFile(ZipArchive archive,
3131
string destFilePath = Path.Combine(destDirectory, fullPath);
3232
entry.ExtractToFile(destFilePath, true);
3333

34-
// GOOD: some check on destination.
34+
// BAD: destFilePath isn't fully resolved, so may still contain ..
35+
if (destFilePath.StartsWith(destDirectory))
36+
entry.ExtractToFile(destFilePath, true);
37+
38+
// BAD
39+
destFilePath = Path.GetFullPath(Path.Combine(destDirectory, fullPath));
40+
entry.ExtractToFile(destFilePath, true);
41+
42+
// GOOD: a check for StartsWith against a fully resolved path
3543
if (destFilePath.StartsWith(destDirectory))
3644
entry.ExtractToFile(destFilePath, true);
3745
}
@@ -51,7 +59,7 @@ private static int UnzipToStream(Stream zipStream, string installDir)
5159
foreach (ZipArchiveEntry entry in archive.Entries)
5260
{
5361
// figure out where we are putting the file
54-
string destFilePath = Path.Combine(InstallDir, entry.FullName);
62+
String destFilePath = Path.Combine(InstallDir, entry.FullName);
5563

5664
Directory.CreateDirectory(Path.GetDirectoryName(destFilePath));
5765

@@ -86,6 +94,15 @@ private static int UnzipToStream(Stream zipStream, string installDir)
8694
Console.WriteLine(@"Writing ""{0}""", destFilePath);
8795
archiveFileStream.CopyTo(fs);
8896
}
97+
98+
// GOOD: Use substring to pick out single component
99+
string fileName = destFilePath.Substring(destFilePath.LastIndexOf("\\"));
100+
var fileInfo2 = new FileInfo(fileName);
101+
using (FileStream fs = fileInfo2.Open(FileMode.Create))
102+
{
103+
Console.WriteLine(@"Writing ""{0}""", destFilePath);
104+
archiveFileStream.CopyTo(fs);
105+
}
89106
}
90107
}
91108
}
@@ -115,7 +132,7 @@ static void Main(string[] args)
115132
// GOOD: the path is checked in this extension method
116133
archive.ExtractToDirectory(targetPath);
117134

118-
UnzipToStream(file, targetPath);
135+
UnzipToStream(file, targetPath);
119136
}
120137
}
121138
}

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
| ZipSlip.cs:24:41:24:52 | access to local variable destFileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:19:31:19:44 | access to property FullName | item path |
22
| ZipSlip.cs:32:41:32:52 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:16:52:16:65 | access to property FullName | item path |
3-
| ZipSlip.cs:61:74:61:85 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
4-
| ZipSlip.cs:68:71:68:82 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
5-
| ZipSlip.cs:75:57:75:68 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
6-
| ZipSlip.cs:83:58:83:69 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
3+
| ZipSlip.cs:36:45:36:56 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:16:52:16:65 | access to property FullName | item path |
4+
| ZipSlip.cs:40:41:40:52 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:16:52:16:65 | access to property FullName | item path |
5+
| ZipSlip.cs:69:74:69:85 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
6+
| ZipSlip.cs:76:71:76:82 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
7+
| ZipSlip.cs:83:57:83:68 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
8+
| ZipSlip.cs:91:58:91:69 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
79
| ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.cs:9:59:9:72 | access to property FullName | item path |

0 commit comments

Comments
 (0)