Skip to content

Swift: Improve doc for swift/sql-injection#17127

Merged
geoffw0 merged 5 commits into
github:mainfrom
geoffw0:swiftsql
Aug 2, 2024
Merged

Swift: Improve doc for swift/sql-injection#17127
geoffw0 merged 5 commits into
github:mainfrom
geoffw0:swiftsql

Conversation

@geoffw0
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 commented Aug 1, 2024

Improve the swift/sql-injection qhelp and examples in various ways, with the aim of making it clearer what these problems look like and how they should be fixed.

geoffw0 added 3 commits August 1, 2024 11:52
… API that's used, adding SQLite3 C API examples, and adding an example of using a prepared statement incorrectly.
@geoffw0 geoffw0 added documentation no-change-note-required This PR does not need a change note ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. Swift labels Aug 1, 2024
@geoffw0 geoffw0 requested a review from a team as a code owner August 1, 2024 16:09
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 1, 2024

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

Example

In the following examples, an 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.

// 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

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.

// 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)
}

References

subatoi
subatoi previously approved these changes Aug 1, 2024
Copy link
Copy Markdown
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Comment thread swift/ql/src/queries/Security/CWE-089/SqlInjection.qhelp Outdated
Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM if @subatoi is happy.

Copy link
Copy Markdown
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@geoffw0 geoffw0 merged commit 9794309 into github:main Aug 2, 2024
@geoffw0 geoffw0 deleted the swiftsql branch October 29, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation no-change-note-required This PR does not need a change note ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. Swift

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants