Swift: Query for SQL injection #10757

geoffw0 · 2022-10-10T16:42:49Z

Query for SQL injection in Swift.

Draft: currently there are just test cases, covering the C API that can be used in Swift, and the SQLite.swift wrapper that I believe is commonly used as well. I'd like someone to review these test cases for realism / misconceptions / any important details I've missed before going too much further.

@atorralba would you mind taking a look?

github-actions · 2022-10-10T16:44:04Z

QHelp previews:

swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp

SQL query built from user-controlled sources

TODO

Recommendation

TODO

Example

TODO


TODO

TODO


TODO

References

TODO
Common Weakness Enumeration: CWE-89.

atorralba

The tests make sense to me. sqlite3_exec and sqlite3_prepare* seem to be the appropriate sinks, because they handle raw SQL statements. sqlite3_bind* handle the parameters of prepared statements, which is safe to do even with untrusted data.

This should cover the raw sqlite3 library. Are we planning on covering wrapper libraries on top of it? e.g. https://github.com/stephencelis/SQLite.swift or https://github.com/groue/GRDB.swift.

swift/ql/test/query-tests/Security/CWE-089/sqlite3_c_api.swift

geoffw0 · 2022-10-11T09:15:04Z

Are we planning on covering wrapper libraries on top of it? e.g. https://github.com/stephencelis/SQLite.swift

The tests in SQLite.swift should cover this library - assuming I've correctly identified the right / most important interfaces.

or https://github.com/groue/GRDB.swift.

Yeah it would be good to cover this library as well. However I'm a bit behind on this task as it is, so I'll create a follow-up ticket rather than addressing this now. --- update: added to the query priorities board.

atorralba

The tests in SQLite.swift should cover this library - assuming I've correctly identified the right / most important interfaces.

D'oh, true, sorry.

swift/ql/test/query-tests/Security/CWE-089/SQLite.swift

swift/ql/src/queries/Security/CWE-089/SqlInjection.ql

github-actions · 2022-10-14T14:20:53Z

QHelp previews:

swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp

errors/warnings:

./swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp:35:6: element "li" not allowed here; expected the element end-tag, text or element "a", "b", "blockquote", "code", "em", "i", "img", "ol", "p", "pre", "sample", "strong", "sub", "sup", "table", "tt", "ul" or "warning"
./swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp:36:6: element "li" not allowed here; expected the element end-tag, text or element "a", "b", "blockquote", "code", "em", "i", "img", "ol", "p", "pre", "sample", "strong", "sub", "sup", "table", "tt", "ul" or "warning"
A fatal error occurred: 1 qhelp files could not be processed.

github-actions · 2022-10-14T14:29:59Z

QHelp previews:

swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp

Database query built from user-controlled sources

If a database query (such as a SQL query) is built from user-provided data without sufficient sanitization, a user may be able to run malicious database queries. An attacker can craft the part of the query they control to change the overall meaning of the query.

Recommendation

Most database connector libraries offer a way to safely embed untrusted data into a query using query parameters or prepared statements. You should use these features to build queries, rather than string concatenation or similar methods without sufficient sanitization.

Example

In the following example, a SQL query is prepared using string interpolation to directly include a user-controlled value userControlledString in the query. An attacker could craft userControlledString to change the overall meaning of the SQL query.

let unsafeQuery = "SELECT * FROM users WHERE username='\(userControlledString)'" // BAD

try db.execute(unsafeQuery)

A better way to do this is with a prepared statement, binding userControlledString to a parameter of that statement. An attacker who controls userControlledString now cannot change the overall meaning of the query.

let safeQuery = "SELECT * FROM users WHERE username=?"

let stmt = try db.prepare(safeQuery, userControlledString) // GOOD
try stmt2.run()

References

Wikipedia: SQL injection.
OWASP: SQL Injection Prevention Cheat Sheet.
Common Weakness Enumeration: CWE-89.

geoffw0 · 2022-10-14T14:46:03Z

I've removed draft status from this PR, it's ready to review.

I'd like to acknowledge a few issues though. IMO none of these prevent us from merging what we have:

we're getting no results on the C API test due to an issue with taint flow, possibly itself caused by an extractor issue. I'm discussing this with other members of the team and I'm not sure if the fix will be something simple I can do here, or something that will need to be done as follow-up.
- this has been fixed, though there is still an issue with the UTF-16 results (probably a missing taint flow step).
  - issue for this: github/codeql-c-team#1269
there are no sanitizers for remote data in numerical form yet (considered to be safe). This is because in my test case taint doesn't flow into remoteNumber anyway, so there's no way to test such a sanitizer (and no strict need for it yet).
- included in issue: github/codeql-c-team#1270
we may also want sanitizers along the lines of Ruby's StringConstCompareBarrier and StringConstArrayInclusionCallBarrier. I might write a follow-up issue for this but I'm inclined not to worry too much until we see patterns like those in real Swift code (they are talked about in https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html).
- included in issue: github/codeql-c-team#1270

geoffw0 · 2022-10-17T12:44:56Z

I've just merged main into this PR - with the extractor fix #10854 that gives us the C API results for this query. We still don't get results through the UTF-16 interface, but that's much less concerning than missing all C API results.

I'd be happy to see this reviewed and merged, and deal with the remaining known issues as follow-up.

jketema

Two small questions. Otherwise this looks ok to me.

swift/ql/src/queries/Security/CWE-089/SqlInjection.ql

jketema · 2022-10-18T14:30:27Z

swift/ql/test/query-tests/Security/CWE-089/SQLite.swift

+	stmt9.bind(remoteString)
+	stmt9.bind([remoteString])
+	stmt9.bind(["username": remoteString])
+	try stmt9.run(remoteString)
+	try stmt9.run([remoteString])
+	try stmt9.run(["username": remoteString])
+	try stmt9.scalar(remoteString)
+	try stmt9.scalar([remoteString])
+	try stmt9.scalar(["username": remoteString])


These I do not quite understand, even after looking at the SQLite.swift docs. Shouldn't these be tagged with // GOOD, or am I missing something?

Yes this whole block is GOOD, I should probably label each line individually.

This is a very odd thing to do, I'm preparing a statement that contains a parameter (the ? on line 81), binding values to the parameter three times over, then running it six times with new bindings on the parameter each time. The point is simply to check that none of the 9 operations that bind values are flagged even with a tainted value - because binding values to parameters is the 'safe' way to pass tainted data into an SQL statement.

jketema · 2022-10-18T15:41:35Z

Does this still need a doc review?

geoffw0 · 2022-10-18T15:41:58Z

Oh, good point!

jketema

I'm happy with the current version, so just waiting for the doc review.

subatoi

Hi there @geoffw0, I'm the first responder for the Docs team this week so I've taken a look at this and left some small comments and asked a couple of minor questions. I hope this isn't adding too much overhead 🙏

I'll just tag my colleague @felicitymay as well just in case (might not be necessary but I'm still quite new to reviewing CodeQL! 😁 )

swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

geoffw0 · 2022-10-19T19:52:27Z

Thanks for the review @subatoi . I believe I've addressed all your points, but please don't hesitate to say if you think it needs more work.

swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp

subatoi

Just added a couple of minor last points to make the language a bit simpler. There's nothing wrong with the existing language; these suggestions are just minor and style-related. Otherwise approving and LGTM. Thank you!

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

geoffw0 · 2022-10-20T16:56:39Z

Thanks - those suggestions definitely help make things read a bit more easily. Please approve the PR if you're happy with the docs now (the code has already been reviewed and approved).

subatoi

Thanks @geoffw0, apologies, didn't realise my review would be dismissed. LGTM!

geoffw0 · 2022-10-20T17:55:21Z

Yep, whenever a change is pushed all reviews of the existing code are dismissed automatically. It makes sense but it can be a bit much sometimes (especially when the change is just accepting a suggestion from the approver!)

geoffw0 · 2022-10-20T17:55:45Z

Thanks again for the review, suggestions and approval. On to the next query!

geoffw0 added 3 commits Oct 10, 2022

Swift: Add framework for SQL Injection query.

bcab9d8

Swift: Test SQL injection via the C API.

964c924

Swift: Test SQL injection via the SQLite.swift library.

4258147

geoffw0 added no-change-note-required This PR does not need a change note Swift labels Oct 10, 2022

github-actions bot added the documentation label Oct 10, 2022

atorralba reviewed Oct 11, 2022

View changes

swift/ql/test/query-tests/Security/CWE-089/sqlite3_c_api.swift Outdated Show resolved Hide resolved

swift/ql/test/query-tests/Security/CWE-089/sqlite3_c_api.swift Outdated Show resolved Hide resolved

atorralba reviewed Oct 11, 2022

View changes

swift/ql/test/query-tests/Security/CWE-089/SQLite.swift Outdated Show resolved Hide resolved

swift/ql/test/query-tests/Security/CWE-089/SQLite.swift Outdated Show resolved Hide resolved

geoffw0 added 6 commits Oct 12, 2022

Swift: Update tests based on feedback.

5496b11

Swift: Add more test variants.

398b2a3

Swift: Rename to match other languages (except Java).

12cb099

Swift: Complete the rename.

ce5631e

Swift: Define SQL sinks.

7d78df2

Swift: Bring it all together into a query.

76ff593

github-code-scanning bot found potential problems Oct 14, 2022

View changes

swift/ql/src/queries/Security/CWE-089/SqlInjection.ql Fixed Show fixed Hide fixed

Swift: Query metadata.

3da3a27

github-code-scanning bot found potential problems Oct 14, 2022

View changes

swift/ql/src/queries/Security/CWE-089/SqlInjection.ql Fixed Show fixed Hide fixed

swift/ql/src/queries/Security/CWE-089/SqlInjection.ql Fixed Show fixed Hide fixed

geoffw0 added 2 commits Oct 14, 2022

Swift: More modern (?) phrasing.

24c6bb4

Swift: Qhelp.

227b10a

Swift: One more go at getting the query message how ql-for-ql wants it.

f96e4eb

Swift: Fix the qhelp.

8eccae1

geoffw0 marked this pull request as ready for review Oct 14, 2022

geoffw0 requested a review from a team as a code owner Oct 14, 2022

geoffw0 added 2 commits Oct 17, 2022

Merge branch 'main' into sqlinject

1301815

Swift: Updated results after merge with main.

1221cba

Swift: Fix bug for sqlite3_prepare_v3.

9767064

geoffw0 mentioned this pull request Oct 17, 2022

Swift: extract all ImplicitConversionExpr #10854

Merged

jketema previously approved these changes Oct 18, 2022

View changes

Swift: annotate all cases.

027b713

geoffw0 dismissed jketema’s stale review via 027b713 Oct 18, 2022

geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Oct 18, 2022

jketema reviewed Oct 18, 2022

View changes

subatoi reviewed Oct 19, 2022

View changes

geoffw0 and others added 4 commits Oct 19, 2022

Apply suggestions from code review

83dc6d1

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

Swift: More 'an SQL' -> 'a SQL'.

05d9c7b

Swift: Attempt to address qhelp suggestions.

495f744

Swift: Another qhelp edit.

5b1e138

subatoi reviewed Oct 20, 2022

View changes

swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp Outdated Show resolved Hide resolved

subatoi reviewed Oct 20, 2022

View changes

swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp Outdated Show resolved Hide resolved

subatoi previously approved these changes Oct 20, 2022

View changes

Apply suggestions from code review

661106c

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

geoffw0 dismissed subatoi’s stale review via 661106c Oct 20, 2022

subatoi approved these changes Oct 20, 2022

View changes

geoffw0 merged commit 1386435 into github:main Oct 20, 2022
13 checks passed

Swift: Query for SQL injection #10757

Swift: Query for SQL injection #10757

geoffw0 commented Oct 10, 2022 •

edited

github-actions bot commented Oct 10, 2022 •

edited

SQL query built from user-controlled sources

Recommendation

Example

References

atorralba left a comment

geoffw0 commented Oct 11, 2022 •

edited

atorralba left a comment

github-actions bot commented Oct 14, 2022

errors/warnings:

github-actions bot commented Oct 14, 2022 •

edited

Database query built from user-controlled sources

Recommendation

Example

References

geoffw0 commented Oct 14, 2022 •

edited

geoffw0 commented Oct 17, 2022

jketema left a comment

jketema Oct 18, 2022

geoffw0 Oct 18, 2022

jketema commented Oct 18, 2022

geoffw0 commented Oct 18, 2022

jketema left a comment

subatoi left a comment

geoffw0 commented Oct 19, 2022

subatoi left a comment

geoffw0 commented Oct 20, 2022

subatoi left a comment

geoffw0 commented Oct 20, 2022

geoffw0 commented Oct 20, 2022

Swift: Query for SQL injection #10757

Swift: Query for SQL injection #10757

Conversation

geoffw0 commented Oct 10, 2022 • edited

github-actions bot commented Oct 10, 2022 • edited

SQL query built from user-controlled sources

Recommendation

Example

References

atorralba left a comment

geoffw0 commented Oct 11, 2022 • edited

atorralba left a comment

github-actions bot commented Oct 14, 2022

errors/warnings:

github-actions bot commented Oct 14, 2022 • edited

Database query built from user-controlled sources

Recommendation

Example

References

geoffw0 commented Oct 14, 2022 • edited

geoffw0 commented Oct 17, 2022

jketema left a comment

jketema Oct 18, 2022

Choose a reason for hiding this comment

geoffw0 Oct 18, 2022

Choose a reason for hiding this comment

jketema commented Oct 18, 2022

geoffw0 commented Oct 18, 2022

jketema left a comment

subatoi left a comment

geoffw0 commented Oct 19, 2022

subatoi left a comment

geoffw0 commented Oct 20, 2022

subatoi left a comment

geoffw0 commented Oct 20, 2022

geoffw0 commented Oct 20, 2022

geoffw0 commented Oct 10, 2022 •

edited

github-actions bot commented Oct 10, 2022 •

edited

geoffw0 commented Oct 11, 2022 •

edited

github-actions bot commented Oct 14, 2022 •

edited

geoffw0 commented Oct 14, 2022 •

edited