Merge pull request #312 from github/rb/stored-xss-1

Implement `rb/stored-xss` query
This commit is contained in:
Alex Ford
2021-10-08 10:33:11 +01:00
committed by GitHub
32 changed files with 5287 additions and 80 deletions

View File

@@ -538,3 +538,32 @@ module XmlParserCall {
abstract predicate externalEntitiesEnabled();
}
}
/**
* A data-flow node that may represent a database object in an ORM system.
*
* Extend this class to refine existing API models. If you want to model new APIs,
* extend `OrmInstantiation::Range` instead.
*/
class OrmInstantiation extends DataFlow::Node instanceof OrmInstantiation::Range {
/** Holds if a call to `methodName` on this instance may return a field of this ORM object. */
bindingset[methodName]
predicate methodCallMayAccessField(string methodName) {
super.methodCallMayAccessField(methodName)
}
}
/** Provides a class for modeling new ORM object instantiation APIs. */
module OrmInstantiation {
/**
* A data-flow node that may represent a database object in an ORM system.
*
* Extend this class to model new APIs. If you want to refine existing API models,
* extend `OrmInstantiation` instead.
*/
abstract class Range extends DataFlow::Node {
/** Holds if a call to `methodName` on this instance may return a field of this ORM object. */
bindingset[methodName]
abstract predicate methodCallMayAccessField(string methodName);
}
}

View File

@@ -0,0 +1,7 @@
/**
* Provides classes for performing local (intra-procedural) and
* global (inter-procedural) data flow analyses.
*/
module DataFlow2 {
import codeql.ruby.dataflow.internal.DataFlowImpl2
}

View File

@@ -166,6 +166,35 @@ class ConstantWriteAccess extends ConstantAccess {
}
override string getAPrimaryQlClass() { result = "ConstantWriteAccess" }
/**
* Gets the fully qualified name for this constant, based on the context in
* which it is defined.
*
* For example, given
* ```rb
* module Foo
* module Bar
* class Baz
* end
* end
* CONST_A = "a"
* end
* ```
*
* the constant `Baz` has the fully qualified name `Foo::Bar::Baz`, and
* `CONST_A` has the fully qualified name `Foo::CONST_A`.
*/
string getQualifiedName() {
/* get the qualified name for the parent module, then append w */
exists(ConstantWriteAccess parent | parent = this.getEnclosingModule() |
result = parent.getQualifiedName() + "::" + this.getName()
)
or
/* base case - there's no parent module */
not exists(ConstantWriteAccess parent | parent = this.getEnclosingModule()) and
result = this.getName()
}
}
/**

File diff suppressed because it is too large Load Diff

View File

@@ -69,7 +69,7 @@ class ActionControllerActionMethod extends Method, HTTP::Server::RequestHandler:
* `<sourcePrefix>app/views/<subpath>/<method_name>.html.erb`.
*/
ErbFile getDefaultTemplateFile() {
controllerTemplatesFolder(this.getControllerClass(), result.getParentContainer()) and
controllerTemplateFile(this.getControllerClass(), result) and
result.getBaseName() = this.getName() + ".html.erb"
}
@@ -220,25 +220,40 @@ class ActionControllerHelperMethod extends Method {
* if such a controller class exists.
*/
ActionControllerControllerClass getAssociatedControllerClass(ErbFile f) {
controllerTemplatesFolder(result, f.getParentContainer())
// There is a direct mapping from template file to controller class
controllerTemplateFile(result, f)
or
// The template `f` is a partial, and it is rendered from within another
// template file, `fp`. In this case, `f` inherits the associated
// controller classes from `fp`.
f.isPartial() and
exists(RenderCall r, ErbFile fp |
r.getLocation().getFile() = fp and
r.getTemplateFile() = f and
result = getAssociatedControllerClass(fp)
)
}
// TODO: improve layout support, e.g. for `layout` method
// https://guides.rubyonrails.org/layouts_and_rendering.html
/**
* Holds if `templatesFolder` is in the correct location to contain template
* files "belonging" to the given `ActionControllerControllerClass`, according
* to Rails conventions.
* Holds if `templatesFile` is a viable file "belonging" to the given
* `ActionControllerControllerClass`, according to Rails conventions.
*
* In particular, this means that an action method in `cls` will by default
* render a correspondingly named template file within `templatesFolder`.
* This handles mappings between controllers in `app/controllers/`, and
* templates in `app/views/` and `app/views/layouts/`.
*/
predicate controllerTemplatesFolder(ActionControllerControllerClass cls, Folder templatesFolder) {
predicate controllerTemplateFile(ActionControllerControllerClass cls, ErbFile templateFile) {
exists(string templatesPath, string sourcePrefix, string subPath, string controllerPath |
controllerPath = cls.getLocation().getFile().getRelativePath() and
templatesPath = templatesFolder.getRelativePath() and
templatesPath = templateFile.getParentContainer().getRelativePath() and
// `sourcePrefix` is either a prefix path ending in a slash, or empty if
// the rails app is at the source root
sourcePrefix = [controllerPath.regexpCapture("^(.*/)app/controllers/(?:.*?)/(?:[^/]*)$", 1), ""] and
controllerPath = sourcePrefix + "app/controllers/" + subPath + "_controller.rb" and
templatesPath = sourcePrefix + "app/views/" + subPath
(
templatesPath = sourcePrefix + "app/views/" + subPath or
templateFile.getRelativePath().matches(sourcePrefix + "app/views/layouts/" + subPath + "%")
)
)
}

View File

@@ -2,7 +2,10 @@ private import codeql.ruby.AST
private import codeql.ruby.Concepts
private import codeql.ruby.controlflow.CfgNodes
private import codeql.ruby.DataFlow
private import codeql.ruby.dataflow.internal.DataFlowDispatch
private import codeql.ruby.ast.internal.Module
private import codeql.ruby.ApiGraphs
private import codeql.ruby.frameworks.StandardLibrary
private class ActiveRecordBaseAccess extends ConstantReadAccess {
ActiveRecordBaseAccess() {
@@ -18,6 +21,25 @@ private class ApplicationRecordAccess extends ConstantReadAccess {
ApplicationRecordAccess() { this.getName() = "ApplicationRecord" }
}
/// See https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html
private string activeRecordPersistenceInstanceMethodName() {
result =
[
"becomes", "becomes!", "decrement", "decrement!", "delete", "delete!", "destroy", "destroy!",
"destroyed?", "increment", "increment!", "new_record?", "persisted?",
"previously_new_record?", "reload", "save", "save!", "toggle", "toggle!", "touch", "update",
"update!", "update_attribute", "update_column", "update_columns"
]
}
// Methods with these names are defined for all active record model instances,
// so they are unlikely to refer to a database field.
private predicate isBuiltInMethodForActiveRecordModelInstance(string methodName) {
methodName = activeRecordPersistenceInstanceMethodName() or
methodName = basicObjectInstanceMethodName() or
methodName = objectInstanceMethodName()
}
/**
* A `ClassDeclaration` for a class that extends `ActiveRecord::Base`. For example,
*
@@ -40,6 +62,37 @@ class ActiveRecordModelClass extends ClassDeclaration {
other.getModule() = resolveScopeExpr(this.getSuperclassExpr())
)
}
// Gets the class declaration for this class and all of its super classes
private ModuleBase getAllClassDeclarations() {
result = this.getModule().getSuperClass*().getADeclaration()
}
/**
* Gets methods defined in this class that may access a field from the database.
*/
Method getAPotentialFieldAccessMethod() {
// It's a method on this class or one of its super classes
result = this.getAllClassDeclarations().getAMethod() and
// There is a value that can be returned by this method which may include field data
exists(DataFlow::Node returned, ActiveRecordInstanceMethodCall cNode, MethodCall c |
exprNodeReturnedFrom(returned, result) and
cNode.flowsTo(returned) and
c = cNode.asExpr().getExpr()
|
// The referenced method is not built-in, and...
not isBuiltInMethodForActiveRecordModelInstance(c.getMethodName()) and
(
// ...The receiver does not have a matching method definition, or...
not exists(
cNode.getInstance().getClass().getAllClassDeclarations().getMethod(c.getMethodName())
)
or
// ...the called method can access a field
c.getATarget() = cNode.getInstance().getClass().getAPotentialFieldAccessMethod()
)
)
}
}
/** A class method call whose receiver is an `ActiveRecordModelClass`. */
@@ -160,5 +213,107 @@ class ActiveRecordSqlExecutionRange extends SqlExecution::Range {
override DataFlow::Node getSql() { result = this }
}
// TODO: model `ActiveRecord` sanitizers
// https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html
/**
* A node that may evaluate to one or more `ActiveRecordModelClass` instances.
*/
abstract class ActiveRecordModelInstantiation extends OrmInstantiation::Range,
DataFlow::LocalSourceNode {
abstract ActiveRecordModelClass getClass();
bindingset[methodName]
override predicate methodCallMayAccessField(string methodName) {
// The method is not a built-in, and...
not isBuiltInMethodForActiveRecordModelInstance(methodName) and
(
// ...There is no matching method definition in the class, or...
not exists(this.getClass().getMethod(methodName))
or
// ...the called method can access a field.
exists(Method m | m = this.getClass().getAPotentialFieldAccessMethod() |
m.getName() = methodName
)
)
}
}
// Names of class methods on ActiveRecord models that may return one or more
// instances of that model. This also includes the `initialize` method.
// See https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html
private string finderMethodName() {
exists(string baseName |
baseName =
[
"fifth", "find", "find_by", "find_or_initialize_by", "find_or_create_by", "first",
"forty_two", "fourth", "last", "second", "second_to_last", "take", "third", "third_to_last"
] and
result = baseName + ["", "!"]
)
or
result = "new"
}
// Gets the "final" receiver in a chain of method calls.
// For example, in `Foo.bar`, this would give the `Foo` access, and in
// `foo.bar.baz("arg")` it would give the `foo` variable access
private Expr getUltimateReceiver(MethodCall call) {
exists(Expr recv |
recv = call.getReceiver() and
(
result = getUltimateReceiver(recv)
or
not recv instanceof MethodCall and result = recv
)
)
}
// A call to `find`, `where`, etc. that may return active record model object(s)
private class ActiveRecordModelFinderCall extends ActiveRecordModelInstantiation, DataFlow::CallNode {
private MethodCall call;
private ActiveRecordModelClass cls;
private Expr recv;
ActiveRecordModelFinderCall() {
call = this.asExpr().getExpr() and
recv = getUltimateReceiver(call) and
resolveConstant(recv) = cls.getQualifiedName() and
call.getMethodName() = finderMethodName()
}
final override ActiveRecordModelClass getClass() { result = cls }
}
// A `self` reference that may resolve to an active record model object
private class ActiveRecordModelClassSelfReference extends ActiveRecordModelInstantiation {
private ActiveRecordModelClass cls;
ActiveRecordModelClassSelfReference() {
exists(Self s |
s.getEnclosingModule() = cls and
s.getEnclosingMethod() = cls.getAMethod() and
s = this.asExpr().getExpr()
)
}
final override ActiveRecordModelClass getClass() { result = cls }
}
// A (locally tracked) active record model object
private class ActiveRecordInstance extends DataFlow::Node {
private ActiveRecordModelInstantiation instantiation;
ActiveRecordInstance() { this = instantiation or instantiation.flowsTo(this) }
ActiveRecordModelClass getClass() { result = instantiation.getClass() }
}
// A call whose receiver may be an active record model object
private class ActiveRecordInstanceMethodCall extends DataFlow::CallNode {
private ActiveRecordInstance instance;
ActiveRecordInstanceMethodCall() { this.getReceiver() = instance }
ActiveRecordInstance getInstance() { result = instance }
}

View File

@@ -65,35 +65,39 @@ private predicate isPrivateKernelMethod(string method) {
]
}
string basicObjectInstanceMethodName() {
result in [
"equal?", "instance_eval", "instance_exec", "method_missing", "singleton_method_added",
"singleton_method_removed", "singleton_method_undefined"
]
}
/**
* Instance methods on `BasicObject`, which are available to all classes.
*/
class BasicObjectInstanceMethodCall extends UnknownMethodCall {
BasicObjectInstanceMethodCall() {
this.getMethodName() in [
"equal?", "instance_eval", "instance_exec", "method_missing", "singleton_method_added",
"singleton_method_removed", "singleton_method_undefined"
]
}
BasicObjectInstanceMethodCall() { this.getMethodName() = basicObjectInstanceMethodName() }
}
string objectInstanceMethodName() {
result in [
"!~", "<=>", "===", "=~", "callable_methods", "define_singleton_method", "display",
"do_until", "do_while", "dup", "enum_for", "eql?", "extend", "f", "freeze", "h", "hash",
"inspect", "instance_of?", "instance_variable_defined?", "instance_variable_get",
"instance_variable_set", "instance_variables", "is_a?", "itself", "kind_of?",
"matching_methods", "method", "method_missing", "methods", "nil?", "object_id",
"private_methods", "protected_methods", "public_method", "public_methods", "public_send",
"remove_instance_variable", "respond_to?", "respond_to_missing?", "send",
"shortest_abbreviation", "singleton_class", "singleton_method", "singleton_methods", "taint",
"tainted?", "to_enum", "to_s", "trust", "untaint", "untrust", "untrusted?"
]
}
/**
* Instance methods on `Object`, which are available to all classes except `BasicObject`.
*/
class ObjectInstanceMethodCall extends UnknownMethodCall {
ObjectInstanceMethodCall() {
this.getMethodName() in [
"!~", "<=>", "===", "=~", "callable_methods", "define_singleton_method", "display",
"do_until", "do_while", "dup", "enum_for", "eql?", "extend", "f", "freeze", "h", "hash",
"inspect", "instance_of?", "instance_variable_defined?", "instance_variable_get",
"instance_variable_set", "instance_variables", "is_a?", "itself", "kind_of?",
"matching_methods", "method", "method_missing", "methods", "nil?", "object_id",
"private_methods", "protected_methods", "public_method", "public_methods", "public_send",
"remove_instance_variable", "respond_to?", "respond_to_missing?", "send",
"shortest_abbreviation", "singleton_class", "singleton_method", "singleton_methods",
"taint", "tainted?", "to_enum", "to_s", "trust", "untaint", "untrust", "untrusted?"
]
}
ObjectInstanceMethodCall() { this.getMethodName() = objectInstanceMethodName() }
}
/**

View File

@@ -3,7 +3,7 @@
*
* Note, for performance reasons: only import this file if
* `ReflectedXSS::Configuration` is needed, otherwise
* `ReflectedXSSCustomizations` should be imported instead.
* `XSS::ReflectedXSS` should be imported instead.
*/
private import ruby
@@ -14,7 +14,7 @@ import codeql.ruby.TaintTracking
* Provides a taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
*/
module ReflectedXSS {
import ReflectedXSSCustomizations::ReflectedXSS
import XSS::ReflectedXSS
/**
* A taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.

View File

@@ -0,0 +1,40 @@
/**
* Provides a taint-tracking configuration for reasoning about stored
* cross-site scripting vulnerabilities.
*
* Note, for performance reasons: only import this file if
* `StoredXSS::Configuration` is needed, otherwise
* `XSS::StoredXSS` should be imported instead.
*/
import ruby
import codeql.ruby.DataFlow
import codeql.ruby.TaintTracking
module StoredXSS {
import XSS::StoredXSS
/**
* A taint-tracking configuration for reasoning about Stored XSS.
*/
class Configuration extends TaintTracking::Configuration {
Configuration() { this = "StoredXss" }
override predicate isSource(DataFlow::Node source) { source instanceof Source }
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
override predicate isSanitizer(DataFlow::Node node) {
super.isSanitizer(node) or
node instanceof Sanitizer
}
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
guard instanceof SanitizerGuard
}
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
isAdditionalXSSTaintStep(node1, node2)
}
}
}

View File

@@ -1,5 +1,10 @@
/**
* Provides classes and predicates used by the XSS queries.
*/
private import ruby
private import codeql.ruby.DataFlow
private import codeql.ruby.DataFlow2
private import codeql.ruby.CFG
private import codeql.ruby.Concepts
private import codeql.ruby.Frameworks
@@ -7,40 +12,34 @@ private import codeql.ruby.frameworks.ActionController
private import codeql.ruby.frameworks.ActionView
private import codeql.ruby.dataflow.RemoteFlowSources
private import codeql.ruby.dataflow.BarrierGuards
import codeql.ruby.dataflow.internal.DataFlowDispatch
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.dataflow.internal.DataFlowDispatch
/**
* Provides default sources, sinks and sanitizers for detecting
* "reflected server-side cross-site scripting"
* vulnerabilities, as well as extension points for adding your own.
* "server-side cross-site scripting" vulnerabilities, as well as
* extension points for adding your own.
*/
module ReflectedXSS {
private module Shared {
/**
* A data flow source for "reflected server-side cross-site scripting" vulnerabilities.
* A data flow source for "server-side cross-site scripting" vulnerabilities.
*/
abstract class Source extends DataFlow::Node { }
/**
* A data flow sink for "reflected server-side cross-site scripting" vulnerabilities.
* A data flow sink for "server-side cross-site scripting" vulnerabilities.
*/
abstract class Sink extends DataFlow::Node { }
/**
* A sanitizer for "reflected server-side cross-site scripting" vulnerabilities.
* A sanitizer for "server-side cross-site scripting" vulnerabilities.
*/
abstract class Sanitizer extends DataFlow::Node { }
/**
* A sanitizer guard for "reflected server-side cross-site scripting" vulnerabilities.
* A sanitizer guard for "server-side cross-site scripting" vulnerabilities.
*/
abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
/**
* A source of remote user input, considered as a flow source.
*/
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
private class ErbOutputMethodCallArgumentNode extends DataFlow::Node {
private MethodCall call;
@@ -124,9 +123,9 @@ module ReflectedXSS {
}
/**
* An additional step that is taint-preserving in the context of reflected XSS.
* An additional step that is preserves dataflow in the context of XSS.
*/
predicate isAdditionalXSSTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
predicate isAdditionalXSSFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
// node1 is a `locals` argument to a render call...
exists(RenderCall call, Pair kvPair, string hashKey |
call.getLocals().getAKeyValuePair() = kvPair and
@@ -198,3 +197,115 @@ module ReflectedXSS {
)
}
}
/**
* Provides default sources, sinks and sanitizers for detecting
* "reflected cross-site scripting" vulnerabilities, as well as
* extension points for adding your own.
*/
module ReflectedXSS {
/** A data flow source for stored XSS vulnerabilities. */
abstract class Source extends Shared::Source { }
/** A data flow sink for stored XSS vulnerabilities. */
abstract class Sink extends Shared::Sink { }
/** A sanitizer for stored XSS vulnerabilities. */
abstract class Sanitizer extends Shared::Sanitizer { }
/** A sanitizer guard for stored XSS vulnerabilities. */
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
// Consider all arbitrary XSS sinks to be reflected XSS sinks
private class AnySink extends Sink instanceof Shared::Sink { }
// Consider all arbitrary XSS sanitizers to be reflected XSS sanitizers
private class AnySanitizer extends Sanitizer instanceof Shared::Sanitizer { }
// Consider all arbitrary XSS sanitizer guards to be reflected XSS sanitizer guards
private class AnySanitizerGuard extends SanitizerGuard instanceof Shared::SanitizerGuard {
override predicate checks(CfgNode expr, boolean branch) {
Shared::SanitizerGuard.super.checks(expr, branch)
}
}
/**
* An additional step that is preserves dataflow in the context of reflected XSS.
*/
predicate isAdditionalXSSTaintStep = Shared::isAdditionalXSSFlowStep/2;
/**
* A source of remote user input, considered as a flow source.
*/
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
}
private module OrmTracking {
/**
* A data flow configuration to track flow from finder calls to field accesses.
*/
class Configuration extends DataFlow2::Configuration {
Configuration() { this = "OrmTracking" }
override predicate isSource(DataFlow2::Node source) { source instanceof OrmInstantiation }
// Select any call node and narrow down later
override predicate isSink(DataFlow2::Node sink) { sink instanceof DataFlow2::CallNode }
override predicate isAdditionalFlowStep(DataFlow2::Node node1, DataFlow2::Node node2) {
Shared::isAdditionalXSSFlowStep(node1, node2)
or
// Propagate flow through arbitrary method calls
node2.(DataFlow2::CallNode).getReceiver() = node1
or
// Propagate flow through "or" expressions `or`/`||`
node2.asExpr().getExpr().(LogicalOrExpr).getAnOperand() = node1.asExpr().getExpr()
}
}
}
module StoredXSS {
/** A data flow source for stored XSS vulnerabilities. */
abstract class Source extends Shared::Source { }
/** A data flow sink for stored XSS vulnerabilities. */
abstract class Sink extends Shared::Sink { }
/** A sanitizer for stored XSS vulnerabilities. */
abstract class Sanitizer extends Shared::Sanitizer { }
/** A sanitizer guard for stored XSS vulnerabilities. */
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
// Consider all arbitrary XSS sinks to be stored XSS sinks
private class AnySink extends Sink instanceof Shared::Sink { }
// Consider all arbitrary XSS sanitizers to be stored XSS sanitizers
private class AnySanitizer extends Sanitizer instanceof Shared::Sanitizer { }
// Consider all arbitrary XSS sanitizer guards to be stored XSS sanitizer guards
private class AnySanitizerGuard extends SanitizerGuard instanceof Shared::SanitizerGuard {
override predicate checks(CfgNode expr, boolean branch) {
Shared::SanitizerGuard.super.checks(expr, branch)
}
}
/**
* An additional step that preserves dataflow in the context of stored XSS.
*/
predicate isAdditionalXSSTaintStep = Shared::isAdditionalXSSFlowStep/2;
private class OrmFieldAsSource extends Source instanceof DataFlow2::CallNode {
OrmFieldAsSource() {
exists(OrmTracking::Configuration subConfig, DataFlow2::CallNode subSrc, MethodCall call |
subConfig.hasFlow(subSrc, this) and
call = this.asExpr().getExpr() and
subSrc.(OrmInstantiation).methodCallMayAccessField(call.getMethodName())
)
}
}
/** A file read, considered as a flow source for stored XSS. */
private class FileSystemReadAccessAsSource extends Source instanceof FileSystemReadAccess { }
// TODO: Consider `FileNameSource` flowing to script tag `src` attributes and similar
}

View File

@@ -65,32 +65,6 @@ newtype DefLoc =
not exists(MethodBase m | m.getAChild+() = write)
}
/**
* Gets the fully qualified name for a constant, based on the context in which it is defined.
*
* For example, given
* ```ruby
* module Foo
* module Bar
* class Baz
* end
* end
* end
* ```
*
* the constant `Baz` has the fully qualified name `Foo::Bar::Baz`.
*/
string constantQualifiedName(ConstantWriteAccess w) {
/* get the qualified name for the parent module, then append w */
exists(ConstantWriteAccess parent | parent = w.getEnclosingModule() |
result = constantQualifiedName(parent) + "::" + w.getName()
)
or
/* base case - there's no parent module */
not exists(ConstantWriteAccess parent | parent = w.getEnclosingModule()) and
result = w.getName()
}
/**
* Gets the constant write that defines the given constant.
* Modules often don't have a unique definition, as they are opened multiple times in different
@@ -100,7 +74,7 @@ string constantQualifiedName(ConstantWriteAccess w) {
ConstantWriteAccess definitionOf(ConstantReadAccess r) {
result =
min(ConstantWriteAccess w |
constantQualifiedName(w) = resolveConstant(r)
w.getQualifiedName() = resolveConstant(r)
|
w order by w.getLocation().toString()
)

View File

@@ -33,13 +33,13 @@
<code>params[:user_name]</code> content within the output tags will be
HTML-escaped automatically before being emitted.
</p>
<sample src="examples/safe.html.erb" />
<sample src="examples/reflected_xss_safe.html.erb" />
<p>
However, the following example is unsafe because user-controlled input is
emitted without escaping, since it is marked as <code>html_safe</code>.
</p>
<sample src="examples/reflective_xss.html.erb" />
<sample src="examples/reflected_xss_unsafe.html.erb" />
</example>
<references>

View File

@@ -4,6 +4,7 @@
* allows for a cross-site scripting vulnerability.
* @kind path-problem
* @problem.severity error
* @security-severity 6.1
* @sub-severity high
* @precision high
* @id rb/reflected-xss

View File

@@ -0,0 +1,71 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
Directly writing an uncontrolled stored value (for example, a database
field) to a webpage, without properly sanitizing the value first, allows
for a cross-site scripting vulnerability.
</p>
<p>
This kind of vulnerability is also called <i>stored</i> cross-site
scripting, to distinguish it from other types of cross-site scripting.
</p>
</overview>
<recommendation>
<p>
To guard against stored cross-site scripting, consider escaping before
using uncontrolled stored values to create HTML content. Some frameworks,
such as Rails, perform this escaping implicitly and by default.
</p>
<p>
Take care when using methods such as <code>html_safe</code> or
<code>raw</code>. They can be used to emit a string without escaping
it, and should only be used when the string has already been manually
escaped (for example, with the Rails <code>html_escape</code> method),
or when the content is otherwise guaranteed to be safe (such as a
hard-coded string).
</p>
</recommendation>
<example>
<p>
The following example is safe because the
<code>user.name</code> content within the output tags will be
HTML-escaped automatically before being emitted.
</p>
<sample src="examples/stored_xss_rails_safe.html.erb" />
<p>
However, the following example may be unsafe because
<code>user.name</code> is emitted without escaping, since it is marked as
<code>html_safe</code>. If the <code>name</code> is not sanitized before
being written to the database, then an attacker could use this to insert
arbitrary content into the HTML output, including scripts.
</p>
<sample src="examples/stored_xss_rails_unsafe.html.erb" />
<p>
In the next example, content from a file on disk is inserted literally
into HTML content. This approach is sometimes used to load script
content, such as extensions for a web application, from files on disk.
Care should taken in these cases to ensure both that the loaded files are
trusted, and that the file cannot be modified by untrusted users.
</p>
<sample src="examples/stored_xss_file_unsafe.html.erb" />
</example>
<references>
<li>
OWASP:
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Ruby_on_Rails_Cheat_Sheet.html#cross-site-scripting-xss">XSS
Ruby on Rails Cheatsheet</a>.
</li>
<li>
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
</li>
</references>
</qhelp>

View File

@@ -0,0 +1,23 @@
/**
* @name Stored cross-site scripting
* @description Using uncontrolled stored values in HTML allows for
* a stored cross-site scripting vulnerability.
* @kind path-problem
* @problem.severity error
* @security-severity 6.1
* @precision high
* @id rb/stored-xss
* @tags security
* external/cwe/cwe-079
* external/cwe/cwe-116
*/
import ruby
import codeql.ruby.security.StoredXSSQuery
import codeql.ruby.DataFlow
import DataFlow::PathGraph
from StoredXSS::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to $@",
source.getNode(), "stored value"

View File

@@ -0,0 +1,3 @@
<script>
<%= File.read(File.join(SCRIPT_DIR, "script.js")).html_safe %>
</script>

View File

@@ -0,0 +1,2 @@
<% user = User.find(1) %>
<p>Hello <%= user.name %>!</p>

View File

@@ -0,0 +1,2 @@
<% user = User.find(1) %>
<p>Hello <%= user.name.html_safe %>!</p>

View File

@@ -55,3 +55,16 @@ constantValue
| constants.rb:39:6:39:31 | MAX_SIZE | constants.rb:37:30:37:33 | 1024 |
| constants.rb:41:6:41:13 | GREETING | constants.rb:17:12:17:64 | ... + ... |
| constants.rb:42:6:42:15 | GREETING | constants.rb:17:12:17:64 | ... + ... |
constantWriteAccessQualifiedName
| constants.rb:1:1:15:3 | ModuleA | ModuleA |
| constants.rb:2:5:4:7 | ClassA | ModuleA::ClassA |
| constants.rb:3:9:3:15 | CONST_A | ModuleA::ClassA::CONST_A |
| constants.rb:6:5:6:11 | CONST_B | ModuleA::CONST_B |
| constants.rb:8:5:14:7 | ModuleB | ModuleA::ModuleB |
| constants.rb:9:9:10:11 | ClassB | ModuleA::ModuleB::ClassB |
| constants.rb:12:9:13:11 | ClassC | ModuleA::ModuleB::ClassC |
| constants.rb:17:1:17:8 | GREETING | GREETING |
| constants.rb:20:5:20:9 | Names | Names |
| constants.rb:31:1:32:3 | ClassD | ClassD |
| constants.rb:34:1:35:3 | ModuleC | ModuleC |
| constants.rb:37:1:37:26 | MAX_SIZE | MAX_SIZE |

View File

@@ -16,3 +16,7 @@ query Expr getConst(Module m, string name) { result = M::ExposedForTestingOnly::
query Expr lookupConst(Module m, string name) { result = M::lookupConst(m, name) }
query predicate constantValue(ConstantReadAccess a, Expr e) { e = a.getValue() }
query predicate constantWriteAccessQualifiedName(ConstantWriteAccess w, string qualifiedName) {
w.getQualifiedName() = qualifiedName
}

View File

@@ -55,5 +55,6 @@ actionControllerHelperMethods
getAssociatedControllerClasses
| app/controllers/foo/bars_controller.rb:1:1:20:3 | BarsController | app/views/foo/bars/_widget.html.erb:0:0:0:0 | app/views/foo/bars/_widget.html.erb |
| app/controllers/foo/bars_controller.rb:1:1:20:3 | BarsController | app/views/foo/bars/show.html.erb:0:0:0:0 | app/views/foo/bars/show.html.erb |
controllerTemplatesFolders
| app/controllers/foo/bars_controller.rb:1:1:20:3 | BarsController | folder://app/views/foo/bars | app/views/foo/bars |
controllerTemplateFiles
| app/controllers/foo/bars_controller.rb:1:1:20:3 | BarsController | app/views/foo/bars/_widget.html.erb:0:0:0:0 | app/views/foo/bars/_widget.html.erb |
| app/controllers/foo/bars_controller.rb:1:1:20:3 | BarsController | app/views/foo/bars/show.html.erb:0:0:0:0 | app/views/foo/bars/show.html.erb |

View File

@@ -18,6 +18,6 @@ query predicate getAssociatedControllerClasses(ActionControllerControllerClass c
cls = getAssociatedControllerClass(f)
}
query predicate controllerTemplatesFolders(ActionControllerControllerClass cls, Folder f) {
controllerTemplatesFolder(cls, f)
query predicate controllerTemplateFiles(ActionControllerControllerClass cls, ErbFile templateFile) {
controllerTemplateFile(cls, templateFile)
}

View File

@@ -44,3 +44,10 @@ potentiallyUnsafeSqlExecutingMethodCall
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
activeRecordModelInstantiations
| ActiveRecordInjection.rb:10:5:10:68 | self | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by | ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
| ActiveRecordInjection.rb:23:5:23:25 | self | ActiveRecordInjection.rb:19:1:25:3 | Admin |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:88:5:88:34 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |

View File

@@ -10,3 +10,9 @@ query predicate activeRecordModelClassMethodCalls(ActiveRecordModelClassMethodCa
query predicate potentiallyUnsafeSqlExecutingMethodCall(PotentiallyUnsafeSqlExecutingMethodCall call) {
any()
}
query predicate activeRecordModelInstantiations(
ActiveRecordModelInstantiation i, ActiveRecordModelClass cls
) {
i.getClass() = cls
}

View File

@@ -0,0 +1,38 @@
edges
| app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/controllers/foo/stores_controller.rb:9:22:9:23 | dt : |
| app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/controllers/foo/stores_controller.rb:13:55:13:56 | dt : |
| app/controllers/foo/stores_controller.rb:9:22:9:23 | dt : | app/views/foo/stores/show.html.erb:38:3:38:16 | @instance_text |
| app/controllers/foo/stores_controller.rb:13:55:13:56 | dt : | app/views/foo/stores/show.html.erb:2:9:2:20 | call to display_text |
| app/controllers/foo/stores_controller.rb:13:55:13:56 | dt : | app/views/foo/stores/show.html.erb:5:9:5:36 | ...[...] |
| app/controllers/foo/stores_controller.rb:13:55:13:56 | dt : | app/views/foo/stores/show.html.erb:9:9:9:26 | ...[...] |
| app/controllers/foo/stores_controller.rb:13:55:13:56 | dt : | app/views/foo/stores/show.html.erb:33:3:33:14 | call to display_text |
| app/controllers/foo/stores_controller.rb:13:55:13:56 | dt : | app/views/foo/stores/show.html.erb:41:76:41:87 | call to display_text : |
| app/views/foo/stores/show.html.erb:41:64:41:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
| app/views/foo/stores/show.html.erb:41:64:41:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
| app/views/foo/stores/show.html.erb:41:76:41:87 | call to display_text : | app/views/foo/stores/show.html.erb:41:64:41:87 | ... + ... : |
nodes
| app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | semmle.label | call to read : |
| app/controllers/foo/stores_controller.rb:9:22:9:23 | dt : | semmle.label | dt : |
| app/controllers/foo/stores_controller.rb:13:55:13:56 | dt : | semmle.label | dt : |
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
| app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
| app/views/foo/stores/show.html.erb:2:9:2:20 | call to display_text | semmle.label | call to display_text |
| app/views/foo/stores/show.html.erb:5:9:5:36 | ...[...] | semmle.label | ...[...] |
| app/views/foo/stores/show.html.erb:9:9:9:26 | ...[...] | semmle.label | ...[...] |
| app/views/foo/stores/show.html.erb:33:3:33:14 | call to display_text | semmle.label | call to display_text |
| app/views/foo/stores/show.html.erb:38:3:38:16 | @instance_text | semmle.label | @instance_text |
| app/views/foo/stores/show.html.erb:41:64:41:87 | ... + ... : | semmle.label | ... + ... : |
| app/views/foo/stores/show.html.erb:41:76:41:87 | call to display_text : | semmle.label | call to display_text : |
| app/views/foo/stores/show.html.erb:47:5:47:16 | call to handle | semmle.label | call to handle |
| app/views/foo/stores/show.html.erb:64:3:64:18 | call to handle | semmle.label | call to handle |
subpaths
#select
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | Cross-site scripting vulnerability due to $@ | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read | stored value |
| app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to $@ | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read | stored value |
| app/views/foo/stores/show.html.erb:2:9:2:20 | call to display_text | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/views/foo/stores/show.html.erb:2:9:2:20 | call to display_text | Cross-site scripting vulnerability due to $@ | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read | stored value |
| app/views/foo/stores/show.html.erb:5:9:5:36 | ...[...] | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/views/foo/stores/show.html.erb:5:9:5:36 | ...[...] | Cross-site scripting vulnerability due to $@ | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read | stored value |
| app/views/foo/stores/show.html.erb:9:9:9:26 | ...[...] | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/views/foo/stores/show.html.erb:9:9:9:26 | ...[...] | Cross-site scripting vulnerability due to $@ | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read | stored value |
| app/views/foo/stores/show.html.erb:33:3:33:14 | call to display_text | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/views/foo/stores/show.html.erb:33:3:33:14 | call to display_text | Cross-site scripting vulnerability due to $@ | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read | stored value |
| app/views/foo/stores/show.html.erb:38:3:38:16 | @instance_text | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read : | app/views/foo/stores/show.html.erb:38:3:38:16 | @instance_text | Cross-site scripting vulnerability due to $@ | app/controllers/foo/stores_controller.rb:8:10:8:29 | call to read | stored value |
| app/views/foo/stores/show.html.erb:47:5:47:16 | call to handle | app/views/foo/stores/show.html.erb:47:5:47:16 | call to handle | app/views/foo/stores/show.html.erb:47:5:47:16 | call to handle | Cross-site scripting vulnerability due to $@ | app/views/foo/stores/show.html.erb:47:5:47:16 | call to handle | stored value |
| app/views/foo/stores/show.html.erb:64:3:64:18 | call to handle | app/views/foo/stores/show.html.erb:64:3:64:18 | call to handle | app/views/foo/stores/show.html.erb:64:3:64:18 | call to handle | Cross-site scripting vulnerability due to $@ | app/views/foo/stores/show.html.erb:64:3:64:18 | call to handle | stored value |

View File

@@ -0,0 +1 @@
queries/security/cwe-079/StoredXSS.ql

View File

@@ -0,0 +1,15 @@
class StoresController < ApplicationController
helper_method :user_handle
def user_handle
User.find(1).handle
end
def show
dt = File.read("foo.txt")
@instance_text = dt
@user = User.find 1
@safe_user_handle = ERB::Util.html_escape(@user.handle)
@other_user_raw_name = User.find(2).raw_name
render "foo/stores/show", locals: { display_text: dt, safe_text: "hello" }
end
end

View File

@@ -0,0 +1,14 @@
class User < ActiveRecord::Base
def is_dummy_user?
self.user_id == 0
end
def raw_name
me = self
me.handle
end
def display_name
self.real_name || self.handle
end
end

View File

@@ -0,0 +1,84 @@
<%# BAD: A local rendered raw as a local variable %>
<%= raw display_text %>
<%# BAD: A local rendered raw via the local_assigns hash %>
<%= raw local_assigns[:display_text] %>
<% key = :display_text %>
<%# BAD: A local rendered raw via the locals_assigns hash %>
<%= raw local_assigns[key] %>
<ul>
<% for key in [:display_text, :safe_text] do %>
<%# BAD: A local rendered raw via the locals hash %>
<%# TODO: we miss that `key` can take `:display_text` as a value here %>
<li><%= raw local_assigns[key] %></li>
<% end %>
</ul>
<%# GOOD: A local rendered with default escaping via the local_assigns hash %>
<%= local_assigns[display_text] %>
<%# GOOD: default escaping of rendered text %>
<%=
full_text = prefix + local_assigns[:display_text]
full_text
%>
<%# GOOD: default escaping of rendered text (from instance var) %>
<%= @instance_text %>
<%# BAD: html_safe marks string as not requiring HTML escaping %>
<%=
display_text.html_safe
%>
<%# BAD: html_safe marks string as not requiring HTML escaping %>
<%=
@instance_text.html_safe
%>
<%= render partial: 'foo/bars/widget', locals: { display_text: "widget_" + display_text } %>
<%# BAD: user_name_handle is a helper method that returns unsanitized database content %>
<%= user_name_handle.html_safe %>
<%# BAD: Direct to a database value without escaping %>
<%= @user.handle.html_safe %>
<%# BAD: Indirect to a database value without escaping %>
<%= @user.raw_name.html_safe %>
<%# GOOD: Direct to a database value with escaping %>
<%= @user.handle %>
<%# GOOD: @safe_user_handle is manually escaped in the controller %>
<%= @safe_user_handle %>
<%# GOOD: object_id is a built-in method, not an ORM access method %>
<%= @user.object_id.html_safe %>
<%# BAD: Direct to a database value without escaping %>
<%=
some_user = User.find 1
some_user.handle.html_safe
%>
<%# BAD: Indirect to a database value without escaping %>
<%=
some_user = User.find 1
some_user.raw_name.html_safe
%>
<%# GOOD: Direct to a database value with escaping %>
<%=
some_user = User.find 1
some_user.handle
%>
<%# BAD: Indirect to a database value without escaping %>
<%# TODO: we do not detect that `display_name` can return a DB field %>
<%= @user.display_name.html_safe %>
<%# BAD: Indirect to a database value without escaping %>
<%= @other_user_raw_name.html_safe %>

View File

@@ -11,6 +11,10 @@
"codeql/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImpl.qll",
"ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl.qll"
],
"DataFlow2": [
"codeql/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImpl2.qll",
"ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl2.qll"
],
"DataFlow Consistency": [
"codeql/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImplConsistency.qll",
"ql/lib/codeql/ruby/dataflow/internal/DataFlowImplConsistency.qll"