Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -217,23 +217,17 @@ class PathCheck extends Sanitizer {
Guard g;

PathCheck() {
<<<<<<< HEAD
// This expression is structurally replicated in a dominating guard
exists(AbstractValues::BooleanValue v | g = this.(GuardedDataFlowNode).getAGuard(_, v))
exists(GuardValue v |
g = this.(GuardedDataFlowNode).getAGuard(_, v) and
exists(v.asBooleanValue())
)
}

override predicate isBarrier(TaintedPathConfig::FlowState state) {
g.(WeakGuard).isBarrier(state)
or
not g instanceof WeakGuard
=======
// This expression is structurally replicated in a dominating guard which is not a "weak" check
exists(Guard g, GuardValue v |
g = this.(GuardedDataFlowNode).getAGuard(_, v) and
exists(v.asBooleanValue()) and
not g instanceof WeakGuard
)
>>>>>>> codeql-cli/latest
}
}

Expand Down
113 changes: 34 additions & 79 deletions csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,17 @@ class PathCombinerViaMethodCall extends UnsanitizedPathCombiner {
}
}

class PathCombinerViaStringInterpolation extends UnsanitizedPathCombiner instanceof InterpolatedStringExpr {}
class PathCombinerViaStringInterpolation extends UnsanitizedPathCombiner instanceof InterpolatedStringExpr
{ }

class PathCombinerViaStringConcatenation extends UnsanitizedPathCombiner instanceof AddExpr {
PathCombinerViaStringConcatenation() {
this.getAnOperand() instanceof StringLiteral
}
PathCombinerViaStringConcatenation() { this.getAnOperand() instanceof StringLiteral }
}

class MethodCallGetFullPath extends MethodCall {
MethodCallGetFullPath() { this.getTarget().hasFullyQualifiedName("System.IO.Path", "GetFullPath") }
MethodCallGetFullPath() {
this.getTarget().hasFullyQualifiedName("System.IO.Path", "GetFullPath")
}
}

/**
Expand All @@ -51,19 +52,17 @@ private module GetFullPathToQualifierTaintTrackingConfiguration implements DataF
}
}

class ZipArchiveEntryClass extends Class{
ZipArchiveEntryClass(){
this.hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry")
}
class ZipArchiveEntryClass extends Class {
ZipArchiveEntryClass() { this.hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry") }
}

/**
* The `FullName` property of `System.IO.Compression.ZipArchiveEntry`.
*/
class ZipArchiveEntryFullNameAccess extends Property{
ZipArchiveEntryFullNameAccess(){
this.getDeclaringType() instanceof ZipArchiveEntryClass and
this.getName() = "FullName"
class ZipArchiveEntryFullNameAccess extends Property {
ZipArchiveEntryFullNameAccess() {
this.getDeclaringType() instanceof ZipArchiveEntryClass and
this.getName() = "FullName"
}
}

Expand Down Expand Up @@ -185,18 +184,17 @@ module SanitizedGuardTT = TaintTracking::Global<SanitizedGuardTaintTrackingConfi
private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) {
source instanceof DataFlow::ParameterNode and
exists(RootSanitizerMethodCall smc |
smc.getEnclosingCallable() = source.getEnclosingCallable()
)
exists(RootSanitizerMethodCall smc | smc.getEnclosingCallable() = source.getEnclosingCallable())
}

predicate isSink(DataFlow::Node sink) {
exists(RootSanitizerMethodCall smc, Expr e |
e = sink.asExpr() and
e = [
smc.getAnArgument(),
smc.getQualifier()
]
e =
[
smc.getAnArgument(),
smc.getQualifier()
]
)
}
}
Expand All @@ -205,25 +203,27 @@ private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::Con
* A Callable that successfully validates a path will resolve under a given directory,
* and if it does not, throws an exception.
*/
private class ValidatingCallableThrowing extends Callable{
private class ValidatingCallableThrowing extends Callable {
Parameter paramFilename;
ValidatingCallableThrowing(){

ValidatingCallableThrowing() {
paramFilename = this.getAParameter() and
// It passes the guard, contraining the function argument to the Guard argument.
exists(ZipSlipGuard g, DataFlow::ParameterNode source, DataFlow::Node sink |
g.getEnclosingCallable() = this and
source = DataFlow::parameterNode(paramFilename) and
sink = DataFlow::exprNode(g.getFilePathArgument()) and
SanitizedGuardTT::flow(source, sink) and
exists(AbstractValues::BooleanValue bv, ThrowStmt throw |
exists(GuardValue bv, ThrowStmt throw |
throw.getEnclosingCallable() = this and
forall(TryStmt try | try.getEnclosingCallable() = this | not throw.getParent+() = try) and
// If there exists a control block that guards against misuse
bv.getValue() = false and
bv.asBooleanValue() = false and
g.controlsNode(throw.getAControlFlowNode(), bv)
)
)
}

Parameter paramFilePath() { result = paramFilename }
}

Expand Down Expand Up @@ -283,9 +283,9 @@ class DirectWrapperSantizierMethod extends AbstractWrapperSanitizerMethod {
sink = DataFlow::exprNode(g.getFilePathArgument()) and
SanitizedGuardTT::flow(source, sink) and
(
exists(AbstractValues::BooleanValue bv |
exists(GuardValue bv |
// If there exists a control block that guards against misuse
bv.getValue() = true and
bv.asBooleanValue() = true and
g.controlsNode(ret.getAControlFlowNode(), bv)
)
or
Expand Down Expand Up @@ -353,7 +353,6 @@ class WrapperSanitizerMethodCall extends SanitizerMethodCall {
index = wrapperMethod.paramFilePath().getIndex()
}


override Expr getFilePathArgument() {
exists(int index |
this.paramFilePathIndex(index) and
Expand All @@ -362,11 +361,11 @@ class WrapperSanitizerMethodCall extends SanitizerMethodCall {
}
}

private predicate wrapperCheckGuard(Guard g, Expr e, AbstractValue v) {
private predicate wrapperCheckGuard(Guard g, Expr e, GuardValue v) {
// A given wrapper method call, with the filePathArgument as a sink, that returns 'true'
g instanceof WrapperSanitizerMethodCall and
g.(WrapperSanitizerMethodCall).getFilePathArgument() = e and
v.(AbstractValues::BooleanValue).getValue() = true
v.asBooleanValue() = true
}

/**
Expand All @@ -388,8 +387,10 @@ class WrapperCheckSanitizer extends Sanitizer {
* A Call to `ValidatingCallableThrowing` which acts as a barrier in a DataFlow
*/
class ValidatingCallableThrowingSanitizer extends Sanitizer {
ValidatingCallableThrowingSanitizer(){
exists(ValidatingCallableThrowing validator, Call validatorCall | validatorCall = validator.getACall() |
ValidatingCallableThrowingSanitizer() {
exists(ValidatingCallableThrowing validator, Call validatorCall |
validatorCall = validator.getACall()
|
this = DataFlow::exprNode(validatorCall.getAnArgument())
)
}
Expand Down Expand Up @@ -418,7 +419,8 @@ class ArchiveEntryFullName extends Source {
class SinkCompressionExtractToFileArgument extends Sink {
SinkCompressionExtractToFileArgument() {
exists(MethodCall mc |
mc.getTarget().hasFullyQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile") and
mc.getTarget()
.hasFullyQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile") and
this.asExpr() = mc.getArgumentForName("destinationFileName")
)
}
Expand Down Expand Up @@ -509,34 +511,8 @@ private module ZipSlipConfig implements DataFlow::ConfigSig {
/**
* A taint tracking module for Zip Slip.
*/
<<<<<<< HEAD
module ZipSlip = TaintTracking::Global<ZipSlipConfig>;
=======
module ZipSlip = TaintTracking::Global<ZipSlipConfig>;

/** An access to the `FullName` property of a `ZipArchiveEntry`. */
class ArchiveFullNameSource extends Source {
ArchiveFullNameSource() {
exists(PropertyAccess pa | this.asExpr() = pa |
pa.getTarget()
.getDeclaringType()
.hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry") and
pa.getTarget().getName() = "FullName"
)
}
}

/** An argument to the `ExtractToFile` extension method. */
class ExtractToFileArgSink extends Sink {
ExtractToFileArgSink() {
exists(MethodCall mc |
mc.getTarget()
.hasFullyQualifiedName("System.IO.Compression", "ZipFileExtensions", "ExtractToFile") and
this.asExpr() = mc.getArgumentForName("destinationFileName")
)
}
}

/** A path argument to a `File.Open`, `File.OpenWrite`, or `File.Create` method call. */
class FileOpenArgSink extends Sink {
FileOpenArgSink() {
Expand Down Expand Up @@ -604,24 +580,3 @@ class SubstringSanitizer extends Sanitizer {
)
}
}

private predicate stringCheckGuard(Guard g, Expr e, GuardValue v) {
g.(MethodCall).getTarget().hasFullyQualifiedName("System", "String", "StartsWith") and
g.(MethodCall).getQualifier() = e and
// A StartsWith check against Path.Combine is not sufficient, because the ".." elements have
// not yet been resolved.
not exists(MethodCall combineCall |
combineCall.getTarget().hasFullyQualifiedName("System.IO", "Path", "Combine") and
DataFlow::localExprFlow(combineCall, e)
) and
v.asBooleanValue() = true
}

/**
* A call to `String.StartsWith()` that indicates that the tainted path value is being
* validated to ensure that it occurs within a permitted output path.
*/
class StringCheckSanitizer extends Sanitizer {
StringCheckSanitizer() { this = DataFlow::BarrierGuard<stringCheckGuard/3>::getABarrierNode() }
}
>>>>>>> codeql-cli/latest