C++: New query cpp/potential-system-data-exposure #8318

geoffw0 · 2022-03-03T12:50:21Z

This is a new query cpp/potential-system-data-exposure closely related to the existing cpp/system-data-exposure, both providing coverage of CWE-497. Roughly speaking, each result of the old cpp/system-data-exposure is now either:

still a result of cpp/system-data-exposure, if the information is exposed to a remote sink (this change was made in a previous PR).
a result of this new query cpp/potential-system-data-exposure, if the above does not hold but the information exposed is considered especially sensitive (e.g. a password being output to stdout).
not a result any more (the old query used to be very noisy).

This part is likely to remain @precision medium, because some projects will consider standard output and similar channels to be safe from attackers. cpp/system-data-exposure is likely to be upgraded to @precision high, as most of those results will carry some real risk.

Note that the diff is larger than the true number of changes because:

I've moved some existing QL code into a SystemData.qll library, shared by both queries.
I've migrated most of our internal tests for CWE-497 to the public repo (there's an internal PR removing them there).

LGTM results: https://lgtm.com/query/7993974152184061577/

results look reasonable to me
there are a few cases being picked up where we read into one field of a struct and output from another; this isn't ideal, but in most cases the result is reasonable regardless.

I've also started a DCA run.

…esults.

MathiasVP

This looks great! Here are my initial comments.

cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql

geoffw0 · 2022-03-04T15:00:16Z

DCA shows:

9 results for the new query; all are in kamailio__kamailio, matching results we'd already found via LGTM.
some performance wobbles, including analysis 3% slower overall; I don't believe this is significant.
cpp/potential-system-data-exposure is probably in the slower half of queries for performance, but far from the slowest, I don't think its causing expensive re-evaluations or anything like that.

cpp/ql/src/Security/CWE/CWE-497/SystemData.qll

geoffw0 · 2022-03-14T19:13:00Z

I think this is PR is ready, apart from merge conflicts and submodule stuff which I'll address tomorrow.

cpp/ql/src/Security/CWE/CWE-497/SystemData.qll

MathiasVP · 2022-03-15T13:33:38Z

LGTM! Do we want a docs review of these changes?

geoffw0 · 2022-03-15T14:59:54Z

Do we want a docs review of these changes?

Yeah, for the two .qhelp files.

@github/docs-content-codeql

docs-bot · 2022-03-15T16:22:01Z

📚 Thanks for the docs ping! 🛎️ This was added to our docs first-responder project board. A team member will be along shortly to respond. To request changes to the docs you can also open a CodeQL docs issue.

skedwards88 · 2022-03-22T20:43:35Z

Docs first responder here 👋 I've added this PR to our board for writer review.

mchammer01 · 2022-03-24T17:00:57Z

Not sure I'll get to reviewing this today. If not, I'll look at this tomorrow 👍🏻

mchammer01

@geoffw0 - LGTM ✨
Approving this so that I don't block this work.
I've left a few suggestions and comments for your consideration. Feel free to ignore them if you don't agree 🙂

Also, I didn't see a change note about the new query, but I'm not sure whether all CodeQL teams are adding change notes for new queries.

cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql

cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.qhelp

cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql

mchammer01 · 2022-03-25T09:01:48Z

cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.qhelp

+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Exposing system data or debugging information may help an adversary to learn about the system and form an attack plan.  An attacker can use error messages that reveal technologies, operating systems, and product versions to tune their attack against known vulnerabilities in these technologies.</p>


I'd make the same changes to to this qhelp file that I suggested for the other qhelp file adversary -> malicious user, remove the to in may help an adversary to learn about, and possibly find a synonym for technologies.
I am a bit confused because the content for both seems to be exactly the same 😕

Done.

We've essentially forked one query into two variations here, and ended up with two versions of the qhelp as well. There are slight differences reflecting the difference in focus between the two queries, most of the text remains identical.

Thanks for explaining, it now makes more sense to me 🙂

MathiasVP · 2022-03-25T09:10:14Z

Also, I didn't see a change note about the new query, but I'm not sure whether all CodeQL teams are adding change notes for new queries.

All language teams should add a change-note for a new query. It's also done in this PR. See this commit.

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>

geoffw0 · 2022-03-25T11:43:20Z

I think I've addressed all concerns / suggestions now.

MathiasVP

LGTM!

mchammer01 · 2022-03-25T14:54:09Z

@MathiasVP - thanks for pointing the change note to me, I didn't see it at all 😞
LGTM ✨

geoffw0 added 13 commits March 2, 2022 15:18

C++: Test layout.

da740cf

C++: Add a couple of new test cases.

19718fa

C++: Add some more test cases (moved from the private repo).

67aa144

C++: Create prototype query.

d95b56f

C++: Add the new query to tests.

70e4a40

C++: Move SystemData class into a library.

a1ace71

C++: There is no overlap between OutputWrite and RemoteFlowSinkFunction.

66b9356

C++: Fill in metadata.

2a14a4f

C++: Change note.

9e193f6

C++: Fix typo and adjust violation message wording.

6e5729c

C++: Use the same trick as in ExposedSystemData to catch a few more r…

07b4bf7

…esults.

C++: Make the bulk of test cases in tests.cpp more relevant.

88b7a08

C++: Improve and differentiate the qhelp.

5c6923c

geoffw0 added C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR labels Mar 3, 2022

geoffw0 requested a review from a team as a code owner March 3, 2022 12:50

github-actions bot added the documentation label Mar 3, 2022

C++: Autoformat.

6848b60

MathiasVP reviewed Mar 3, 2022

View reviewed changes

geoffw0 added 3 commits March 4, 2022 18:00

C++: VariableAccess -> Expr.

4316026

C++: Add more information about registry query parameters.

f1d6234

C++: Only look for sensitive strings in appropriate parameters.

cb33ed4

MathiasVP reviewed Mar 7, 2022

View reviewed changes

cpp/ql/src/Security/CWE/CWE-497/SystemData.qll Outdated Show resolved Hide resolved

C++: QLDoc.

9ebdb2a

MathiasVP added a commit to MathiasVP/ql that referenced this pull request Mar 8, 2022

C++: An alternative design for the IPA type proposed in github#8318.

b853824

geoffw0 added 3 commits March 14, 2022 17:53

C++: Use asDefiningArgument() where appropriate.

d1b04b4

C++: Fix large newtype.

7c93eb1

C++: Fix QLDoc.

73710e9

MathiasVP reviewed Mar 14, 2022

View reviewed changes

cpp/ql/src/Security/CWE/CWE-497/SystemData.qll Show resolved Hide resolved

geoffw0 added 2 commits March 15, 2022 11:23

Merge branch 'main' into cwe497b

28315df

Merge branch 'main' into cwe497b

71e0da7

MathiasVP previously approved these changes Mar 15, 2022

View reviewed changes

C++: Fix broken merge.

46f3f28

geoffw0 dismissed MathiasVP’s stale review via 46f3f28 March 15, 2022 14:55

MathiasVP added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Mar 15, 2022

geoffw0 added 2 commits March 15, 2022 20:00

C++: Fix ODR/dbcheck issue in test.

92d748e

Merge branch 'main' into cwe497b

95a63a6

mchammer01 self-requested a review March 24, 2022 15:56

mchammer01 previously approved these changes Mar 25, 2022

View reviewed changes

geoffw0 dismissed mchammer01’s stale review via 6b6ee61 March 25, 2022 11:06

geoffw0 and others added 3 commits March 25, 2022 11:06

Apply suggestions from code review

6b6ee61

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>

Update cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql

11074b6

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>

C++: More 'adversary' -> 'malicious user' and related doc changes.

e377eeb

MathiasVP approved these changes Mar 25, 2022

View reviewed changes

Merge branch 'main' into cwe497b

9f3fd57

geoffw0 merged commit 2014599 into github:main Mar 25, 2022

C++: New query cpp/potential-system-data-exposure #8318

C++: New query cpp/potential-system-data-exposure #8318

Uh oh!

Conversation

geoffw0 commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffw0 commented Mar 4, 2022

Uh oh!

Uh oh!

geoffw0 commented Mar 14, 2022

Uh oh!

Uh oh!

MathiasVP commented Mar 15, 2022

Uh oh!

geoffw0 commented Mar 15, 2022

Uh oh!

docs-bot commented Mar 15, 2022

Uh oh!

skedwards88 commented Mar 22, 2022

Uh oh!

mchammer01 commented Mar 24, 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!

mchammer01 Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

geoffw0 Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

mchammer01 Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented Mar 25, 2022

Uh oh!

geoffw0 commented Mar 25, 2022

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

mchammer01 commented Mar 25, 2022

Uh oh!

Uh oh!

geoffw0 commented Mar 3, 2022 •

edited

Loading