Swift: Add libxml2 sinks to the XXE query #11165
Conversation
Only XMLParser sinks for the time being
Use an alert message consistent with the other languages
|
QHelp previews: swift/ql/src/queries/Security/CWE-611/XXE.qhelpResolving XML external entity in user-controlled dataParsing 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. RecommendationThe 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 ExampleThe following example uses the To guard against XXE attacks, the References
|
| * Holds if taint can flow from `source` to `sink` in one local step, | ||
| * including bitwise operations, accesses to `.rawValue`, and casts to `Int32`. | ||
| */ | ||
| private predicate lib2xmlOptionLocalTaintStep(DataFlow::Node source, DataFlow::Node sink) { |
There was a problem hiding this comment.
This is a little ad-hoc, but the expected use should actually adhere to this pattern (Int32(XML_PARSE_NOENT.rawValue), potentially bitwise-OR'ed with other options, and reaching the options argument of a libxml2 XML-parsing call).
There was a problem hiding this comment.
Which of the steps in this predicate do you think should ultimately be ported to the data flow or taint flow libraries? Most of them look like they could be taint steps to me.
There was a problem hiding this comment.
The bitwise operation and .rawValue ones are probably of general interest as taint steps. The Int32 constructor technically applies globally too, although it may seem a little more niche.
| // --- tests --- | ||
|
|
||
| func sourcePtr() -> UnsafeMutablePointer<UInt8> { return UnsafeMutablePointer<UInt8>.allocate(capacity: 0) } | ||
| func sourceCharPtr() -> UnsafeMutablePointer<CChar> { return UnsafeMutablePointer<CChar>.allocate(capacity: 0) } |
There was a problem hiding this comment.
What do you think these sources would look like in actual Swift code? I'm very happy with this solution for the query test, but we might want to plan work to ensure proper taint flow through the UnsafeMutablePointer operations in real code leading up to an XML parse call.
We do have github/codeql-c-team#1333 planned if that's relevant.
There was a problem hiding this comment.
I tried to model them properly at first, but it started getting too complicated and I used this "dummy" sources as a fallback.
I think we'd need to model, at the very least, the various flavors of initialize and with*, which seem the ones that could translate our current sources to an UnsafeMutablePointer. Probably there are more, this would need its own PR to do it right I guess.
I don't think we should worry about direct UnsafeMutablePointer sources for now, but that would be easy to model anyway.
| * Holds if taint can flow from `source` to `sink` in one local step, | ||
| * including bitwise operations, accesses to `.rawValue`, and casts to `Int32`. | ||
| */ | ||
| private predicate lib2xmlOptionLocalTaintStep(DataFlow::Node source, DataFlow::Node sink) { |
There was a problem hiding this comment.
Which of the steps in this predicate do you think should ultimately be ported to the data flow or taint flow libraries? Most of them look like they could be taint steps to me.
Note this is built on top of #11086, #11120, and #11138 until those are merged (at which time this will rebased). It also cherry-picks ce86c71, but that'll be removed after rebase since it's already in
main. If you want to review early, look at commit a927ac2 only (and potentially any other commit after that).