Swift: Uncontrolled format string query #11529

geoffw0 · 2022-12-01T19:25:48Z

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

…apparently wasn't right.

github-actions · 2022-12-01T19:26:50Z

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.

swift/ql/lib/codeql/swift/security/UncontrolledFormatStringQuery.qll

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

geoffw0 · 2022-12-02T10:32:47Z

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

+from TaintedFormatConfiguration config, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode
+where config.hasFlowPath(sourceNode, sinkNode)
+select sinkNode.getNode(), sourceNode, sinkNode, "This format string depends on $@.",
+  sourceNode.getNode(), "this user-provided value"


We're now as close to the cs/uncontrolled-format-string message as I can get, without introducing the 'source type' string into the message (which is not really suitable for display to users; I'm not sure why the csharp query does that).

MathiasVP

A few comments.

swift/ql/lib/codeql/swift/security/UncontrolledFormatStringQuery.qll

swift/ql/src/queries/Security/CWE-321/HardcodedEncryptionKey.ql

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 · 2022-12-08T13:04:38Z

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 · 2022-12-08T21:10:55Z

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 · 2022-12-09T11:27:06Z

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

The changes LGTM 👍

MathiasVP · 2022-12-09T12:33:00Z

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 · 2022-12-15T10:45:13Z

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 · 2022-12-15T11:19:08Z

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 · 2022-12-15T12:46:15Z

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 · 2022-12-15T13:17:03Z

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

geoffw0 · 2022-12-15T15:24:43Z

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 · 2023-01-03T17:37:29Z

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 · 2023-01-09T09:14:48Z

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 · 2023-01-09T10:51:14Z

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

geoffw0 · 2023-01-12T11:08:12Z

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

stevecat

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

geoffw0 · 2023-01-17T16:16:06Z

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!

geoffw0 added 8 commits Dec 1, 2022

Swift: Uncontrolled format string query (initial version).

a221095

Swift: Add tests.

32c4728

Swift: Add doc.

2b61f26

Swift: Add metadata.

58e9a04

Swift: Add security-severity, and correct one for another query that …

87fa159

…apparently wasn't right.

Swift: Move query logic to a .qll.

4359686

Swift: Separate out a FormatString library as well.

ad05cc3

Swift: correct the example.

157a782

geoffw0 added no-change-note-required This PR does not need a change note Swift labels Dec 1, 2022

geoffw0 requested a review from a team as a code owner Dec 1, 2022

github-actions bot added the documentation label Dec 1, 2022

github-code-scanning bot found potential problems Dec 1, 2022

View changes

Swift: Corrections.

f7ebd13

geoffw0 force-pushed the format branch from e5612d5 to f7ebd13 Compare Dec 1, 2022

github-code-scanning bot found potential problems Dec 1, 2022

View changes

swift/ql/src/queries/Security/CWE-134/UncontrolledFormatString.ql Fixed Show fixed Hide fixed

swift/ql/src/queries/Security/CWE-134/UncontrolledFormatString.ql Fixed Show fixed Hide fixed

Swift: try again to satisfy ql-for-ql.

85a0a42

github-code-scanning bot found potential problems Dec 2, 2022

View changes

MathiasVP reviewed Dec 2, 2022

View changes

swift/ql/lib/codeql/swift/security/UncontrolledFormatStringQuery.qll Show resolved Hide resolved

swift/ql/src/queries/Security/CWE-321/HardcodedEncryptionKey.ql Outdated Show resolved Hide resolved

geoffw0 added 3 commits Dec 2, 2022

Swift: Revert security-severity on CWE-321, for now.

cf3345e

Swift: Add UncontrolledFormatStringExtensions.qll.

dba3444

Swift: Add support for CSV modelled sinks as well.

07ea006

MathiasVP reviewed Dec 8, 2022

View changes

Swift: Autoformat.

24ce1c2

Merge branch 'main' into format

1f7d96a

geoffw0 force-pushed the format branch from bcb661b to 1f7d96a Compare Dec 15, 2022

geoffw0 added 3 commits Dec 16, 2022

Merge branch 'main' into format

640f894

Merge branch 'main' into format

e05bb7f

Swift: Update .expected following a toString change in main.

fc646a6

Merge branch 'main' into format

e45750a

Swift: Add getVaList stub to the test.

9333e80

geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jan 12, 2023

stevecat approved these changes Jan 12, 2023

View changes

geoffw0 merged commit ea06ad1 into github:main Jan 17, 2023
19 checks passed

Dec	JAN	Feb
	18
2022	2023	2024

Swift: Uncontrolled format string query #11529

Swift: Uncontrolled format string query #11529

geoffw0 commented Dec 1, 2022

github-actions bot commented Dec 1, 2022 •

edited

Uncontrolled format string

Recommendation

Example

References

geoffw0 Dec 2, 2022

MathiasVP left a comment

MathiasVP left a comment

geoffw0 commented Dec 8, 2022

geoffw0 commented Dec 8, 2022

atorralba commented Dec 9, 2022

MathiasVP commented Dec 9, 2022

geoffw0 commented Dec 15, 2022

MathiasVP commented Dec 15, 2022 •

edited

geoffw0 commented Dec 15, 2022

MathiasVP commented Dec 15, 2022

geoffw0 commented Dec 15, 2022

geoffw0 commented Jan 3, 2023

atorralba commented Jan 9, 2023

geoffw0 commented Jan 9, 2023

geoffw0 commented Jan 12, 2023

stevecat left a comment

geoffw0 commented Jan 17, 2023

Swift: Uncontrolled format string query #11529

Swift: Uncontrolled format string query #11529

Conversation

geoffw0 commented Dec 1, 2022

github-actions bot commented Dec 1, 2022 • edited

Uncontrolled format string

Recommendation

Example

References

geoffw0 Dec 2, 2022

Choose a reason for hiding this comment

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP left a comment

Choose a reason for hiding this comment

geoffw0 commented Dec 8, 2022

geoffw0 commented Dec 8, 2022

atorralba commented Dec 9, 2022

MathiasVP commented Dec 9, 2022

geoffw0 commented Dec 15, 2022

MathiasVP commented Dec 15, 2022 • edited

geoffw0 commented Dec 15, 2022

MathiasVP commented Dec 15, 2022

geoffw0 commented Dec 15, 2022

geoffw0 commented Jan 3, 2023

atorralba commented Jan 9, 2023

geoffw0 commented Jan 9, 2023

geoffw0 commented Jan 12, 2023

stevecat left a comment

Choose a reason for hiding this comment

geoffw0 commented Jan 17, 2023

github-actions bot commented Dec 1, 2022 •

edited

MathiasVP commented Dec 15, 2022 •

edited