diff --git a/.github/workflows/compile-queries.yml b/.github/workflows/compile-queries.yml index c44aa56a7530..c9bd7547b834 100644 --- a/.github/workflows/compile-queries.yml +++ b/.github/workflows/compile-queries.yml @@ -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 diff --git a/.github/workflows/csharp-qltest.yml b/.github/workflows/csharp-qltest.yml index 2b8ecad83d9b..ae196c3b91e8 100644 --- a/.github/workflows/csharp-qltest.yml +++ b/.github/workflows/csharp-qltest.yml @@ -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: diff --git a/.github/workflows/go-tests.yml b/.github/workflows/go-tests.yml index 9d518ac70b65..6e63ab46f524 100644 --- a/.github/workflows/go-tests.yml +++ b/.github/workflows/go-tests.yml @@ -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 diff --git a/.github/workflows/ql-for-ql-build.yml b/.github/workflows/ql-for-ql-build.yml index e8ac1fa0f173..b03961539021 100644 --- a/.github/workflows/ql-for-ql-build.yml +++ b/.github/workflows/ql-for-ql-build.yml @@ -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 diff --git a/.github/workflows/ruby-build.yml b/.github/workflows/ruby-build.yml index 392c6ff83026..624e74cd3dd5 100644 --- a/.github/workflows/ruby-build.yml +++ b/.github/workflows/ruby-build.yml @@ -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 diff --git a/.github/workflows/ruby-qltest.yml b/.github/workflows/ruby-qltest.yml index 19d5325091fd..2345fbd8a377 100644 --- a/.github/workflows/ruby-qltest.yml +++ b/.github/workflows/ruby-qltest.yml @@ -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: diff --git a/.github/workflows/swift.yml b/.github/workflows/swift.yml index ff9cd29e238d..615421ba825c 100644 --- a/.github/workflows/swift.yml +++ b/.github/workflows/swift.yml @@ -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: @@ -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 @@ -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 diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll index 7ac82dae1b32..603c1a61ce35 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll @@ -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 + } + } } /** @@ -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) } /** @@ -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() } @@ -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. */ diff --git a/ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.qhelp b/ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.qhelp new file mode 100644 index 000000000000..9b8944b1d65c --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.qhelp @@ -0,0 +1,65 @@ + + + + +

+ 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. +

+ +

+ 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. +

+ +

+ 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. +

+ + + +

+ In the Rails web framework, CSRF protection is enabled by the adding a call to + the protect_from_forgery method inside an + ActionController class. Typically this is done in the + ApplicationController 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 + protect_from_forgery with: :exception can help to avoid this + by raising an exception on an invalid CSRF token instead. +

+ + + +

+ The following example shows a case where CSRF protection is enabled with + a secure request handling strategy of :exception. +

+ + + + + + +
  • Wikipedia: Cross-site request forgery
    • +
  • OWASP: Cross-site request forgery
    • +
  • Securing Rails Applications: Cross-Site Request Forgery (CSRF)
    • +
  • Veracode: When Rails' protect_from_forgery Fails.
    • +     + +     diff --git a/ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.ql b/ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.ql new file mode 100644 index 000000000000..4210d798863a --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.ql @@ -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" diff --git a/ruby/ql/src/queries/security/cwe-352/examples/ProtectFromForgeryGood.rb b/ruby/ql/src/queries/security/cwe-352/examples/ProtectFromForgeryGood.rb new file mode 100644 index 000000000000..ecab2c8ea45a --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-352/examples/ProtectFromForgeryGood.rb @@ -0,0 +1,4 @@ +class ApplicationController < ActionController::Base + protect_from_forgery with: :exception +end + \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.expected b/ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.expected new file mode 100644 index 000000000000..7fe8b3490a95 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.expected @@ -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 | diff --git a/ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.qlref b/ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.qlref new file mode 100644 index 000000000000..8e9e894fe518 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.qlref @@ -0,0 +1 @@ +queries/security/cwe-352/CSRFProtectionNotEnabled.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-352/railsapp/app/controllers/alternative_root_controller.rb b/ruby/ql/test/query-tests/security/cwe-352/railsapp/app/controllers/alternative_root_controller.rb new file mode 100644 index 000000000000..8cbf31529c15 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-352/railsapp/app/controllers/alternative_root_controller.rb @@ -0,0 +1,3 @@ +class AlternativeRootController < ActionController::Base + # BAD: no protect_from_forgery call +end \ No newline at end of file