Swift: Uncontrolled format string query

[geoffw0]

Adds a query for uncontrolled format strings. Includes tests, docs, and a re-usable library for identifying format strings in the first place.

[github-actions]

QHelp previews:

swift/ql/src/queries/Security/CWE-134/UncontrolledFormatString.qhelp

Uncontrolled format string

Passing untrusted format strings to functions that use printf style formatting can lead to buffer overflows and data representation problems. An attacker may be able to exploit this weakness to crash the program or obtain sensitive information from its internal state.

Recommendation

Use a constant string literal for the format string to prevent the possibility of data flow from an untrusted source. This also helps to prevent errors where the format arguments do not match the format string.

If the format string cannot be constant, ensure that it comes from a secure data source or is compiled into the source code. If you need to include a string value from the user, use an appropriate specifier (such as %@) in the format string and include the user provided value as a format argument.

Example

In this example, the format string includes a user-controlled inputString:

print(String(format: "User input: " + inputString)) // vulnerable

To fix it, make inputString a format argument rather than part of the format string, as in the following code:

print(String(format: "User input: %@", inputString)) // fixed

References

• OWASP: Format string attack.
• Common Weakness Enumeration: CWE-134.

[MathiasVP]

A few comments.

[MathiasVP]

LGTM (with the exception of the missing autoformat)! Let's wait for @atorralba comments on dba3444 and 07ea006, though.

I don't think we've run this on DCA yet, have we?

[geoffw0]

I don't think we've run this on DCA yet, have we?

If I have it's evolved quite a bit since. I'll push the autoformat fix and run it now.

[geoffw0]

DCA is hard to read - a lot of noise is unfortunately normal on Swift right now - but I'm suspicious we have somewhat slow stage timings associated with swift/uncontrolled-format-string, e.g.

DanielZSY__RxCommonKit	swift/uncontrolled-format-string	1	?	?	107.6	0

I'm going to investigate locally next week. If I don't find anything, I'll re-run DCA instead.

[atorralba]

Let's wait for @atorralba comments on dba3444 and 07ea006, though.

The changes LGTM 👍

[MathiasVP]

DCA is hard to read - a lot of noise is unfortunately normal on Swift right now - but I'm suspicious we have somewhat slow stage timings associated with swift/uncontrolled-format-string, e.g.

DanielZSY__RxCommonKit	swift/uncontrolled-format-string	1	?	?	107.6	0

I'm going to investigate locally next week. If I don't find anything, I'll re-run DCA instead.

Indeed, it does look like the new query is substantially slower than the rest of the dataflow queries. The slowest one appears to be on freeotp/freeotp-ios.

[geoffw0]

I've been trying to understand this performance issue here, having run the query locally quite a few times I'm beginning to doubt it really exists. It appears that evaluating RemoteFlowSource (and likely other dataflow elements) for the first time is not cheap, and one or other injection query is going to pay the cost for that on any particular run.

I'm going to run DCA again and hopefully see through some of the noise...

[MathiasVP]

I've been trying to understand this performance issue here, having run the query locally quite a few times I'm beginning to doubt it really exists. It appears that evaluating RemoteFlowSource (and likely other dataflow elements) for the first time is not cheap, and one or other injection query is going to pay the cost for that on any particular run.

Indeed, it may be that swift/uncontrolled-format-string is the first query to evaluate dataflow (in which case it'll evaluate all the cached dataflow predicates and IPA branches). If that's what's happening then I'm perfectly happy with this 👍.

It is strange that freeotp/freeotp-ios is so much slower, though.

[geoffw0]

It is strange that freeotp/freeotp-ios is so much slower, though.

You're right - but the issue turns out to be nothing to do with the uncontrolled format string query. I've created a fix in #11708 .

[MathiasVP]

Great! Now the only thing left is making CI happy. I see it failed with FAILED(EXTRACTION), but otherwise this LGTM!

[geoffw0]

I think the second DCA run is just showing us (1) that analysis is slower with +1 query (2) the above discussed slowness on freeotp/freeotp-ios and (3) noise. i.e. things we were expecting.

[geoffw0]

I've just merged in latest main and fixed the merge (a toString had changed). qltests-linux (only) is still failing with:

Could not extract a dataset in /home/runner/work/codeql/codeql/swift/ql/test/query-tests/Security/CWE-134: Extraction command /home/runner/work/codeql/codeql/swift/extractor-pack/tools/qltest.sh failed with status 1. Use --show-extractor-output to see its output.

Its difficult to add --show-extractor-output on a PR check, and locally I have a Mac so I can't reproduce this. I'm not sure how to proceed.

[atorralba]

I've just merged in latest main and fixed the merge (a toString had changed). qltests-linux (only) is still failing with:

Could not extract a dataset in /home/runner/work/codeql/codeql/swift/ql/test/query-tests/Security/CWE-134: Extraction command /home/runner/work/codeql/codeql/swift/extractor-pack/tools/qltest.sh failed with status 1. Use --show-extractor-output to see its output.

Its difficult to add --show-extractor-output on a PR check, and locally I have a Mac so I can't reproduce this. I'm not sure how to proceed.

In my experience, that's normally caused by some function or class existing in the Mac Swift APIs but not in the Linux ones. When this happens, I use an Ubuntu VM where I try to compile the problematic test file and see exactly what's failing.

In this case, the error is the following:

test.swift:89:81: error: cannot find 'getVaList' in scope
    NSException.raise(NSExceptionName("exception"), format: tainted, arguments: getVaList([])) // BAD

I think stubbing getVaList should fix the issue.

[geoffw0]

Thanks, I've managed to fix it and will be on the lookout for this issue in future.

[geoffw0]

Looks like this is all good except I've forgotten to ask for a docs review...

[guntrip]

Reviewing on behalf of the docs-content team. These changes look good! ✨

[geoffw0]

Thanks for the docs review @stevecat . It looks like this PR has been through enough reviews, has had 👍 from all the right people, and now the checks pass and we have docs approval. Merging!