Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: Add view-component-input threat model #18466

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions docs/codeql/reusables/threat-model-description.rst
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ The less commonly used categories are:
- ``database-access-result`` which represents a database access. Currently only used by JavaScript.
- ``file-write`` which represents opening a file in write mode. Currently only used in C#.
- ``reverse-dns`` which represents reverse DNS lookups. Currently only used in Java.
- ``view-component-input`` which represents inputs to a React, Vue, or Angular component (also known as "props"). Currently only used by JavaScript/TypeScript.

When running a CodeQL analysis, the ``remote`` threat model is included by default. You can optionally include other threat models as appropriate when using the CodeQL CLI and in GitHub code scanning. For more information, see `Analyzing your code with CodeQL queries <https://docs.github.com/code-security/codeql-cli/getting-started-with-the-codeql-cli/analyzing-your-code-with-codeql-queries#including-model-packs-to-add-potential-sources-of-tainted-data>`__ and `Customizing your advanced setup for code scanning <https://docs.github.com/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning#extending-codeql-coverage-with-threat-models>`__.
10 changes: 10 additions & 0 deletions javascript/ql/lib/semmle/javascript/Concepts.qll
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ class ThreatModelSource extends DataFlow::Node instanceof ThreatModelSource::Ran

/** Gets a string that describes the type of this threat-model source. */
string getSourceType() { result = super.getSourceType() }

/**
* Holds if this is a source of data that is specific to the web browser environment.
*/
predicate isClientSideSource() { super.isClientSideSource() }
}

/** Provides a class for modeling new sources for specific threat-models. */
Expand All @@ -48,6 +53,11 @@ module ThreatModelSource {

/** Gets a string that describes the type of this threat-model source. */
abstract string getSourceType();

/**
* Holds if this is a source of data that is specific to the web browser environment.
*/
predicate isClientSideSource() { this.getThreatModel() = "view-component-input" }
}
}

Expand Down
47 changes: 47 additions & 0 deletions javascript/ql/lib/semmle/javascript/ViewComponentInput.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* Provides a classes and predicates for contributing to the `view-component-input` threat model.
*/

private import javascript

/**
* An input to a view component, such as React props.
*/
abstract class ViewComponentInput extends DataFlow::Node {
/** Gets a string that describes the type of this threat-model source. */
abstract string getSourceType();
}

private class ViewComponentInputAsThreatModelSource extends ThreatModelSource::Range instanceof ViewComponentInput
{
ViewComponentInputAsThreatModelSource() { not isSafeType(this.asExpr().getType()) }

final override string getThreatModel() { result = "view-component-input" }

final override string getSourceType() { result = ViewComponentInput.super.getSourceType() }
}

private predicate isSafeType(Type t) {
t instanceof NumberLikeType
or
t instanceof BooleanLikeType
or
t instanceof UndefinedType
or
t instanceof NullType
or
t instanceof VoidType
or
hasSafeTypes(t, t.(UnionType).getNumElementType())
or
isSafeType(t.(IntersectionType).getAnElementType())
}

/** Hold if the first `n` components of `t` are safe types. */
private predicate hasSafeTypes(UnionType t, int n) {
isSafeType(t.getElementType(0)) and
n = 1
or
isSafeType(t.getElementType(n - 1)) and
hasSafeTypes(t, n - 1)
}
14 changes: 14 additions & 0 deletions javascript/ql/lib/semmle/javascript/frameworks/Angular2.qll
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ private import semmle.javascript.security.dataflow.CodeInjectionCustomizations
private import semmle.javascript.security.dataflow.ClientSideUrlRedirectCustomizations
private import semmle.javascript.DynamicPropertyAccess
private import semmle.javascript.dataflow.internal.PreCallGraphStep
private import semmle.javascript.ViewComponentInput

/**
* Provides classes for working with Angular (also known as Angular 2.x) applications.
Expand Down Expand Up @@ -575,4 +576,17 @@ module Angular2 {
)
}
}

private class InputFieldAsViewComponentInput extends ViewComponentInput {
InputFieldAsViewComponentInput() {
this =
API::moduleImport("@angular/core")
.getMember("Input")
.getReturn()
.getADecoratedMember()
.asSource()
}

override string getSourceType() { result = "Angular component input field" }
}
}
7 changes: 7 additions & 0 deletions javascript/ql/lib/semmle/javascript/frameworks/React.qll
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import javascript
private import semmle.javascript.dataflow.internal.FlowSteps as FlowSteps
private import semmle.javascript.dataflow.internal.PreCallGraphStep
private import semmle.javascript.ViewComponentInput

/**
* Gets a reference to the 'React' object.
Expand Down Expand Up @@ -868,3 +869,9 @@ private class PropsFlowStep extends PreCallGraphStep {
)
}
}

private class ReactPropAsViewComponentInput extends ViewComponentInput {
ReactPropAsViewComponentInput() { this = any(ReactComponent c).getADirectPropsAccess() }

override string getSourceType() { result = "React props" }
}
89 changes: 81 additions & 8 deletions javascript/ql/lib/semmle/javascript/frameworks/Vue.qll
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/

import javascript
import semmle.javascript.ViewComponentInput

module Vue {
/** The global variable `Vue`, as an API graph entry point. */
Expand Down Expand Up @@ -85,17 +86,16 @@ module Vue {
* A class with a `@Component` decorator, making it usable as an "options" object in Vue.
*/
class ClassComponent extends DataFlow::ClassNode {
private ClassDefinition cls;
DataFlow::Node decorator;

ClassComponent() {
exists(ClassDefinition cls |
this = cls.flow() and
cls.getADecorator().getExpression() = decorator.asExpr() and
(
componentDecorator().flowsTo(decorator)
or
componentDecorator().getACall() = decorator
)
this = cls.flow() and
cls.getADecorator().getExpression() = decorator.asExpr() and
(
componentDecorator().flowsTo(decorator)
or
componentDecorator().getACall() = decorator
)
}

Expand All @@ -105,6 +105,9 @@ module Vue {
* These options correspond to the options one would pass to `new Vue({...})` or similar.
*/
API::Node getDecoratorOptions() { result = decorator.(API::CallNode).getParameter(0) }

/** Gets the AST node for the class definition. */
ClassDefinition getClassDefinition() { result = cls }
}

private string memberKindVerb(DataFlow::MemberKind kind) {
Expand Down Expand Up @@ -460,6 +463,12 @@ module Vue {

SingleFileComponent() { this = MkSingleFileComponent(file) }

/** Gets a call to `defineProps` in this component. */
DataFlow::CallNode getDefinePropsCall() {
result = DataFlow::globalVarRef("defineProps").getACall() and
result.getFile() = file
}

override Template::Element getTemplateElement() {
exists(HTML::Element e | result.(Template::HtmlElement).getElement() = e |
e.getFile() = file and
Expand Down Expand Up @@ -697,4 +706,68 @@ module Vue {

override ClientSideRemoteFlowKind getKind() { result = kind }
}

/**
* Holds if the given type annotation indicates a value that is not typically considered taintable.
*/
private predicate isSafeType(TypeAnnotation type) {
type.isBooleany() or
type.isNumbery() or
type.isRawFunction() or
type instanceof FunctionTypeExpr
}

/**
* Holds if the given field has a type that indicates that is can not contain a taintable value.
*/
private predicate isSafeField(FieldDeclaration field) { isSafeType(field.getTypeAnnotation()) }

private DataFlow::Node getPropSpec(Component component) {
result = component.getOption("props")
or
result = component.(SingleFileComponent).getDefinePropsCall().getArgument(0)
}

/**
* Holds if `component` has an input prop with the given name, that is of a taintable type.
*/
private predicate hasTaintableProp(Component component, string name) {
exists(DataFlow::SourceNode spec | spec = getPropSpec(component).getALocalSource() |
spec.(DataFlow::ArrayCreationNode).getAnElement().getStringValue() = name
or
exists(DataFlow::PropWrite write |
write = spec.getAPropertyWrite(name) and
not DataFlow::globalVarRef(["Number", "Boolean"]).flowsTo(write.getRhs())
)
)
or
exists(FieldDeclaration field |
field = component.getAsClassComponent().getClassDefinition().getField(name) and
DataFlow::moduleMember("vue-property-decorator", "Prop")
.getACall()
.flowsToExpr(field.getADecorator().getExpression()) and
not isSafeField(field)
)
or
// defineProps() can be called with only type arguments and then the Vue compiler will
// infer the prop types.
exists(CallExpr call, FieldDeclaration field |
call = component.(SingleFileComponent).getDefinePropsCall().asExpr() and
field = call.getTypeArgument(0).(InterfaceTypeExpr).getMember(name) and
not isSafeField(field)
)
}

private class PropAsViewComponentInput extends ViewComponentInput {
PropAsViewComponentInput() {
exists(Component component, string name | hasTaintableProp(component, name) |
this = component.getAnInstanceRef().getAPropertyRead(name)
or
// defineProps() returns the props
this = component.(SingleFileComponent).getDefinePropsCall().getAPropertyRead(name)
)
}

override string getSourceType() { result = "Vue prop" }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ module CommandInjection {
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source instanceof ActiveThreatModelSource {
ActiveThreatModelSourceAsSource() { not this instanceof ClientSideRemoteFlowSource }
ActiveThreatModelSourceAsSource() { not this.isClientSideSource() }

override string getSourceType() { result = "a user-provided value" }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ module CorsMisconfigurationForCredentials {
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source instanceof ActiveThreatModelSource {
ActiveThreatModelSourceAsSource() { not this instanceof ClientSideRemoteFlowSource }
ActiveThreatModelSourceAsSource() { not this.isClientSideSource() }
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ deprecated class LogInjectionConfiguration extends TaintTracking::Configuration
* A source of remote user controlled input.
*/
class RemoteSource extends Source instanceof RemoteFlowSource {
RemoteSource() { not this instanceof ClientSideRemoteFlowSource }
RemoteSource() { not this.isClientSideSource() }
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ module RegExpInjection {
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source instanceof ActiveThreatModelSource {
ActiveThreatModelSourceAsSource() { not this instanceof ClientSideRemoteFlowSource }
ActiveThreatModelSourceAsSource() { not this.isClientSideSource() }
}

private import IndirectCommandInjectionCustomizations
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,18 @@ private module Cached {

/**
* A source of remote input in a web browser environment.
*
* Note that this does not include `view-component-input` sources even if that threat model has been enabled by the user.
* Consider using the predicate `ThreatModelSource#isClientSideSource()` to check for a broader class of client-side sources.
*/
cached
abstract class ClientSideRemoteFlowSource extends RemoteFlowSource {
/** Gets a string indicating what part of the browser environment this was derived from. */
cached
abstract ClientSideRemoteFlowKind getKind();

cached
final override predicate isClientSideSource() { any() }
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ module RequestForgery {
not this.(ClientSideRemoteFlowSource).getKind().isPathOrUrl()
}

override predicate isServerSide() { not this instanceof ClientSideRemoteFlowSource }
override predicate isServerSide() { not super.isClientSideSource() }
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ module ResourceExhaustion {
private class ActiveThreatModelSourceAsSource extends Source instanceof ActiveThreatModelSource {
ActiveThreatModelSourceAsSource() {
// exclude source that only happen client-side
not this instanceof ClientSideRemoteFlowSource and
not this.isClientSideSource() and
not this = DataFlow::parameterNode(any(PostMessageEventHandler pmeh).getEventParameter())
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,7 @@ module TaintedPath {
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source instanceof ActiveThreatModelSource {
ActiveThreatModelSourceAsSource() { not this instanceof ClientSideRemoteFlowSource }
ActiveThreatModelSourceAsSource() { not this.isClientSideSource() }
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
category: majorAnalysis
---
* Added a new threat model kind called `view-component-input`, which can enabled with [advanced setup](https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning#extending-codeql-coverage-with-threat-models).
When enabled, all React props, Vue props, and input fields in an Angular component are seen as taint sources, even if none of the corresponding instantiation sites appear to pass in a tainted value.
Some users may prefer this as a "defense in depth" option but note that it may result in false positives.
Regardless of whether the threat model is enabled, CodeQL will propagate taint from the instantiation sites of such components into the components themselves.
13 changes: 3 additions & 10 deletions javascript/ql/src/meta/alerts/TaintSources.ql
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,6 @@
import javascript
import meta.internal.TaintMetrics

string getName(DataFlow::Node node) {
result = node.(RemoteFlowSource).getSourceType()
or
not node instanceof RemoteFlowSource and
result = "Taint source"
}

from DataFlow::Node node
where node = relevantTaintSource()
select node, getName(node)
from ThreatModelSource node
where node = relevantTaintSource() and node.getThreatModel() = "remote"
select node, getTaintSourceName(node)
19 changes: 19 additions & 0 deletions javascript/ql/src/meta/alerts/ThreatModelSources.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* @name Threat model sources
* @description Sources of possibly untrusted input that can be configured via threat models.
* @kind problem
* @problem.severity recommendation
* @id js/meta/alerts/threat-model-sources
* @tags meta
* @precision very-low
*/

import javascript
import meta.internal.TaintMetrics

from ThreatModelSource node, string threatModel
where
node = relevantTaintSource() and
threatModel = node.getThreatModel() and
threatModel != "remote" // "remote" is reported by TaintSources.ql
select node, getTaintSourceName(node) + " (\"" + threatModel + "\" threat model)"
11 changes: 9 additions & 2 deletions javascript/ql/src/meta/internal/TaintMetrics.qll
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ DataFlow::Node relevantTaintSink(string kind) {
DataFlow::Node relevantTaintSink() { result = relevantTaintSink(_) }

/**
* Gets a relevant remote flow source.
* Gets a relevant threat model source.
*/
RemoteFlowSource relevantTaintSource() { not result.getFile() instanceof IgnoredFile }
ThreatModelSource relevantTaintSource() { not result.getFile() instanceof IgnoredFile }

/**
* Gets the output of a call that shows intent to sanitize a value
Expand All @@ -100,3 +100,10 @@ DataFlow::Node relevantSanitizerInput() {
result = any(HtmlSanitizerCall call).getInput() and
not result.getFile() instanceof IgnoredFile
}

string getTaintSourceName(DataFlow::Node node) {
result = node.(ThreatModelSource).getSourceType()
or
not node instanceof ThreatModelSource and
result = "Taint source"
}
Loading