Skip to content

Swift: Add new query for XML External Entities (XML) vulnerabilities #11086

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
merged 7 commits into from
Nov 14, 2022

Conversation

atorralba
Copy link
Contributor

Adds a new query to cover XXE vulnerabilities. Only covers XMLParser sinks for now, follow up PRs will add more sinks in third-party libraries.

Note that the CSV summaries added for the classes Data and InputStream are the bare minimum for the XXE tests to work properly. These classes need proper modeling, which deserve their own PRs.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

QHelp previews:

swift/ql/src/queries/Security/CWE-611/XXE.qhelp

Resolving XML external entity in user-controlled data

Parsing untrusted XML files with a weakly configured XML parser may lead to an XML External Entity (XXE) attack. This type of attack uses external entity references to access arbitrary files on a system, carry out denial-of-service attacks, or server-side request forgery. Even when the result of parsing is not returned to the user, out-of-band data retrieval techniques may allow attackers to steal sensitive data. Denial of services can also be carried out in this situation.

Recommendation

The easiest way to prevent XXE attacks is to disable external entity handling when parsing untrusted data. How this is done depends on the library being used. Note that some libraries, such as recent versions of XMLParser, disable entity expansion by default, so unless you have explicitly enabled entity expansion, no further action needs to be taken.

Example

The following example uses the XMLParser class to parse a string data. If that string is from an untrusted source, this code may be vulnerable to an XXE attack, since the parser is also setting its shouldResolveExternalEntities option to true:

let parser = XMLParser(data: remoteData) // BAD (parser explicitly enables external entities)
parser.shouldResolveExternalEntities = true

To guard against XXE attacks, the shouldResolveExternalEntities option should be left unset or explicitly set to false.

let parser = XMLParser(data: remoteData) // GOOD (parser explicitly disables external entities)
parser.shouldResolveExternalEntities = false

References

@atorralba atorralba force-pushed the atorralba/swift/xxe-query branch from f9c49bb to da67b10 Compare November 3, 2022 11:39
@geoffw0
Copy link
Contributor

geoffw0 commented Nov 3, 2022

I think this might be ready for docs review?

@atorralba atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Nov 4, 2022
@mchammer01 mchammer01 self-requested a review November 8, 2022 14:10
@mchammer01
Copy link
Contributor

I'll review this for Docs 😃

mchammer01
mchammer01 previously approved these changes Nov 8, 2022
Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

@atorralba - this LGTM ✨
A few minor suggestions 🙂

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@atorralba
Copy link
Contributor Author

Hey @mchammer01 thanks for the review! Actually, the QHelp file was copy-pasted from the XXE query of other languages, but still I applied your suggestions. We can always apply those same fixes to the other languages in a follow-up PR 😄

@mchammer01
Copy link
Contributor

Oh sorry @atorralba - I didn't know that 🙈

Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

LGTM 💖

@geoffw0
Copy link
Contributor

geoffw0 commented Nov 14, 2022

DCA run looks good to me (aside from some noise that has nothing to do with the new query). 👍

@atorralba atorralba merged commit a21db3b into github:main Nov 14, 2022
@atorralba atorralba deleted the atorralba/swift/xxe-query branch November 14, 2022 11:34
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