diff --git a/ruby/src/security/CWE-770/UserControlledMaxIterations.qhelp b/ruby/src/security/CWE-770/UserControlledMaxIterations.qhelp new file mode 100644 index 00000000..2757cfbc --- /dev/null +++ b/ruby/src/security/CWE-770/UserControlledMaxIterations.qhelp @@ -0,0 +1,39 @@ + + + + +

When a remote user-controlled data value can be used as part of the limit of times an operation can be executed, such behavior could lead to a denial of service.

+ + + + +

Ensure the limitation and the validation of any incoming value to a reasonable value.

+ + + + +

+In this example a user-controlled data value such as `1_000` reaches a repeatable operation as `1_000` times. A simple exploit would be for an attacker to send a huge value as `999_999_999` or provoke an endless loop with a negative value. +

+ + + +

To fix this vulnerability, it is required to constrain the size of the user input and validate the incoming value.

+ +

For illustration purposes, we can limit the possible values for the user input to between `1` and `1_000`.

+ + + + + + +
  • + CVE-2022-23837: High severity denial of service vulnerability in Sidekiq, there is no limit on the number of days when requesting stats for the graph. This overloads the system, affecting the Web UI, and makes it unavailable to users. +
    • + +
  • The suggested fix for the Sidekiq denial of service vulnerability.
    • + +     +     diff --git a/ruby/src/security/CWE-770/UserControlledMaxIterations.ql b/ruby/src/security/CWE-770/UserControlledMaxIterations.ql new file mode 100644 index 00000000..4a4ebae1 --- /dev/null +++ b/ruby/src/security/CWE-770/UserControlledMaxIterations.ql @@ -0,0 +1,150 @@ +/** + * @name Denial of Service using unconstrained integer/float value + * @description A remote user-controlled integer/float value can reach a condition that controls how many times a repeatable operation can be executed. A malicious user may misuse that value to cause an application-level denial of service. + * @kind path-problem + * @id githubsecuritylab/user-controlled-max-iterations + * @precision high + * @problem.severity error + * @tags security + * experimental + * external/cwe/cwe-770 + */ + +import ruby +import codeql.ruby.ApiGraphs +import codeql.ruby.Concepts +import codeql.ruby.TaintTracking +import codeql.ruby.dataflow.RemoteFlowSources +import codeql.ruby.dataflow.BarrierGuards +import codeql.ruby.AST +import codeql.ruby.controlflow.CfgNodes as CfgNodes +import codeql.ruby.CFG +import codeql.ruby.dataflow.internal.DataFlowPublic +import codeql.ruby.InclusionTests + +/** + * Ensure that the user-provided value is limited to a certain amount + * + * @param node The node to check if limited by: + * 1. A comparison operation node with a less than or less than or equal operator + * 2. A comparison operation node with a greater than or greater than or equal operator + * 3. A comparison operation node with a greater than or greater than or equal operator and the branch is false + * 4. A comparison operation node with a less than or less than or equal operator and the branch is false + */ +predicate underAValue(CfgNodes::AstCfgNode g, CfgNode node, boolean branch) { + exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode cn | cn = g | + exists(string op | + ( + // arg <= LIMIT OR arg < LIMIT + (op = "<" or op = "<=") and + branch = true and + op = cn.getOperator() and + node = cn.getLeftOperand() + or + // LIMIT >= arg OR LIMIT > arg + (op = ">" or op = ">=") and + branch = true and + op = cn.getOperator() and + node = cn.getRightOperand() + or + // not arg >= LIMIT OR not arg > LIMIT + (op = ">" or op = ">=") and + branch = false and + op = cn.getOperator() and + node = cn.getLeftOperand() + or + // not LIMIT <= arg OR not LIMIT < argo + (op = "<" or op = "<=") and + branch = false and + op = cn.getOperator() and + node = cn.getRightOperand() + ) + ) + ) +} + +/** + * Sidekiq ensure using the `params` function that all keys in the resulting hash are strings and ingest `request.params`. So a call to `params` function is considered as a remote flow source. + * + * https://github.com/sidekiq/sidekiq/blob/79d254d9045bb5805beed6aaffec1886ef89f71b/lib/sidekiq/web/action.rb#L30-L37 + */ +class ParamsRFS extends RemoteFlowSource::Range { + ParamsRFS() { + exists(ElementReference er, MethodCall mc | + er.getReceiver() = mc and + mc.getMethodName() = "params" and + this.asExpr() = er.getAControlFlowNode() + ) + } + + override string getSourceType() { result = "Request params data" } +} + +private module DoSConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + //source instanceof ParamsRFS or + source instanceof RemoteFlowSource + } + + predicate isBarrier(DataFlow::Node sanitizer) { + // Sanitize the user-provided value if limited (underAValue check) + sanitizer = DataFlow::BarrierGuard::getABarrierNode() + } + + /** + * Support additional flow step for a case like using a default value `(params["days"] | 100).to_i` + */ + predicate isAdditionalFlowStep(DataFlow::Node previousNode, DataFlow::Node nextNode) { + // Additional flow step like `(RFS | INTEGER).to_i` or `(FLOAT | RFS).to_f` + exists(ParenthesizedExpr loe, DataFlow::CallNode c, ExprNode en | + en = c.getReceiver() and + c.getMethodName() = ["to_i", "to_f"] and + c = nextNode and + loe = en.asExpr().getExpr() and + loe.getStmt(_).(LogicalOrExpr).getAnOperand() = previousNode.asExpr().getExpr() and + not previousNode.asExpr().getExpr() instanceof IntegerLiteral + ) + or + // Additional flow step like `RFS.to_i` or `RFS.to_f` + exists(MethodCall mc | + mc.getReceiver() = previousNode.asExpr().getExpr() and + mc.getMethodName() = ["to_i", "to_f"] and + mc = nextNode.asExpr().getExpr() + ) + } + + /** + * - Check if the user-provided value is used in a repeatable operation using `downto`, `upto` or `times` + * - Check if the user-provided value is used in a repeatable operation using `for` loop or conditional loop like `until` or `while`. + */ + predicate isSink(DataFlow::Node sink) { + // sink = n in `100.downto(n)` or `1.upto(n)` + exists(MethodCall mc | + sink.asExpr().getExpr() = mc.getArgument(0) and + mc.getMethodName() = ["downto", "upto"] + ) + or + // sink = n in `n.times()` or `n.downto(1)` or `n.upto(100)` + exists(MethodCall mc | + sink.asExpr().getExpr() = mc.getReceiver() and + mc.getMethodName() = ["times", "downto", "upto"] + ) + or + // sink = 1..n in `for i in 0..n` + exists(ForExpr forLoop | sink.asExpr().getExpr() = forLoop.getValue()) + or + // sink = n in `until n` + exists(ConditionalLoop conditionalLoop | + sink.asExpr().getExpr() = conditionalLoop.getCondition() + ) + } +} + +module DoSFlow = TaintTracking::Global; + +import DoSFlow::PathGraph + +from DoSFlow::PathNode source, DoSFlow::PathNode sink +where DoSFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "This $@ can control $@ a repeatable operation is executed.", + source.getNode(), "user-provided value", sink.getNode(), "how many times" diff --git a/ruby/test/security/CWE-770/UserControlledMaxIterations.expected b/ruby/test/security/CWE-770/UserControlledMaxIterations.expected new file mode 100644 index 00000000..9567b8db --- /dev/null +++ b/ruby/test/security/CWE-770/UserControlledMaxIterations.expected @@ -0,0 +1,19 @@ +edges +| UserControlledMaxIterations__bad.rb:3:7:3:11 | limit | UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | provenance | | +| UserControlledMaxIterations__bad.rb:3:7:3:11 | limit | UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | provenance | | +| UserControlledMaxIterations__bad.rb:3:15:3:20 | call to params | UserControlledMaxIterations__bad.rb:3:15:3:28 | ...[...] | provenance | | +| UserControlledMaxIterations__bad.rb:3:15:3:28 | ...[...] | UserControlledMaxIterations__bad.rb:3:15:3:33 | call to to_i | provenance | | +| UserControlledMaxIterations__bad.rb:3:15:3:33 | call to to_i | UserControlledMaxIterations__bad.rb:3:7:3:11 | limit | provenance | | +nodes +| UserControlledMaxIterations__bad.rb:3:7:3:11 | limit | semmle.label | limit | +| UserControlledMaxIterations__bad.rb:3:15:3:20 | call to params | semmle.label | call to params | +| UserControlledMaxIterations__bad.rb:3:15:3:28 | ...[...] | semmle.label | ...[...] | +| UserControlledMaxIterations__bad.rb:3:15:3:33 | call to to_i | semmle.label | call to to_i | +| UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | semmle.label | limit | +| UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | semmle.label | limit | +subpaths +#select +| UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | UserControlledMaxIterations__bad.rb:3:15:3:20 | call to params | UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | This $@ can control $@ a repeatable operation is executed. | UserControlledMaxIterations__bad.rb:3:15:3:20 | call to params | user-provided value | UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | how many times | +| UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | UserControlledMaxIterations__bad.rb:3:15:3:28 | ...[...] | UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | This $@ can control $@ a repeatable operation is executed. | UserControlledMaxIterations__bad.rb:3:15:3:28 | ...[...] | user-provided value | UserControlledMaxIterations__bad.rb:11:7:11:11 | limit | how many times | +| UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | UserControlledMaxIterations__bad.rb:3:15:3:20 | call to params | UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | This $@ can control $@ a repeatable operation is executed. | UserControlledMaxIterations__bad.rb:3:15:3:20 | call to params | user-provided value | UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | how many times | +| UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | UserControlledMaxIterations__bad.rb:3:15:3:28 | ...[...] | UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | This $@ can control $@ a repeatable operation is executed. | UserControlledMaxIterations__bad.rb:3:15:3:28 | ...[...] | user-provided value | UserControlledMaxIterations__bad.rb:16:7:16:11 | limit | how many times | diff --git a/ruby/test/security/CWE-770/UserControlledMaxIterations.qlref b/ruby/test/security/CWE-770/UserControlledMaxIterations.qlref new file mode 100644 index 00000000..c10d17c5 --- /dev/null +++ b/ruby/test/security/CWE-770/UserControlledMaxIterations.qlref @@ -0,0 +1 @@ +security/CWE-770/UserControlledMaxIterations.ql \ No newline at end of file diff --git a/ruby/test/security/CWE-770/UserControlledMaxIterations__bad.rb b/ruby/test/security/CWE-770/UserControlledMaxIterations__bad.rb new file mode 100644 index 00000000..a1ca68fb --- /dev/null +++ b/ruby/test/security/CWE-770/UserControlledMaxIterations__bad.rb @@ -0,0 +1,21 @@ +class UserController < ActionController::Base + def bad_examples + limit = params[:limit].to_i + + # repeat a simple operation for the number of limit specified using upto() + 1.upto(days) do |i| + put "a repeatable operation" + end + + # repeat a simple operation for the number of limit specified using times() + limit.times do + put "a repeatable operation" + end + + # repeat a simple operation for the number of limit specified using downto() + limit.downto(1) do |i| + put "a repeatable operation" + end + + end + end diff --git a/ruby/test/security/CWE-770/UserControlledMaxIterations__good.rb b/ruby/test/security/CWE-770/UserControlledMaxIterations__good.rb new file mode 100644 index 00000000..455988ca --- /dev/null +++ b/ruby/test/security/CWE-770/UserControlledMaxIterations__good.rb @@ -0,0 +1,17 @@ +class UserController < ActionController::Base + def good_example + limit = params[:limit].to_i + + # limit the limit between 1 and 1000 + if not (limit => 1 && limit < 1000) + limit = 10 + end + + + # repeat a simple operation for the number of limit specified using upto() + 1.upto(days) do |i| + put "a repeatable operation" + end + + end + end \ No newline at end of file