Skip to content

Commit b764c43

Browse files
MathiasVPMathiasVP
authored
Merge pull request #346 from microsoft/users/chanely/insecure-random
Flow from insecure random number to sensitive sink
2 parents 4df918d + b48624a commit b764c43

18 files changed

Lines changed: 525 additions & 56 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,4 @@ You can install the [CodeQL for Visual Studio Code](https://marketplace.visualst
3131
The `.vscode/tasks.json` file defines custom tasks specific to working in this repository. To invoke one of these tasks, select the `Terminal | Run Task...` menu option, and then select the desired task from the dropdown. You can also invoke the `Tasks: Run Task` command from the command palette.
3232

3333

34+

‎powershell/ql/lib/semmle/code/powershell/ApiGraphs.qll‎

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -367,8 +367,6 @@ module API {
367367

368368
Node methodEdge(string name) { none() }
369369

370-
Node instanceEdge() { none() }
371-
372370
final predicate isImplicit() { not this.isExplicit(_) }
373371

374372
predicate isExplicit(DataFlow::Node node) { none() }
@@ -426,26 +424,6 @@ module API {
426424
}
427425
}
428426

429-
class NewObjectTypeNameNode extends AbstractTypeNameNode, Impl::MkNewObjectTypeNameNode {
430-
NewObjectTypeNameNode() { this = Impl::MkNewObjectTypeNameNode(prefix) }
431-
432-
final override Node getSuccessor(string name) {
433-
result = Impl::MkNewObjectTypeNameNode(prefix + "." + name)
434-
}
435-
436-
final override Node instanceEdge() {
437-
exists(DataFlow::ObjectCreationNode creation |
438-
prefix = creation.getLowerCaseConstructedTypeName() and
439-
Specific::needsNewObjectTypeNameNode(creation, prefix) and
440-
result = getForwardStartNode(creation)
441-
)
442-
}
443-
444-
final override predicate isExplicit(DataFlow::Node node) {
445-
Specific::needsNewObjectTypeNameNode(node, prefix)
446-
}
447-
}
448-
449427
/**
450428
* An API entry point.
451429
*
@@ -539,7 +517,6 @@ module API {
539517
MkMethodAccessNode(DataFlow::CallNode call) or
540518
MkExplicitTypeNameNode(string prefix) { Specific::needsExplicitTypeNameNode(_, prefix) } or
541519
MkImplicitTypeNameNode(string prefix) { Specific::needsImplicitTypeNameNode(prefix) } or
542-
MkNewObjectTypeNameNode(string prefix) { Specific::needsNewObjectTypeNameNode(_, prefix) } or
543520
MkForwardNode(DataFlow::LocalSourceNode node, TypeTracker t) { isReachable(node, t) } or
544521
/** Intermediate node for following backward data flow. */
545522
MkBackwardNode(DataFlow::LocalSourceNode node, TypeTracker t) { isReachable(node, t) } or
@@ -721,8 +698,6 @@ module API {
721698
pred = getForwardEndNode(call.getQualifier()) and
722699
succ = getForwardStartNode(call)
723700
)
724-
or
725-
pred.(TypeNameNode).instanceEdge() = succ
726701
}
727702

728703
cached

‎powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -743,7 +743,7 @@ class FlowSummaryNode extends NodeImpl, TFlowSummaryNode {
743743

744744
override EmptyLocation getLocationImpl() { any() }
745745

746-
override predicate nodeIsHidden() { any() }
746+
override predicate nodeIsHidden() { this.getSummaryNode().isHidden() }
747747

748748
override string toStringImpl() { result = this.getSummaryNode().toString() }
749749
}

‎powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPublic.qll‎

Lines changed: 55 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -570,35 +570,76 @@ class ToStringCallNode extends CallNode {
570570
}
571571

572572
/** A use of a type name, viewed as a node in a data flow graph. */
573-
class TypeNameNode extends ExprNode {
574-
override CfgNodes::ExprNodes::TypeNameExprCfgNode n;
575-
576-
override CfgNodes::ExprNodes::TypeNameExprCfgNode getExprNode() { result = n }
573+
abstract private class TypeNameNodeImpl extends ExprNode {
574+
private predicate parseName(string namespace, string typename) {
575+
exists(string fullName | fullName = this.getPossiblyQualifiedName() |
576+
if fullName.matches("%.%")
577+
then
578+
namespace = fullName.regexpCapture("([a-zA-Z0-9\\.]+)\\.([a-zA-Z0-9]+)", 1) and
579+
typename = fullName.regexpCapture("([a-zA-Z0-9\\.]+)\\.([a-zA-Z0-9]+)", 2)
580+
else (
581+
namespace = "" and
582+
typename = fullName
583+
)
584+
)
585+
}
577586

578587
bindingset[result]
579588
pragma[inline_late]
580-
string getAName() { result = n.getAName() }
589+
string getAName() { this.parseName(_, result.toLowerCase()) }
581590

582-
string getLowerCaseName() { result = n.getLowerCaseName() }
591+
string getLowerCaseName() { this.parseName(_, result) }
583592

584-
predicate isQualified() { n.isQualified() }
593+
predicate isQualified() { this.getNamespace() != "" }
585594

586595
predicate hasQualifiedName(string namespace, string typename) {
587-
n.hasQualifiedName(namespace, typename)
596+
this.isQualified() and
597+
this.parseName(namespace, typename)
588598
}
589599

590-
string getNamespace() { result = n.getNamespace() }
600+
string getNamespace() { this.parseName(result, _) }
591601

592-
string getPossiblyQualifiedName() { result = n.getPossiblyQualifiedName() }
602+
abstract string getPossiblyQualifiedName();
593603
}
594604

595-
/** A use of a qualified type name, viewed as a node in a data flow graph. */
596-
class QualifiedTypeNameNode extends TypeNameNode {
597-
override CfgNodes::ExprNodes::QualifiedTypeNameExprCfgNode n;
605+
/** A use of a type name, viewed as a node in a data flow graph. */
606+
final class TypeNameNode = TypeNameNodeImpl;
607+
608+
private class TypeNameExprNode extends TypeNameNodeImpl {
609+
override CfgNodes::ExprNodes::TypeNameExprCfgNode n;
610+
611+
final override CfgNodes::ExprNodes::TypeNameExprCfgNode getExprNode() { result = n }
612+
613+
override string getPossiblyQualifiedName() { result = n.getPossiblyQualifiedName() }
614+
}
615+
616+
private class StringTypeNameNode extends TypeNameNodeImpl {
617+
override CfgNodes::ExprNodes::StringLiteralExprCfgNode n;
598618

599-
final override CfgNodes::ExprNodes::QualifiedTypeNameExprCfgNode getExprNode() { result = n }
619+
StringTypeNameNode() {
620+
exists(CfgNodes::ExprNodes::ObjectCreationCfgNode creation |
621+
creation.getExpr() instanceof DotNetObjectCreation and
622+
n = creation.getConstructedTypeExpr()
623+
)
624+
}
625+
626+
final override CfgNodes::ExprNodes::StringLiteralExprCfgNode getExprNode() { result = n }
627+
628+
override string getPossiblyQualifiedName() { result = n.getValueString().toLowerCase() }
600629
}
601630

631+
/** A use of a qualified type name, viewed as a node in a data flow graph. */
632+
abstract class QualifiedTypeNameNodeImpl extends TypeNameNodeImpl {
633+
QualifiedTypeNameNodeImpl() { this.isQualified() }
634+
}
635+
636+
/** A use of a qualified type name, viewed as a node in a data flow graph. */
637+
final class QualifiedTypeNameNode = QualifiedTypeNameNodeImpl;
638+
639+
private class QualifiedTypeNameExprNode extends QualifiedTypeNameNodeImpl, TypeNameExprNode { }
640+
641+
private class QualifiedStringTypeNameNode extends QualifiedTypeNameNodeImpl, StringTypeNameNode { }
642+
602643
/** A use of an automatic variable, viewed as a node in a data flow graph. */
603644
class AutomaticVariableNode extends ExprNode {
604645
override CfgNodes::ExprNodes::AutomaticVariableCfgNode n;

‎powershell/ql/lib/semmle/code/powershell/frameworks/System.model.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
11
extensions:
2+
- addsTo:
3+
pack: microsoft/powershell-all
4+
extensible: summaryModel
5+
data:
6+
- ["system.random", "Method[nextbytes]", "Argument[this]", "Argument[0]", "taint"]
27
- addsTo:
38
pack: microsoft/powershell-all
49
extensible: sourceModel

‎powershell/ql/lib/semmle/code/powershell/frameworks/data/internal/ApiGraphModelsSpecific.qll‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -101,16 +101,6 @@ predicate needsImplicitTypeNameNode(string component) {
101101
)
102102
}
103103

104-
predicate needsNewObjectTypeNameNode(DataFlow::ObjectCreationNode creation, string component) {
105-
creation.asExpr().getExpr() instanceof DotNetObjectCreation and
106-
exists(string type, int index |
107-
type = creation.getLowerCaseConstructedTypeName() and
108-
index = [0 .. strictcount(type.indexOf("."))] and
109-
component =
110-
strictconcat(int i, string s | s = type.splitAt(".", i) and i <= index | s, "." order by i)
111-
)
112-
}
113-
114104
/** Gets a Powershell-specific interpretation of the given `type`. */
115105
API::Node getExtraNodeFromType(string rawType) {
116106
exists(string type, string suffix, DataFlow::TypeNameNode typeName |

‎powershell/ql/lib/semmle/code/powershell/security/InsecureRandomnessCustomizations.qll‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* insecure-randomness vulnerabilities, as well as extension points for
4+
* adding your own.
5+
*/
6+
7+
private import semmle.code.powershell.dataflow.DataFlow
8+
import semmle.code.powershell.ApiGraphs
9+
10+
module InsecureRandomness {
11+
/**
12+
* A data flow source for insecure-randomness vulnerabilities.
13+
*/
14+
abstract class Source extends DataFlow::Node { }
15+
16+
/**
17+
* A data flow sink for insecure-randomness vulnerabilities.
18+
*/
19+
abstract class Sink extends DataFlow::Node {
20+
/** Gets a human-readable description of this sink. */
21+
abstract string getSinkDescription();
22+
}
23+
24+
/**
25+
* A sanitizer for insecure-randomness vulnerabilities.
26+
*/
27+
abstract class Sanitizer extends DataFlow::Node { }
28+
29+
/** A call to the `Get-Random` cmdlet. */
30+
class GetRandomSource extends Source instanceof DataFlow::CallNode {
31+
GetRandomSource() { this.matchesName("Get-Random") }
32+
}
33+
34+
/** An instantiation of `System.Random` via `New-Object`. */
35+
class SystemRandomObjectCreation extends Source instanceof DataFlow::ObjectCreationNode {
36+
SystemRandomObjectCreation() {
37+
this.asExpr()
38+
.getExpr()
39+
.(ObjectCreation)
40+
.getAnArgument()
41+
.getValue()
42+
.stringMatches("System.Random")
43+
}
44+
}
45+
46+
/** A call to `[System.Random]::new()` via the API graph. */
47+
class SystemRandomNewCall extends Source instanceof DataFlow::CallNode {
48+
SystemRandomNewCall() {
49+
this = API::getTopLevelMember("system").getMember("random").getMember("new").asCall()
50+
}
51+
}
52+
53+
/** Assignment to .Key or .IV on an object. */
54+
class CryptoKeyIvAssignmentSink extends Sink {
55+
CryptoKeyIvAssignmentSink() {
56+
exists(AssignStmt a, MemberExprWriteAccess m |
57+
m = a.getLeftHandSide() and
58+
m.getLowerCaseMemberName() in ["key", "iv"] and
59+
this.asExpr().getExpr() = a.getRightHandSide()
60+
)
61+
}
62+
63+
override string getSinkDescription() { result = "a cryptographic key or IV" }
64+
}
65+
66+
/** Argument to a cryptographic constructor (HMAC key or KDF salt). */
67+
class CryptoConstructorSink extends Sink {
68+
CryptoConstructorSink() {
69+
exists(DataFlow::ObjectCreationNode oc |
70+
(
71+
oc.getLowerCaseConstructedTypeName().matches("%hmac%")
72+
or
73+
oc.getLowerCaseConstructedTypeName().matches("%rfc2898derivebytes%")
74+
) and
75+
this = oc.getAnArgument() and
76+
not this = oc.getConstructedTypeNode()
77+
)
78+
}
79+
80+
override string getSinkDescription() { result = "a cryptographic operation" }
81+
}
82+
83+
/** First positional argument to ConvertTo-SecureString. */
84+
class SecureStringSink extends Sink {
85+
SecureStringSink() {
86+
exists(DataFlow::CallNode call |
87+
call.matchesName("ConvertTo-SecureString") and
88+
this = call.getPositionalArgument(0)
89+
)
90+
}
91+
92+
override string getSinkDescription() { result = "a secure string conversion" }
93+
}
94+
95+
/** Value used in HTTP headers passed to Invoke-RestMethod or Invoke-WebRequest. */
96+
class HttpHeaderValueSink extends Sink {
97+
HttpHeaderValueSink() {
98+
exists(DataFlow::CallNode call, HashTableExpr ht |
99+
(call.matchesName("Invoke-RestMethod") or call.matchesName("Invoke-WebRequest")) and
100+
call.getNamedArgument("headers").asExpr().getExpr() = ht and
101+
this.asExpr().getExpr() = ht.getAValue()
102+
)
103+
}
104+
105+
override string getSinkDescription() { result = "an HTTP header" }
106+
}
107+
}

‎powershell/ql/lib/semmle/code/powershell/security/InsecureRandomnessQuery.qll‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about
3+
* insecure-randomness vulnerabilities (CWE-338).
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `InsecureRandomnessFlow` is needed, otherwise
7+
* `InsecureRandomnessCustomizations` should be imported instead.
8+
*/
9+
10+
import powershell
11+
import semmle.code.powershell.dataflow.TaintTracking
12+
import InsecureRandomnessCustomizations::InsecureRandomness
13+
import semmle.code.powershell.dataflow.DataFlow
14+
15+
private module Config implements DataFlow::ConfigSig {
16+
predicate isSource(DataFlow::Node source) { source instanceof Source }
17+
18+
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
19+
20+
predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
21+
}
22+
23+
/**
24+
* Taint-tracking for reasoning about insecure-randomness vulnerabilities.
25+
*/
26+
module InsecureRandomnessFlow = TaintTracking::Global<Config>;

‎powershell/ql/src/queries/security/cwe-338/InsecureRandomness.qhelp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>Using non-cryptographic random number generators for security-sensitive operations can compromise security. `Get-Random` and `System.Random` are not suitable for generating cryptographic keys, security tokens, or other security-critical random values.</p>
6+
</overview>
7+
8+
<recommendation>
9+
<p>Use a cryptographically secure random number generator such as `[System.Security.Cryptography.RandomNumberGenerator]` instead.</p>
10+
</recommendation>
11+
12+
<example>
13+
<p>Insecure random:</p>
14+
<sample src="examples/InsecureRandomness/InsecureRandomnessBad.ps1" />
15+
16+
<p>Secure alternative:</p>
17+
<sample src="examples/InsecureRandomness/InsecureRandomnessGood.ps1" />
18+
</example>
19+
20+
<references>
21+
<li><a href="https://cwe.mitre.org/data/definitions/338.html">CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)</a>.</li>
22+
<li><a href="https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.randomnumbergenerator">System.Security.Cryptography.RandomNumberGenerator</a>.</li>
23+
</references>
24+
25+
</qhelp>

‎powershell/ql/src/queries/security/cwe-338/InsecureRandomness.ql‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Use of insecure random number generator in security-sensitive context
3+
* @description Using non-cryptographic random number generators such as Get-Random or System.Random
4+
* for security-sensitive operations can compromise security.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 7.5
8+
* @precision high
9+
* @id powershell/insecure-randomness
10+
* @tags security
11+
* external/cwe/cwe-330
12+
* external/cwe/cwe-338
13+
*/
14+
15+
import powershell
16+
import semmle.code.powershell.security.InsecureRandomnessQuery
17+
import InsecureRandomnessFlow::PathGraph
18+
19+
from InsecureRandomnessFlow::PathNode source, InsecureRandomnessFlow::PathNode sink
20+
where InsecureRandomnessFlow::flowPath(source, sink)
21+
select sink.getNode(), source, sink,
22+
"Insecure random value flows to " + sink.getNode().(Sink).getSinkDescription() + " from $@.",
23+
source.getNode(), "this insecure random source"

0 commit comments

Comments
 (0)