Swift: Queries for CWE-311 (originally CWE-200) #10061

geoffw0 · 2022-08-16T09:07:49Z

The idea is to have two queries, one for "Cleartext storage of sensitive information in a local database" and one for "Cleartext transmission of sensitive information". Both will use the same heuristic framework for determining what is sensitive information, but each will have different taint sinks. Right now there are no queries, just test cases covering the following:

storage using the Core Data database library
storage using the Realm database library
transmission as part of a URL
transmission using NWConnection.send

At this stage I would appreciate a review of these tests, with a particular focus on whether I've modelled Core Data and Realm in a reasonably realistic way (albeit with a lot of setup + result handling code missing as its long winded and should be irrelevant to the queries). I'm also open to other suggestions, including other sinks that might be worth considering.

…e (it has alternative meanings so is not a good test).

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql

geoffw0 · 2022-08-25T22:01:52Z

I've just pushed a load of progress.

SensitiveExprs.qll library, which detects likely sensitive / private data heuristically (based on the proven CPP implementation; though we'll probably want to refine things a bit more for the Swift eco-system when we have more data).
written the queries.
.qhelp for the queries.

There are some refinements I still want to make (to deal with some potential false positives; and spot certain types of sensitive data more reliably), but I'd rather do these things as follow-up. This PR is already quite big and I believe complete enough to be reviewed and merged now.

geoffw0 · 2022-08-25T22:31:30Z

The merge with main has caused some non-trivial problems in the taint flow. I'll have to fix them next week.

I've requested a docs review for the two .qhelp files.

mchammer01 · 2022-08-30T07:24:49Z

@geoffw0 👋🏻 - just wondering if this is ready for editorial review. I am confused by the presence of the ready-for-doc-review label in this draft PR. Thanks for clarifying 🙏🏻 🙂

geoffw0 · 2022-08-30T08:31:24Z

just wondering if this is ready for editorial review.

The docs are finished and ready for review. Something needs fixing with the query itself running on latest main, which I need to look into, but the fix won't affect docs.

mchammer01 · 2022-08-30T08:56:52Z

Great, thanks for clarifying @geoffw0 ⚡

mchammer01

@geoffw0 - this LGTM ✨
I've made minor comments on punctuation. You can ignore any you don't agree with, but it would be great to add the full stops to the list items in the references.
Also, perhaps I misunderstood but I thought these two queries related to CWE 311 🤔
The previews failed so I wasn't able to preview the qhelp files. Also, do we need to add something to the changelog?

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.qhelp

swift/ql/src/queries/Security/CWE-311/CleartextTransmission.qhelp

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.qhelp

swift/ql/src/queries/Security/CWE-311/CleartextTransmission.qhelp

swift/ql/src/queries/Security/CWE-311/CleartextTransmission.ql

mchammer01 · 2022-08-30T11:17:19Z

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql

+ * @precision medium
+ * @id swift/cleartext-storage-database
+ * @tags security
+ *       external/cwe/cwe-312


Shouldn't this be CWE 311?

Suggested change

* external/cwe/cwe-312

* external/cwe/cwe-311

Again, CWE-312 is a child of CWE-311. The relevant part of the hierarchy looks something like this:

CWE-693: Protection Mechanism Failure |-- ... \-- CWE-311: Missing Encryption of Sensitive Data |-- CWE-312: Cleartext Storage of Sensitive Information \-- CWE-319: Cleartext Transmission of Sensitive Information

Thanks for clarifying 🙇🏻‍♀️

….qhelp Co-authored-by: mc <[email protected]>

Co-authored-by: mc <[email protected]>

geoffw0 · 2022-08-30T11:36:20Z

Thanks for the review @mchammer01 !

geoffw0 · 2022-08-30T13:22:58Z

The previews failed so I wasn't able to preview the qhelp files.

Where is this?

Also, do we need to add something to the changelog?

Swift isn't released to the public yet, so we're not writing change notes until then.

mchammer01 · 2022-08-30T13:26:34Z

Ah, the previews ran correctly now (see QHelp Preview Checks check above, which has a green tick). When I reviewed the PR, it had failed.

geoffw0 · 2022-08-30T17:14:37Z

The merge issues are fixed, the query is now working again. And ready to review.

Some of the results are less than perfect, that will be improved by one or more follow-up PRs at some stage. Right now I just want to get the essentials in.

MathiasVP

A couple of questions. LGTM otherwise!

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql

…e special flow cases.

geoffw0 · 2022-09-01T08:13:17Z

All comments addressed (thanks for the review and help).

MathiasVP

LGTM!

MathiasVP · 2022-09-01T08:37:25Z

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql

+  override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
+    // flow out from fields of a `RealmSwiftObject` at the sink, for example in `obj.var = tainted; sink(obj)`.
+    isSink(node) and
+    exists(ClassDecl cd |
+      c.getAReadContent().(DataFlow::Content::FieldContent).getField() = cd.getAMember() and
+      cd.getType().(NominalType).getABaseType*().getName() = "RealmSwiftObject"
+    )
+    or
+    // any default implicit reads
+    super.allowImplicitRead(node, c)
+  }
+}


Fantastic! I'm very happy to see that we could get rid of all the additional taint steps 😍. I just opened #10254 which will hopefully make it possible to get a base type declaration without going the types themselves. But we can always do that change as a follow-up to avoid an unnecessary merge from main.

geoffw0 added 2 commits August 11, 2022 16:10

Swift: Create query + test stubs.

c1be060

Swift: Add many tests.

dd51b7f

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

geoffw0 added 10 commits August 25, 2022 22:15

Swift: Additional test cases.

1c52836

Swift: Make tests more accurate and don't use 'pwd' as a variable nam…

2aa6dd2

…e (it has alternative meanings so is not a good test).

Swift: Add a SensitiveExprs lib (and test it).

dacb7f5

Swift: CleartextTransmission query.

0cd2efc

Swift: Fix duplicate results.

456ab98

Swift: Core Data support.

3126fb9

Swift: Realm database support.

698a9e2

Swift: Special cases to get taint flow working.

2690732

Swift: Qhelp and examples for both queries.

9a53a40

Swift: Query descriptions and metadata.

926da4b

github-actions bot added the documentation label Aug 25, 2022

github-advanced-security bot found potential problems Aug 25, 2022

View reviewed changes

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql Fixed Show resolved Hide resolved

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql Fixed Show fixed Hide fixed

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql Fixed Show fixed Hide fixed

geoffw0 added 2 commits August 25, 2022 23:06

Swift: Make QL-for-QL happy.

7fd64f1

Merge branch 'main' into cleartext

2d57786

geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Aug 25, 2022

mchammer01 previously approved these changes Aug 30, 2022

View reviewed changes

Update swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase…

645364e

….qhelp Co-authored-by: mc <[email protected]>

geoffw0 dismissed mchammer01’s stale review via 645364e August 30, 2022 11:24

Apply suggestions from code review

4984d8f

Co-authored-by: mc <[email protected]>

geoffw0 added 2 commits August 30, 2022 18:04

Swift: Fix issues.

430a8e1

Swift: Add missing test annotations.

3e4a6be

geoffw0 marked this pull request as ready for review August 30, 2022 17:13

geoffw0 requested a review from a team as a code owner August 30, 2022 17:13

MathiasVP previously approved these changes Aug 30, 2022

View reviewed changes

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql Outdated Show resolved Hide resolved

swift/ql/src/queries/Security/CWE-311/CleartextStorageDatabase.ql Outdated Show resolved Hide resolved

geoffw0 added 3 commits August 31, 2022 16:04

Merge remote-tracking branch 'upstream/main' into swiftcleanup

60fad4d

Swift: Accept test changes after merging main (again).

c0bc0d7

Swift: Use allowImplicitRead as a better solution replacing one of th…

129ed42

…e special flow cases.

geoffw0 dismissed MathiasVP’s stale review via 129ed42 August 31, 2022 17:01

geoffw0 added 2 commits September 1, 2022 08:53

Swift: Remove special case (no longer required).

92a927e

Swift: Fix finding base classes.

d3250a7

MathiasVP approved these changes Sep 1, 2022

View reviewed changes

MathiasVP merged commit 96752f0 into github:main Sep 1, 2022

geoffw0 mentioned this pull request Sep 1, 2022

Swift: Use TypeDecl.getABaseTypeDecl(). #10256

Merged

Swift: Queries for CWE-311 (originally CWE-200) #10061

Swift: Queries for CWE-311 (originally CWE-200) #10061

Uh oh!

Conversation

geoffw0 commented Aug 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffw0 commented Aug 25, 2022

Uh oh!

geoffw0 commented Aug 25, 2022

Uh oh!

mchammer01 commented Aug 30, 2022

Uh oh!

geoffw0 commented Aug 30, 2022

Uh oh!

mchammer01 commented Aug 30, 2022

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mchammer01 Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

geoffw0 Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

mchammer01 Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

geoffw0 commented Aug 30, 2022

Uh oh!

geoffw0 commented Aug 30, 2022

Uh oh!

mchammer01 commented Aug 30, 2022

Uh oh!

geoffw0 commented Aug 30, 2022

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

geoffw0 commented Sep 1, 2022

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MathiasVP Sep 1, 2022 •

edited

Loading