Skip to content

Swift: Add predicate injection query #11670

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

Merged

Conversation

atorralba
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 13, 2022

QHelp previews:

swift/ql/src/queries/Security/CWE-943/PredicateInjection.qhelp

Predicate built from user-controlled sources

Predicates represent logical conditions that can be used to check whether an object matches them. If a predicate is built from user-provided data without sufficient sanitization, an attacker may be able to change the overall meaning of the predicate.

Recommendation

When building a predicate from untrusted data, you should either pass it to the appropriate arguments parameter during initialization, or as an array of substitution variables before evaluation. You should not append or concatenate it to the body of the predicate.

Example

In the following insecure example, NSPredicate is built directly from data obtained from an HTTP request. This is untrusted, and can be arbitrarily set by an attacker to alter the meaning of the predicate:

let remoteString = try String(contentsOf: URL(string: "https://example.com/")!)

let filenames: [String] = ["img1.png", "img2.png", "img3.png", "img.txt", "img.csv"]

let predicate = NSPredicate(format: "SELF LIKE \(remoteString)")
let filtered = filenames.filter(){ filename in
    predicate.evaluate(with: filename)
}
print(filtered)

A better way to do this is to use the arguments parameter of NSPredicate's constructor. This prevents attackers from altering the meaning of the predicate, even if they control the externally obtained data, as seen in the following secure example:

let remoteString = try String(contentsOf: URL(string: "https://example.com/")!)

let filenames: [String] = ["img1.png", "img2.png", "img3.png", "img.txt", "img.csv"]

let predicate = NSPredicate(format: "SELF LIKE %@", remoteString)
let filtered = filenames.filter(){ filename in
    predicate.evaluate(with: filename)
}
print(filtered)

References

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

This looks great!

A few very nitpicky comments follow. We will also need a docs review as usual.

@atorralba atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Dec 15, 2022
geoffw0
geoffw0 previously approved these changes Dec 15, 2022
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM. Happy for this to be merged after docs review.

atorralba and others added 2 commits December 15, 2022 12:35
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
@mchammer01 mchammer01 requested a review from subatoi December 19, 2022 13:58
Copy link
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.

👋 from Docs—just a few minor suggestions!

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>
@atorralba
Copy link
Contributor Author

Thanks for the review @subatoi! 🙇 All suggestions applied.

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

Thanks @atorralba ! In the unlikely event you need anyone else from Docs to review this again after tomorrow and before the 3rd January, please post in #code-security-docs and someone should be able to help. Have a good break!

@MathiasVP MathiasVP merged commit 9be9636 into github:main Jan 9, 2023
@atorralba atorralba deleted the atorralba/swift/predicate-injection branch January 9, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 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.

4 participants