Repository navigation
Unified: Add SQL injection query #22942
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.
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
Open
geoffw0
wants to merge
15
commits into
github:main
Choose a base branch
from
geoffw0:unifiedsqlinjection
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
5e9026d
unified: Copy Swift SQL injection query and tests unchanged
geoffw0 14f7a14
unified: Collect original Swift SQL injection configuration
geoffw0 53152a3
unified: Port SQL injection query with annotated test baseline
geoffw0 8ce85ea
unified: Model unambiguous Swift SQL injection sinks
geoffw0 9f930b0
unified: Recognize SQL sinks by resolved parameter names
geoffw0 9a3c558
unified: Resolve SQLite run and scalar SQL sinks precisely
geoffw0 92ebcc0
unified: Clarify SQL injection prevention guidance
geoffw0 42158ff
unified: move the new test cases into organization of existing test c…
geoffw0 c0afa1f
unified: Add / edit doc and comments.
geoffw0 b1a43f7
unified: Add a few more test cases around parameter names and call ta…
geoffw0 2bcd3ab
unified: Clean up heuristicSink predicate.
geoffw0 5921954
unified: Undo the .qhelp change. I'm not convinced it's meaningfully…
geoffw0 c38ed2a
unified: Remove database-store sinks that duplicate sql-sinks (the da…
geoffw0 b7b57b2
unified: Add a couple more test cases.
geoffw0 8706d4f
unified: Implement Copilot suggestion.
geoffw0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
38 changes: 38 additions & 0 deletions
38
unified/ql/src/queries/security/CWE-089/SqlInjection.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
|
|
||
| <p> | ||
| 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. | ||
| </p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p> | ||
| 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. You can also escape (sanitize) user-controlled strings so that they can be included directly in an SQL command. A library function should be used for escaping, because this approach is only safe if the escaping function is robust against all possible inputs. | ||
| </p> | ||
|
|
||
| </recommendation> | ||
| <example> | ||
|
|
||
| <p>In the following examples, an SQL query is prepared using string interpolation to directly include a user-controlled value <code>userControlledString</code> in the query. An attacker could craft <code>userControlledString</code> to change the overall meaning of the SQL query. | ||
| </p> | ||
|
|
||
| <sample src="SqlInjectionBad.swift" /> | ||
|
|
||
| <p>A better way to do this is with a prepared statement, binding <code>userControlledString</code> to a parameter of that statement. An attacker who controls <code>userControlledString</code> now cannot change the overall meaning of the query. | ||
| </p> | ||
|
|
||
| <sample src="SqlInjectionGood.swift" /> | ||
|
|
||
| </example> | ||
| <references> | ||
|
|
||
| <li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li> | ||
| <li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html">SQL Injection Prevention Cheat Sheet</a>.</li> | ||
|
|
||
| </references> | ||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| /** | ||
| * @name Database query built from user-controlled sources | ||
| * @description Building a database query from user-controlled sources is vulnerable to insertion of malicious code by the user. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @security-severity 8.8 | ||
| * @precision high | ||
| * @id unified/swift/sql-injection | ||
| * @tags security | ||
| * external/cwe/cwe-089 | ||
| */ | ||
|
|
||
| import unified | ||
| private import codeql.unified.internal.typeinference.TypeInference as TypeInference | ||
|
|
||
| private predicate sqliteSwiftSink(DataFlow::Node node) { | ||
| // TODO: Use MaD once legacy selectors distinguish Connection methods from Statement bindings. | ||
| exists(CallExpr call, FunctionDeclaration target | | ||
| target = TypeInference::resolveCallTarget(call) and | ||
| target.getEnclosingClass().getName() = "Connection" and | ||
| target.getName() = ["prepare", "run", "scalar"] and | ||
| node.asExpr() = call.getArgument(0).getValue() | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * An SQL injection sink that is determined by imprecise methods. | ||
| */ | ||
| private predicate heuristicSink(DataFlow::Node node) { | ||
| exists(Argument arg | | ||
| node.asExpr() = arg.getValue() and | ||
| ( | ||
| // by argument name | ||
| arg.getName() = "sql" | ||
| or | ||
| // by parameter name | ||
| // TODO: make a helper predicate `Argument.getParameter` for this? | ||
| exists(CallExpr call, int index, Parameter param | | ||
| arg = call.getArgument(index) and | ||
| param = TypeInference::resolveCallTarget(call).getParameter(index) and | ||
|
geoffw0 marked this conversation as resolved.
Resolved
|
||
| param.getPattern().(Identifier).getValue() = "sql" | ||
| ) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * A taint configuration for tainted data that reaches a SQL sink. | ||
| */ | ||
| module SqlInjectionConfig implements DataFlow::ConfigSig { | ||
| predicate isSource(DataFlow::Node node) { Models::isSource(node, _) } | ||
|
|
||
| predicate isSink(DataFlow::Node node) { | ||
| Models::isSink(node, "sql-injection") | ||
| or | ||
| sqliteSwiftSink(node) | ||
| or | ||
| heuristicSink(node) | ||
| } | ||
|
|
||
| predicate isBarrier(DataFlow::Node node) { | ||
| // TODO: Port the numeric-type barrier from original Swift's SqlInjectionExtensions.qll. | ||
| none() | ||
| } | ||
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
| } | ||
|
|
||
| module SqlInjectionFlow = TaintTracking::Global<SqlInjectionConfig>; | ||
|
|
||
| import SqlInjectionFlow::PathGraph | ||
|
|
||
| from SqlInjectionFlow::PathNode sourceNode, SqlInjectionFlow::PathNode sinkNode | ||
| where SqlInjectionFlow::flowPath(sourceNode, sinkNode) | ||
| select sinkNode.getNode(), sourceNode, sinkNode, "This query depends on a $@.", | ||
| sourceNode.getNode(), "user-provided value" | ||
12 changes: 12 additions & 0 deletions
12
unified/ql/src/queries/security/CWE-089/SqlInjectionBad.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // with SQLite.swift | ||
|
|
||
| let unsafeQuery = "SELECT * FROM users WHERE username='\(userControlledString)'" | ||
|
|
||
| try db.execute(unsafeQuery) // BAD | ||
|
|
||
| let stmt = try db.prepare(unsafeQuery) // also BAD | ||
| try stmt.run() | ||
|
|
||
| // with SQLite3 C API | ||
|
|
||
| let result = sqlite3_exec(db, unsafeQuery, nil, nil, nil) // BAD |
19 changes: 19 additions & 0 deletions
19
unified/ql/src/queries/security/CWE-089/SqlInjectionGood.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // with SQLite.swift | ||
|
|
||
| let safeQuery = "SELECT * FROM users WHERE username=?" | ||
|
|
||
| let stmt = try db.prepare(safeQuery, userControlledString) // GOOD | ||
| try stmt.run() | ||
|
|
||
| // with sqlite3 C API | ||
|
|
||
| var stmt2: OpaquePointer? | ||
|
|
||
| if (sqlite3_prepare_v2(db, safeQuery, -1, &stmt2, nil) == SQLITE_OK) { | ||
| if (sqlite3_bind_text(stmt2, 1, userControlledString, -1, SQLITE_TRANSIENT) == SQLITE_OK) { // GOOD | ||
| let result = sqlite3_step(stmt2) | ||
|
|
||
| // ... | ||
| } | ||
| sqlite3_finalize(stmt2) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not gonna spend time looking at this hack, but I hope you'll try to discourage the agent from introducing more of these workarounds. They just slow us down.