Swift: Add path injection sinks for sqlite3 and SQLite.swift

[geoffw0]

Add swift/path-injection sinks for the sqlite3 and SQLite.swift libraries. There are two missing results that will require follow-up work after this PR:

• CSV sinks don't seem to be able to match EnumElementExprs (i.e. enum constructor calls).
• CSV sinks don't seem to be able to match writes to global (non-member) variables.

This is my first PR for a while that has a change note, and possibly the first PR for Swift with a change note. Does it look right? Do we need to flip any switches to enable change note generation / publishing / the pull request check?

[MathiasVP]

• CSV sinks don't seem to be able to match writes to global (non-member) variables.

I don't see any global variables in the added tests? In any case, Swift doesn't support global variable flow yet anyway.

This is my first PR for a while that has a change note, and possibly the first PR for Swift with a change note. Does it look right? Do we need to flip any switches to enable change note generation / publishing / the pull request check?

You don't need to do anything special other than creating the change note. (Publishing change notes happen as part of the release process.) I've added one small suggestion to move it to a different folder, though.

Otherwise, this LGTM

[geoffw0]

I don't see any global variables in the added tests?

The variable in question is sqlite3_temp_directory.

In any case, Swift doesn't support global variable flow yet anyway.

We don't need flow through global variables, just permitting one to be a sink (expressed as CSV). I almost have a solution to this locally, actually, but something about the post-update node part isn't working.

[MathiasVP]

The variable in question is sqlite3_temp_directory.

Oh, sorry. Completely missed that one!

We don't need flow through global variables, just permitting one to be a sink (expressed as CSV). I almost have a solution to this locally, actually, but something about the post-update node part isn't working.

Do you mean to catch the missing alert on this line:

sqlite3_temp_directory = UnsafeMutablePointer<CChar>(mutating: NSString(string: remoteString).utf8String) // $ MISSING: hasPathInjection=253

?

If so, I don't completely follow what you're saying about post-update nodes. There shouldn't be a post-update node for sqlite3_temp_directory since it's neither a qualifier for a field nor an argument that gets updated.

[geoffw0]

There shouldn't be a post-update node for sqlite3_temp_directory since it's neither a qualifier for a field nor an argument that gets updated.

Ah, I had assumed basically anything that is written to has a pre- and post-update node. But in the case of a simple assignment I can see there's little reason to be interested in a distinct 'pre-update' value, so the node itself is the post-update node I'm looking for.

[geoffw0]

Swift doesn't support global variable flow yet anyway.

So in:

myGlobal = tainted
somethingElse = myGlobal

we don't expect flow to reach anything on the second line yet. And there are no PostUpdate nodes. But on the first line, as far as I can tell taint doesn't even reach the DataFlow::Node on the left side of the assignment, for the access to myGlobal. Should it? Where should I look to fix this? Or if not, how should this type of sink be expressed?

[MathiasVP]

myGlobal = tainted

will probably be a SsaDefinitionNode (i.e., node.asDefinition() will have a result). I don't think we have any way to express this as a sink in MaD currently.

[geoffw0]

I don't think we have any way to express this as a sink in MaD currently.

I'll try expressing it as a non-MAD sink then, if that might work. I'll probably have to get back to this after my holiday now.

[geoffw0]

I've just pushed some fixes:

• writes to the global variable sqlite3_temp_directory are now recognized in QL but the solution isn't great - it's specific to assignment by AssignExpr.
  • I think this will have to do for now. It's far from the most important part of this PR.
  • do you happen to know if we have any issues about dataflow on global variables already?
• I also added a model for UnsafeMutablePointer.init(mutating:) to get the test case fully working.
• writes to the enum element decl Connection.Location.uri are now recognized in QL and the solution should be robust.
  • I briefly tried to make MAD recognize EnumElementDecls in interpretElement0 but that wasn't enough. I don't think it was able to decode the Argument[0] part on the enum decl.
  • I'll write up an issue about MAD for EnumElementDecls.
    • decided to wait and see if this comes up again. It might be very uncommon this matters.

[geoffw0]

I've just merged with main and fixed a merge conflict in PointerTypes.qll.

I think this PR is ready to merge (despite the two non-MAD sink models not being as general as I'd like; but to put it into perspective, they're just two of many sinks for this query).

[MathiasVP]

The changes LGTM! Did we ever run a DCA on this, though?

[geoffw0]

Running one now...

[MathiasVP]

Thanks a lot! Feel free to merge if DCA comes back happy 🤞

[geoffw0]

DCA shows a 3% analysis slowdown and 6 bad rows for stage timings. However the rows don't appear to relate to this PR and the slowdown is small and not consistent across projects. I think we're just looking at a bit of wobble.

So I'm merging this, but let me know if you disagree about dismissing the above, I can always investigate further after the fact.