1 capture

02 May 2025

About this capture

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback  

Saved searches  

Use saved searches to filter your results more quickly

To see all available qualifiers, see our documentation.

C++: New query for 'Cleartext transmission of sensitive information'  #6713  

New issue  

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.  

Sign up for GitHub  

By clicking “Sign up for GitHub”, you agree to our terms of service and  privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?  Sign in  to your account  



Jump to bottom  





Merged  

MathiasVP  merged 20 commits into  github:main from geoffw0:cwe139  
Oct 1, 2021  






Merged  

C++: New query for 'Cleartext transmission of sensitive information'   #6713

MathiasVP  merged 20 commits into  github:main from geoffw0:cwe139  
Oct 1, 2021  

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.  Learn more about bidirectional Unicode characters  
Show hidden characters

Copy link  

Contributor  

geoffw0   commented  Sep 17, 2021  

Add a new query 'Cleartext transmission of sensitive information' (cpp/cleartext-transmission). This fills a gap in our existing queries 'Cleartext storage of sensitive information in file' (cpp/cleartext-storage-file), 'Cleartext storage of sensitive information in buffer' (cpp/cleartext-storage-buffer) and 'Cleartext storage of sensitive information in an SQLite database' (cpp/cleartext-storage-database) - and allows us to catch the SAMATE Juliet tests cases for CWE-319.

---

Results on LGTM: https://lgtm.com/query/8348355846815574667/ --- decent number of results, without being noisy on any particular project. Most results look worthwhile, though it's hard to judge which if any are actual security vulnerabilities without digging deep into the design of the software in question. The alibaba/AliSQL is notable as a likely FP, as the path goes through a function encrypt_and_write_file, which presumably does what it says. The result in openwall/john also hints at encryption being present.

Results on SAMATE Juliet tests: https://jenkins.internal.semmle.com/job/Security/job/SAMATE/job/SAMATE-cpp-detailed/163/ --- we catch all of the CWE-319 tests, but unfortunately produce a large number of FPs on that same CWE.

Results on DCA: https://github.com/github/codeql-dca-main/issues/285 --- 448 results in the SAMATE Juliet test, 1 result in git. The latter looks like a reasonable result, and also shows up in the LGTM run (where it's easier to view).

---

I've given the query @precision medium for now, with the hopes of fixing most of the FPs in a follow-up and upgrading it to high.
 

geoffw0   added 12commits  September 6, 2021 18:11











C++: Add CleartextTransmission query.  






2463024  













C++: Add basic test.  






cd5a534  













C++: Test dataflow and other slightly more complex cases.  






29ad3bf  













C++: Support 'send' as well.  






1707d67  













C++: Support various functions / variants.  






3ba9e80  













C++: Fix false positives involving STDIN_FILENO.  






e696eaa  













C++: Full dataflow version.  






f58177f  













C++: Upgrade to path problem.  






ee7ccd7  













C++: Add a test demonstrating taint.  






0e8064d  













C++: We get many more real world results using taint tracking.  






67c6b35  













C++: Assign precision and severity; medium for now, since there are F…   …  






a3de94e  

…Ps in SAMATE Juliet.

CPP: Fix QLDoc comments.  






90bc138  











geoffw0  added  the  C++  label  Sep 17, 2021  




geoffw0  requested a review  from a team  as a code owner  September 17, 2021 14:05 




github-actions bot  added  the  documentation  label  Sep 17, 2021  














C++: Change note.  






5124345  













MathiasVP   reviewed   Sep 17, 2021  

View reviewed changes  

Copy link  

Contributor  

MathiasVP   left a comment  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

This looks really good! I'm pretty happy with the code as-is, but I'll give the rest of the team some time to review it as well.
 

cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql  

		*
		* note: functions such as `write` may be writing to a network source or a file. We could attempt to determine which, and sort results into `cpp/cleartext-transmission` and perhaps `cpp/cleartext-storage-file`. In practice it usually isn't very important which query reports a result as long as its reported exactly once.
		*/
		class NetworkSend extends NetworkSendRecv {

Copy link  

Contributor  

MathiasVP   Sep 17, 2021   •  

edited  

Loading  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Could we replace this whole class with RemoteFlowSinkFunction?
 

Copy link  

Contributor   Author  

geoffw0   Sep 17, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Yes! Ish...

First, this class is a call toaRemoteFlowSinkFunction/RemoteFlowSourceFunction, not a RemoteFlowSinkFunction/RemoteFlowSourceFunction itself, so I'll still have to wrap those - unless there's a cleaner way to wire it up. But this does avoid duplicating the list of functions, which is a good thing.

Second, RemoteFlowSourceFunction doesn't have an equivalent of getSocketExpr() for ruling out reads from standard input. We could have RemoteFlowSourceFunction expose this information or even do the work to rule out these sources itself?

Third, RemoteFlowSourceFunction also includes getEnv (I'm not sure why, I thought the whole point was to be able to specify sources a bit more precisely). So ideally we'd use something finer that doesn't include that (but Recv is currently private).
 

Copy link  

Contributor  

MathiasVP   Sep 17, 2021   •  

edited  

Loading  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Second, RemoteFlowSourceFunction doesn't have an equivalent of getSocketExpr() for ruling out reads from standard input. We could have RemoteFlowSourceFunction expose this information or even do the work to rule out these sources itself?


This seems like a thing we would want to do in RemoteFlowSourceFunction, yeah.


Third, RemoteFlowSourceFunction also includes getEnv (I'm not sure why, I thought the whole point was to be able to specify sources a bit more precisely). So ideally we'd use something finer that doesn't include that (but Recv is currently private).


I think getEnv is only included in LocalFlowSourceFunction, right? That's at least what I see here.
 

Copy link  

Contributor  

rdmarsh2   Sep 17, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

First, this class is a call toaRemoteFlowSinkFunction/RemoteFlowSourceFunction, not a RemoteFlowSinkFunction/RemoteFlowSourceFunction itself, so I'll still have to wrap those - unless there's a cleaner way to wire it up. But this does avoid duplicating the list of functions, which is a good thing.


We have RemoteFlowSource wrapping RemoteFlowSourceFunctioninsemmle.code.cpp.models.interfaces.FlowSource, but no equivalent for sinks.
 

👍 1 geoffw0 reacted with thumbs up emoji

Copy link  

Contributor   Author  

geoffw0   Sep 24, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Could we replace this whole class with RemoteFlowSinkFunction?
Could we replace this whole file with RemoteFlowSourceFunction?


I've updated the code to (more-or-less) do this.

The following are considered a RemoteFlowSourceFunction (# = not already modelled by NetworkRecv): recv, recvfrom, recvmsg, read, pread, readv, preadv, preadv2#, gets#, fgets#, fgetws#, getdelim#, getwdelim#, __getdelim#, fread#. Some of the new functions read from a socket, some from a FILE *, and I'm a bit concerned that we'll get overlaps with cpp/cleartext-storage-file from the latter. They're differently structured queries though so I'm not sure this will really be an issue in practice. Probably best to fix it when we see it.

The following are considered a RemoteFlowSinkFunction (all already modelled by NetworkSend): send, sendto, sendmsg, write, writev, pwritev, pwritev2. So no problems there.


We have RemoteFlowSource wrapping RemoteFlowSourceFunctioninsemmle.code.cpp.models.interfaces.FlowSource, but no equivalent for sinks.


This looks promising, but it's in the security.FlowSources lib which conflicts with dataflow.TaintTracking (and as you point out RemoteFlowSink would need to be added). This PR has enough in it that I won't do this change here, but if you feel strongly I can do it as a follow-up?



Second, RemoteFlowSourceFunction doesn't have an equivalent of getSocketExpr() for ruling out reads from standard input. We could have RemoteFlowSourceFunction expose this information or even do the work to rule out these sources itself?




This seems like a thing we would want to do in RemoteFlowSourceFunction, yeah.


It turns out RemoteFlowSourceFunction can't rule out stdin/stdout by itself, as it's modelling the function not the call and thus has no idea of parameter values. We could potentially do this work in an improved RemoteFlowSource class at some point. But for now I've added a hasSocketInput predicate to the two models so that users can tell which parameter is the socket (if any).


I think getEnv is only included in LocalFlowSourceFunction, right?


Correct, thanks.

---

TL;DR: I've updated the query to use the RemoteFlowSourceFunction and RemoteFlowSinkFunction models. I believe all points above have been addressed, apart from the possible follow-up PR improving RemoteFlowSource.
 

👍 1 MathiasVP reacted with thumbs up emoji

Copy link  

Contributor  

jbj   Sep 27, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

One of the reasons we generally prefer modelling Function rather than Call is that a Function model also applies directly to IR dataflow, whereas a Call model has to be translated. Of course, a Call model can have more details. The hybrid you've arrived at here LGTM.
 

Copy link  

Contributor   Author  

geoffw0   Sep 28, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

It might be better (at least, less code) to join the NetworkSend and NetworkReceive parts at the Function level, then look at calls further down. I'll have a go at a refactor...
 

Copy link  

Contributor   Author  

geoffw0   Sep 28, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

It doesn't make much difference to code quantity or clarity IMO. The awkward bit remains converting from a FunctionInput / FunctionOutput to an ExprorDataFlow::Node. Is there a reason we don't have predicates to do this in FunctionInputsAndOutputs.qll?
 

Copy link  

Contributor  

MathiasVP   Oct 1, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

AFAIK there is no reason why we don't have this logic centralized in FunctionInputsAndOutputs.qll, no. I'll open an issue for it.
 

cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql   Outdated   Show resolved  Hide resolved  









geoffw0   added 2commits  September 17, 2021 16:04











Merge branch 'main' into cwe139  






24668b2  













C++: Accept subpaths in tests.  






e7c82d7  











geoffw0  force-pushed  the   cwe139   branch  from  34e52ebtoe7c82d7  Compare   September 17, 2021 15:54 







geoffw0   added 2commits  September 24, 2021 15:12











C++: Naive translation to use RemoteFlow*Function.  






9f59bc8  













C++: Add and use getRemoteSocket predicates.  






6901d9d  

Copy link  

Contributor   Author  

geoffw0   commented  Sep 24, 2021  

As discussed elsewhere, the plan is to merge this after the feature freeze (Monday).

jbj   reviewed   Sep 27, 2021  

View reviewed changes  





cpp/ql/src/Security/CWE/CWE-311/CleartextStorage.inc.qhelp   Outdated   Show resolved  Hide resolved  


cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql  


Comment on lines   +100  to +104  

		// a zero socket descriptor is standard input, which is not interesting for this query.
		not exists(Zero zero |
		DataFlow::localFlow(DataFlow::exprNode(zero),
		DataFlow::exprNode(transmission.getSocketExpr()))
		)

Copy link  

Contributor  

jbj   Sep 27, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Isn't the same problem present on the transmission side? Writing to stdout and stderr would be file descriptors 1 and 2, respectively.
 

Copy link  

Contributor   Author  

geoffw0   Sep 28, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Yes absolutely. Addressing this is on my list of things to do in the CWE-319 ticket, but I don't think there's an existing library class to replace Zero with that will just do it. There is some configurability in Nullness.qll (that defines Zero), but it might be better to use GVN or dataflow instead.
 

Copy link  

Contributor  

MathiasVP   Sep 28, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

I think it would be fine to exclude any constant-valued socket expression. The ones we care about here are always created from calling socket or similar functions, right?
 

Copy link  

Contributor   Author  

geoffw0   Sep 28, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Yes, I think that's sensible. Not only does it avoid a list of cases, any constant we're unaware of is likely to be something special we shouldn't make assumptions about.
 

👍 2 MathiasVP and jbj reacted with thumbs up emoji

cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql  

		*
		* note: functions such as `write` may be writing to a network source or a file. We could attempt to determine which, and sort results into `cpp/cleartext-transmission` and perhaps `cpp/cleartext-storage-file`. In practice it usually isn't very important which query reports a result as long as its reported exactly once.
		*/
		class NetworkSend extends NetworkSendRecv {

Copy link  

Contributor  

jbj   Sep 27, 2021  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

One of the reasons we generally prefer modelling Function rather than Call is that a Function model also applies directly to IR dataflow, whereas a Call model has to be translated. Of course, a Call model can have more details. The hybrid you've arrived at here LGTM.
 

Copy link  

Contributor  

jbj   commented  Sep 27, 2021  

I really like the LGTM results for this query even though there are a few FPs. I'm guessing most projects don't realize they've vendored code that will do unencrypted auth to an HTTP server or a proxy server. Maybe that seemed fine 10 years ago.

👍 1 geoffw0 reacted with thumbs up emoji

geoffw0   and others  added 3commits  September 28, 2021 15:13











" data-pjax="true" class="Link--secondary markdown-title" href="/web/20250502085923/https://github.com/github/codeql/pull/6713/commits/10323ac819f930b3503f0899cf666e1798ceb5ec">Update cpp/ql/src/Security/CWE/CWE-311/CleartextStorage.inc.qhelp   …  






10323ac  

Co-authored-by: Jonas Jensen <jbj@github.com>

C++: Correct comment.  






89098f5  













C++: Autoformat.  






679b0f9  

Copy link  

Contributor   Author  

geoffw0   commented  Oct 1, 2021  

Fixed autoformat. There are a couple of things that came up in discussions to be addressed as follow-up, but I believe this PR itself is ready to merge.

MathiasVP   approved these changes   Oct 1, 2021  

View reviewed changes  

Copy link  

Contributor  

MathiasVP   left a comment  

There was a problem hiding this comment.
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.  

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

LGTM! (assuming it passes CI)
 

MathiasVP  merged commit a3cf721  into   github:main   Oct 1, 2021  

Sign up for free  to join this conversation on GitHub.  Already have an account?  Sign in to comment  

Reviewers

jbj jbj left review comments

rdmarsh2 rdmarsh2 left review comments

MathiasVP MathiasVP approved these changes

Assignees

No one assigned

Labels  

C++   documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants  








Add this suggestion to a batch that can be applied as a single commit.  This suggestion is invalid because no changes were made to the code.  Suggestions cannot be applied while the pull request is closed.  Suggestions cannot be applied while viewing a subset of changes.  Only one suggestion per line can be applied in a batch.  Add this suggestion to a batch that can be applied as a single commit.  Applying suggestions on deleted lines is not supported.  You must change the existing code in this line in order to create a valid suggestion.  Outdated suggestions cannot be applied.  This suggestion has been applied or marked resolved.  Suggestions cannot be applied from pending reviews.  Suggestions cannot be applied on multi-line comments.  Suggestions cannot be applied while the pull request is queued to merge.  Suggestion cannot be applied right now. Please check back later.  

Footer

© 2025 GitHub, Inc.  

Footer navigation

● Terms  

● Privacy  

● Security  

● Status  

● Docs  

● Contact  

● Manage cookies  

● Do not share my personal information  


You can’t perform that action at this time.