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
15 changes: 14 additions & 1 deletion .github/workflows/compile-queries.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,21 @@ on:
pull_request:

jobs:
choose-runner:
outputs:
runs-on:
runs-on: ubuntu-latest
steps:
- if: vars.use-large-runners-for-speed
run: |
echo "runner=ubuntu-latest-xl" >> $GITHUB_OUTPUT
- if: ${{ !vars.use-large-runners-for-speed }}
run: |
echo "runner=ubuntu-latest" >> $GITHUB_OUTPUT

compile-queries:
runs-on: ubuntu-latest-xl
needs: choose-runner
runs-on: choose-runner.outputs.runner

steps:
- uses: actions/checkout@v4
Expand Down
16 changes: 15 additions & 1 deletion .github/workflows/csharp-qltest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,22 @@ jobs:
--dbscheme=ql/lib/semmlecode.csharp.dbscheme --target-dbscheme=downgrades/initial/semmlecode.csharp.dbscheme |
xargs codeql execute upgrades testdb
diff -q testdb/semmlecode.csharp.dbscheme downgrades/initial/semmlecode.csharp.dbscheme

choose-runner:
outputs:
runs-on:
runs-on: ubuntu-latest
steps:
- if: vars.use-large-runners-for-speed
run: |
echo "runner=ubuntu-latest-xl" >> $GITHUB_OUTPUT
- if: ${{ !vars.use-large-runners-for-speed }}
run: |
echo "runner=ubuntu-latest" >> $GITHUB_OUTPUT

qltest:
runs-on: ubuntu-latest-xl
needs: choose-runner
runs-on: choose-runner.outputs.runner
strategy:
fail-fast: false
matrix:
Expand Down
15 changes: 14 additions & 1 deletion .github/workflows/go-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,22 @@ on:
env:
GO_VERSION: '~1.21.0'
jobs:
choose-runner:
outputs:
runs-on:
runs-on: ubuntu-latest
steps:
- if: vars.use-large-runners-for-speed
run: |
echo "runner=ubuntu-latest-xl" >> $GITHUB_OUTPUT
- if: ${{ !vars.use-large-runners-for-speed }}
run: |
echo "runner=ubuntu-latest" >> $GITHUB_OUTPUT

test-linux:
name: Test Linux (Ubuntu)
runs-on: ubuntu-latest-xl
needs: choose-runner
runs-on: choose-runner.outputs.runner
steps:
- name: Set up Go ${{ env.GO_VERSION }}
uses: actions/setup-go@v5
Expand Down
15 changes: 14 additions & 1 deletion .github/workflows/ql-for-ql-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,21 @@ env:
CARGO_TERM_COLOR: always

jobs:
choose-runner:
outputs:
runs-on:
runs-on: ubuntu-latest
steps:
- if: vars.use-large-runners-for-speed
run: |
echo "runner=ubuntu-latest-xl" >> $GITHUB_OUTPUT
- if: ${{ !vars.use-large-runners-for-speed }}
run: |
echo "runner=ubuntu-latest" >> $GITHUB_OUTPUT

analyze:
runs-on: ubuntu-latest-xl
needs: choose-runner
runs-on: choose-runner.outputs.runner
steps:
### Build the queries ###
- uses: actions/checkout@v4
Expand Down
15 changes: 14 additions & 1 deletion .github/workflows/ruby-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,21 @@ jobs:
ruby/extractor/target/release/codeql-extractor-ruby
ruby/extractor/target/release/codeql-extractor-ruby.exe
retention-days: 1
choose-runner:
outputs:
runs-on:
runs-on: ubuntu-latest
steps:
- if: vars.use-large-runners-for-speed
run: |
echo "runner=ubuntu-latest-xl" >> $GITHUB_OUTPUT
- if: ${{ !vars.use-large-runners-for-speed }}
run: |
echo "runner=ubuntu-latest" >> $GITHUB_OUTPUT

compile-queries:
runs-on: ubuntu-latest-xl
needs: choose-runner
runs-on: choose-runner.outputs.runner
steps:
- uses: actions/checkout@v4
- name: Fetch CodeQL
Expand Down
16 changes: 15 additions & 1 deletion .github/workflows/ruby-qltest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,22 @@ jobs:
--dbscheme=ql/lib/ruby.dbscheme --target-dbscheme=downgrades/initial/ruby.dbscheme |
xargs codeql execute upgrades testdb
diff -q testdb/ruby.dbscheme downgrades/initial/ruby.dbscheme

choose-runner:
outputs:
runs-on:
runs-on: ubuntu-latest
steps:
- if: vars.use-large-runners-for-speed
run: |
echo "runner=ubuntu-latest-xl" >> $GITHUB_OUTPUT
- if: ${{ !vars.use-large-runners-for-speed }}
run: |
echo "runner=ubuntu-latest" >> $GITHUB_OUTPUT

qltest:
runs-on: ubuntu-latest-xl
needs: choose-runner
runs-on: choose-runner.outputs.runner
strategy:
fail-fast: false
steps:
Expand Down
27 changes: 22 additions & 5 deletions .github/workflows/swift.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ on:
- codeql-cli-*

jobs:
choose-runner:
outputs:
runs-on:
runs-on: ubuntu-latest
steps:
- if: vars.use-large-runners-for-speed
run: |
echo "runner=ubuntu-latest-xl" >> $GITHUB_OUTPUT
- if: ${{ !vars.use-large-runners-for-speed }}
run: |
echo "runner=ubuntu-latest" >> $GITHUB_OUTPUT

# not using a matrix as you cannot depend on a specific job in a matrix, and we want to start linux checks
# without waiting for the macOS build
build-and-test-macos:
Expand All @@ -42,13 +54,16 @@ jobs:
- uses: actions/checkout@v4
- uses: ./swift/actions/build-and-test
build-and-test-linux:
runs-on: ubuntu-latest-xl
needs: choose-runner
runs-on: choose-runner.outputs.runner
steps:
- uses: actions/checkout@v4
- uses: ./swift/actions/build-and-test
qltests-linux:
needs: build-and-test-linux
runs-on: ubuntu-latest-xl
needs:
- build-and-test-linux
- choose-runner
runs-on: choose-runner.outputs.runner
steps:
- uses: actions/checkout@v4
- uses: ./swift/actions/run-ql-tests
Expand All @@ -60,8 +75,10 @@ jobs:
- uses: actions/checkout@v4
- uses: ./swift/actions/run-ql-tests
integration-tests-linux:
needs: build-and-test-linux
runs-on: ubuntu-latest-xl
needs:
- build-and-test-linux
- choose-runner
runs-on: choose-runner.outputs.runner
steps:
- uses: actions/checkout@v4
- uses: ./swift/actions/run-integration-tests
Expand Down
80 changes: 47 additions & 33 deletions ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,35 @@ private import codeql.ruby.dataflow.internal.DataFlowDispatch
module ActionController {
// TODO: move the rest of this file inside this module.
import codeql.ruby.frameworks.actioncontroller.Filters

/**
* An ActionController class which sits at the top of the class hierarchy.
* In other words, it does not subclass any other class in source code.
*/
class RootController extends ActionControllerClass {
RootController() {
not exists(ActionControllerClass parent | this != parent and this = parent.getADescendent())
}
}

/**
* A call to `protect_from_forgery`.
*/
class ProtectFromForgeryCall extends CsrfProtectionSetting::Range, DataFlow::CallNode {
ProtectFromForgeryCall() {
this = actionControllerInstance().getAMethodCall("protect_from_forgery")
}

private string getWithValueText() {
result = this.getKeywordArgument("with").getConstantValue().getSymbol()
}

// Calls without `with: :exception` can allow for bypassing CSRF protection
// in some scenarios.
override boolean getVerificationSetting() {
if this.getWithValueText() = "exception" then result = true else result = false
}
}
}

/**
Expand All @@ -38,18 +67,10 @@ module ActionController {
*/
class ActionControllerClass extends DataFlow::ClassNode {
ActionControllerClass() {
this =
[
DataFlow::getConstant("ActionController").getConstant("Base"),
// In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
// treat it separately in case the `ApplicationController` definition is not in the database.
DataFlow::getConstant("ApplicationController"),
// ActionController::Metal technically doesn't contain all of the
// methods available in Base, such as those for rendering views.
// However we prefer to be over-sensitive in this case in order to find
// more results.
DataFlow::getConstant("ActionController").getConstant("Metal")
].getADescendentModule()
this = DataFlow::getConstant("ApplicationController").getADescendentModule()
or
this = actionControllerBaseClass().getADescendentModule() and
not exists(DataFlow::ModuleNode m | m = actionControllerBaseClass().asModule() | this = m)
}

/**
Expand All @@ -73,6 +94,20 @@ class ActionControllerClass extends DataFlow::ClassNode {
}
}

private DataFlow::ConstRef actionControllerBaseClass() {
result =
[
// In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
// treat it separately in case the `ApplicationController` definition is not in the database.
DataFlow::getConstant("ActionController").getConstant("Base"),
// ActionController::Metal technically doesn't contain all of the
// methods available in Base, such as those for rendering views.
// However we prefer to be over-sensitive in this case in order to find
// more results.
DataFlow::getConstant("ActionController").getConstant("Metal")
]
}

private API::Node actionControllerInstance() {
result = any(ActionControllerClass cls).getSelf().track()
}
Expand Down Expand Up @@ -405,27 +440,6 @@ class ActionControllerSkipForgeryProtectionCall extends CsrfProtectionSetting::R
override boolean getVerificationSetting() { result = false }
}

/**
* A call to `protect_from_forgery`.
*/
private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetting::Range,
DataFlow::CallNode
{
ActionControllerProtectFromForgeryCall() {
this = actionControllerInstance().getAMethodCall("protect_from_forgery")
}

private string getWithValueText() {
result = this.getKeywordArgument("with").getConstantValue().getSymbol()
}

// Calls without `with: :exception` can allow for bypassing CSRF protection
// in some scenarios.
override boolean getVerificationSetting() {
if this.getWithValueText() = "exception" then result = true else result = false
}
}

/**
* A call to `send_file`, which sends the file at the given path to the client.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>

<overview>
<p>
Cross-site request forgery (CSRF) is a type of vulnerability in which an
attacker is able to force a user to carry out an action that the user did
not intend.
</p>

<p>
The attacker tricks an authenticated user into submitting a request to the
web application. Typically this request will result in a state change on
the server, such as changing the user's password. The request can be
initiated when the user visits a site controlled by the attacker. If the
web application relies only on cookies for authentication, or on other
credentials that are automatically included in the request, then this
request will appear as legitimate to the server.
</p>

<p>
A common countermeasure for CSRF is to generate a unique token to be
included in the HTML sent from the server to a user. This token can be
used as a hidden field to be sent back with requests to the server, where
the server can then check that the token is valid and associated with the
relevant user session.
</p>
</overview>

<recommendation>
<p>
In the Rails web framework, CSRF protection is enabled by the adding a call to
the <code>protect_from_forgery</code> method inside an
<code>ActionController</code> class. Typically this is done in the
<code>ApplicationController</code> class, or an equivalent class from which
other controller classes are subclassed.

The default behaviour of this method is to null the session when an invalid
CSRF token is provided. This may not be sufficient to avoid a CSRF
vulnerability - for example if parts of the session are memoized. Calling
<code>protect_from_forgery with: :exception</code> can help to avoid this
by raising an exception on an invalid CSRF token instead.
</p>
</recommendation>

<example>
<p>
The following example shows a case where CSRF protection is enabled with
a secure request handling strategy of <code>:exception</code>.
</p>

<sample src="examples/ProtectFromForgeryGood.rb"/>

</example>

<references>
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Cross-site_request_forgery">Cross-site request forgery</a></li>
<li>OWASP: <a href="https://owasp.org/www-community/attacks/csrf">Cross-site request forgery</a></li>
<li>Securing Rails Applications: <a href="https://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf">Cross-Site Request Forgery (CSRF)</a></li>
<li>Veracode: <a href="https://www.veracode.com/blog/managing-appsec/when-rails-protectfromforgery-fails">When Rails' protect_from_forgery Fails</a>.</li>
</references>

</qhelp>
23 changes: 23 additions & 0 deletions ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* @name CSRF protection not enabled
* @description Not enabling CSRF protection may make the application
* vulnerable to a Cross-Site Request Forgery (CSRF) attack.
* @kind problem
* @problem.severity warning
* @security-severity 8.8
* @precision high
* @id rb/csrf-protection-not-enabled
* @tags security
* external/cwe/cwe-352
*/

import codeql.ruby.AST
import codeql.ruby.Concepts
import codeql.ruby.frameworks.ActionController

from ActionController::RootController c
where
not exists(ActionController::ProtectFromForgeryCall call |
c.getSelf().flowsTo(call.getReceiver())
)
select c, "Potential CSRF vulnerability due to forgery protection not being enabled"
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class ApplicationController < ActionController::Base
protect_from_forgery with: :exception
end

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
| railsapp/app/controllers/alternative_root_controller.rb:1:1:3:3 | AlternativeRootController | Potential CSRF vulnerability due to forgery protection not being enabled |
Loading