Skip to content
Open
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
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* When a guard such as `isSafe(x)` is defined, we now also automatically handle `isSafe(x) == true` and `isSafe(x) != false`.
Original file line number Diff line number Diff line change
Expand Up @@ -595,14 +595,42 @@ ControlFlowNode guardNode(ConditionBlock conditionBlock, boolean flipped) {
result = conditionBlock.getLastNode() and
flipped = false
or
// Recursive case: if a guard node is a `not`-expression,
// Recursive cases:
// if a guard node is a `not`-expression,
// the operand is also a guard node, but with inverted polarity.
exists(UnaryExprNode notNode |
result = notNode.getOperand() and
notNode.getNode().getOp() instanceof Not
|
notNode = guardNode(conditionBlock, flipped.booleanNot())
)
or
// if a guard node is compared to a boolean literal,
// the other operand is also a guard node,
// but with polarity depending on the literal (and on the comparison).
exists(CompareNode cmpNode, Cmpop op, ControlFlowNode b, boolean should_flip |
(
cmpNode.operands(result, op, b) or
cmpNode.operands(b, op, result)
) and
not result.getNode() instanceof BooleanLiteral and
(
// comparing to the boolean
(op instanceof Eq or op instanceof Is) and
// we should flip if the value compared against, here the value of `b`, is false
should_flip = b.getNode().(BooleanLiteral).booleanValue().booleanNot()
or
// comparing to the negation of the boolean
(op instanceof NotEq or op instanceof IsNot) and
// again, we should flip if the value compared against, here the value of `not b`, is false.
// That is, if the value of `b` is true.
should_flip = b.getNode().(BooleanLiteral).booleanValue()
)
|
// we flip `flipped` according to `should_flip` via the formula `flipped xor should_flip`.
flipped in [true, false] and
cmpNode = guardNode(conditionBlock, flipped.booleanXor(should_flip))
)
}

/**
Expand Down
6 changes: 6 additions & 0 deletions python/ql/lib/semmle/python/frameworks/AntiSSRF.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
extensions:
- addsTo:
pack: codeql/python-all
extensible: barrierGuardModel
data:
- ['AntiSSRF', 'Member[URIValidator].Member[in_domain,in_azure_keyvault_domain,in_azure_storage_domain].Argument[0]', "true", 'request-forgery']
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ private import semmle.python.Concepts
private import semmle.python.dataflow.new.RemoteFlowSources
private import semmle.python.dataflow.new.BarrierGuards
private import semmle.python.ApiGraphs
private import semmle.python.frameworks.data.internal.ApiGraphModels

/**
* Provides default sources, sinks and sanitizers for detecting
Expand Down Expand Up @@ -177,35 +178,7 @@ module ServerSideRequestForgery {
)
}

/** A validation of a URI using the `AntiSSRF` library, considered as a full-ssrf sanitizer. */
private class UriValidator extends FullUrlControlSanitizer {
UriValidator() { this = DataFlow::BarrierGuard<uri_validator/3>::getABarrierNode() }
}

import semmle.python.dataflow.new.internal.DataFlowPublic

private predicate uri_validator(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
exists(DataFlow::CallCfgNode call, string funcs |
funcs in ["in_domain", "in_azure_keyvault_domain", "in_azure_storage_domain"] and
call = API::moduleImport("AntiSSRF").getMember("URIValidator").getMember(funcs).getACall() and
call.getArg(0).asCfgNode() = node
|
// validator call directly (e.g., if URIValidator.in_domain(...) )
g = call.asCfgNode() and
branch = true
or
// validator used in a comparison
exists(Cmpop op, Node n, ControlFlowNode l |
n.getALocalSource() = call and g.(CompareNode).operands(n.asCfgNode(), op, l)
|
// validator == true or validator == false or validator is True or validator is False
(op instanceof Eq or op instanceof Is) and
branch = l.getNode().(BooleanLiteral).booleanValue()
or
// validator != false or validator != true or validator is not True or validator is not False
(op instanceof NotEq or op instanceof IsNot) and
branch = l.getNode().(BooleanLiteral).booleanValue().booleanNot()
)
)
private class ExternalRequestForgerySanitizer extends FullUrlControlSanitizer {
ExternalRequestForgerySanitizer() { ModelOutput::barrierNode(this, "request-forgery") }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,12 @@ isSanitizer
| test_logical.py:176:24:176:24 | ControlFlowNode for s |
| test_logical.py:185:24:185:24 | ControlFlowNode for s |
| test_logical.py:193:24:193:24 | ControlFlowNode for s |
| test_logical.py:199:28:199:28 | ControlFlowNode for s |
| test_logical.py:206:28:206:28 | ControlFlowNode for s |
| test_logical.py:211:28:211:28 | ControlFlowNode for s |
| test_logical.py:214:28:214:28 | ControlFlowNode for s |
| test_logical.py:219:28:219:28 | ControlFlowNode for s |
| test_logical.py:226:28:226:28 | ControlFlowNode for s |
| test_logical.py:231:28:231:28 | ControlFlowNode for s |
| test_logical.py:234:28:234:28 | ControlFlowNode for s |
| test_reference.py:31:28:31:28 | ControlFlowNode for s |
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,49 @@ def test_with_exception_neg():

ensure_not_tainted(s)

def test_comparison_with_bool():
s = TAINTED_STRING

if is_safe(s) == True:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted

if is_safe(s) == False:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) != True:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) != False:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted
Comment on lines +198 to +216
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness, we should add versions of these tests with the arguments to ==/!= swapped. (I sincerely hope no one has ever written False is not is_safe(s), so let's not bother with those.)


if is_safe(s) is True:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted

if is_safe(s) is False:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) is not True:
ensure_tainted(s) # $ tainted
else:
ensure_not_tainted(s)

if is_safe(s) is not False:
ensure_not_tainted(s)
else:
ensure_tainted(s) # $ tainted

# Make tests runable

test_basic()
Expand All @@ -211,3 +254,4 @@ def test_with_exception_neg():
test_with_exception_neg()
except:
pass
test_comparison_with_bool()