Skip to content

Go: Add BigQuery as a sink for SQLi queries #2 #19561

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Go: Add BigQuery as a sink for SQLi queries #2 #19561

wants to merge 4 commits into from
+1,400 −1

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented May 22, 2025

Supersedes #18460.

@Copilot Copilot AI review requested due to automatic review settings May 22, 2025 14:23
@owen-mc owen-mc requested a review from a team as a code owner May 22, 2025 14:23
Copilot
Copilot AI reviewed May 22, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for treating cloud.google.com/go/bigquery.Client.Query as a SQL-injection sink in CodeQL analyses, along with test scaffolding, expected outputs, and a change note.

  • Introduce a Go test module (bigquery.go, go.mod) demonstrating an untrusted string passed to Client.Query.
  • Add CodeQL test query (bigquery.ql) and sink model (cloud.google.com.go.bigquery.model.yml) to mark Query as an SQLi sink.
  • Provide test expectations and change notes (*.expected, *.md).

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/go.mod New module with BigQuery dependency for tests
go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/bigquery.ql CodeQL test that matches calls to BigQuery Query
go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/bigquery.go Example Go code invoking Query with untrusted data
go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/bigquery.expected Expected test output for the BigQuery sink
go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/QueryString.ql Test modules for SQL::QueryString and dataflow validation
go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/QueryString.expected Expected results placeholder for QueryString tests
go/ql/lib/ext/cloud.google.com.go.bigquery.model.yml Extension model marking Client.Query as a SQLi sink
go/ql/lib/change-notes/2025-05-22-bigquery-client-query-sql-injection.md Documentation of the newly recognized sink
Comments suppressed due to low confidence (2)

go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/bigquery.ql:3

  • [nitpick] The variables a and b are not descriptive. Consider renaming them to packageName and functionName (or similar) to clarify their roles in hasQualifiedName.
from SQL::QueryString qs, Function func, string a, string b

go/ql/test/library-tests/semmle/go/frameworks/SQL/bigquery/QueryString.ql:24

  • This forces element to be empty, which prevents capturing the actual query string in the QueryString test. Update this to element = qs.toString() (or remove the filter) so the test can validate the real value.
    element = "" and

@owen-mc owen-mc mentioned this pull request May 22, 2025
Closed
7 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 22, 2025 •
edited
Loading

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

go

Generated file changes for go

  • Changes to framework-coverage-go.rst:
+    `bigquery <https://pkg.go.dev/cloud.google.com/go/bigquery>`_,``cloud.google.com/go/bigquery*``,,,1
-    `gorqlite <https://github.com/rqlite/gorqlite>`_,"``github.com/raindog308/gorqlite*``, ``github.com/rqlite/gorqlite*``",16,4,48
+    `gorqlite <https://github.com/rqlite/gorqlite>`_,"``github.com/raindog308/gorqlite*``, ``github.com/rqlite/gorqlite*``, ``github.com/kanikanema/gorqlite*``",24,6,72
-    Others,``github.com/kanikanema/gorqlite``,8,2,24
-    Totals,,688,1069,1556
+    Totals,,688,1069,1557
  • Changes to framework-coverage-go.csv:
+ cloud.google.com/go/bigquery,1,,,,,,,,,,,,,,1,,,,,,,,,,,,

@owen-mc
Add bigquery to frameworks.csv
fb92999 
Also fix up github.com/kanikanema/gorqlite
Napalys
Napalys approved these changes Jun 3, 2025
View reviewed changes
Copy link
Contributor

@Napalys Napalys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Just couple quick questions:

  • QueryString.ql file is something you drag along for inline tests?
  • The column within MaD which states manual, that one implies that the following MaD was added manually or does it mean something else?

@owen-mc
Copy link
Contributor Author

owen-mc commented Jun 3, 2025 •
edited
Loading

  • Yes, QueryString.ql is just there for inline tests. I copied it from neighbouring folders, which test SQL modelling for other libraries. Some of it is not relevant, but it seems simplest to me to be testing the same thing in all the different folders, so tests are more easily comparable.
  • Yes, that column is set to "Manual" if a human has created (or even checked) the model. We sometimes run bulk model-generation tools, and they set that column to something else, like "df-generated" for the data flow model generator. The original purpose of this was just to collect data on how many generated models we had, but now for some languages (like java) it is actually used in virtual dispatch, where "manual" indicates something has a higher confidence than "generated".

mbg
mbg reviewed Jun 3, 2025
View reviewed changes
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of questions here:

  1. If the QueryString.ql part is shared between multiple tests and there's nothing specific to this, would it make sense to move this to a shared location and have the code shared among tests, rather than duplicated?
  2. This may be a lack of understanding on my part of how all this fits together, but is there anything that's checking that the new sink is correctly identified as part of a SQL injection query? As far as I can tell, the bigquery.ql test only checks that the argument is correctly identified as a query string?

@owen-mc
Copy link
Contributor Author

owen-mc commented Jun 3, 2025

@mbg

  1. Do you have any suggestions for where to share it? All the tests for different SQL frameworks are in separate folders with separate go.mod files, which I think makes things a bit clearer, but if we went back to them all being the same folder then we would only need one copy of QueryString.ql. However, I would argue that it's too much work for this PR to reorganise the tests.
  2. These tests are for SQL framework tests. They are testing that we correctly identify APIs that have been modelled as SQL::QueryString and SQL::Query. It sounds like you are thinking of query tests for SQL injection which are in go/ql/test/query-tests/Security/CWE-089/. We do not currently test for the many different SQL frameworks in that query test. You could argue that we could, but I think it is too much work for this PR to reorganise the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbg mbg mbg left review comments

Copilot code review Copilot Copilot left review comments

@Napalys Napalys Napalys approved these changes

Assignees
No one assigned
Labels
documentation Go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@owen-mc @mbg @Napalys