Swift: Add AEXML sinks to XXE query

[atorralba]

Note this is built on top of #11086 and #11120 until those are merged (at which time this will rebased). If you want to review early, look at commit bbf7bb3 only (and potentially any other commit after that).

The caveat of these sinks is that they heavily rely on local data flow, and because of that we might have some FNs when tracking the unsafe option all the way to the actual sink requires global data flow instead.

[github-actions]

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

• OWASP: XML External Entity (XXE) Processing.
• OWASP: XML External Entity Prevention Cheat Sheet.
• Timothy Morgen: XML Schema, DTD, and Entity Attacks.
• Timur Yunusov, Alexey Osipov: XML Out-Of-Band Data Retrieval.
• Common Weakness Enumeration: CWE-611.
• Common Weakness Enumeration: CWE-776.
• Common Weakness Enumeration: CWE-827.

[atorralba]

These two pragma[inline]s are an attempt at avoiding fully computing these classes, which seemed to be potentially too big depending on the codebase. Hopefully inlining them where they are used should add the appropriate context to prevent it, while allowing us to reuse this code, but please let me know if this solution is not desirable performance-wise and/or there's a better alternative. I'll launch DCA in any case to see how this behaves.

[geoffw0]

Yep, I agree we should do a DCA run to check how this does. Though I'm not overly concerned about Swift query performance - I think it will be more effective to do a review later on (when we have a full query suite and thus better data).

[geoffw0]

Looks like a good addition. 👍

[geoffw0]

The AEXML library itself has some defaults you don't specify here. Your test cases below obviously don't use them. Can you imagine it matters?

[atorralba]

The empty AEXMLOptions and String.Encoding = utf8 would hardly affect the results. At most, we could verify that we correctly have TNs when the default options: argument is used, but the test would look very similar to the ones explicitly setting an empty AEXMLOptions (i.e. the *safeImplicit test cases). I'm open to adding those additional cases nevertheless, if you think they are worth it.

[atorralba]

But if we decide not to, I could completely remove all defaults from the stubs, so that we are consistent and avoid potential confusion.

[geoffw0]

I'm happy how it is. But I have wondered when implementing tests myself whether I should put effort into modelling and using default values accurately to how the libraries do it. I think you're right there isn't much difference between using a default and using a non-default value that's the same as it anyway. The query certainly doesn't care if a default is there / used.