Skip to content

C++: Port PrivateData.qll from C# and use it in cpp/cleartext-transmission#8580

Merged
geoffw0 merged 13 commits intogithub:mainfrom
geoffw0:privdata
Mar 31, 2022
Merged

C++: Port PrivateData.qll from C# and use it in cpp/cleartext-transmission#8580
geoffw0 merged 13 commits intogithub:mainfrom
geoffw0:privdata

Conversation

@geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Mar 28, 2022 •
edited
Loading

Port PrivateData.qll from C#.

  • the C# version was chosen as a base, rather than the version in experimental as had been originally planned. They appear to be extremely similar, but the C# version is more complete.
  • the experimental version has been deprecated.
  • additional enhancements / tuning has been done to maximize results with minimal FPs.

Use PrivateData.qll in cpp/cleartext-transmission.

  • on LGTM we find a small number of new results, generally I think they're good, a few are weak or FP results.
  • cpp/cleartext-storage-buffer, cpp/cleartext-storage-file and cpp/cleartext-storage-database might also benefit, but I need to further investigate whether results are good for these queries.

@geoffw0 geoffw0 added the C++ label Mar 28, 2022
@geoffw0 geoffw0 requested a review from a team as a code owner March 28, 2022 14:19
MathiasVP
MathiasVP reviewed Mar 28, 2022
View reviewed changes
@rdmarsh2
Copy link
Contributor

rdmarsh2 commented Mar 28, 2022

the experimental version has been deprecated.

Our deprecation policy doesn't apply to the experimental directory, so I think we can just delete this. The original version was contributed by @dilanbhalla from Microsoft, but I believe it's only used by the queries contributed in the same PR and not by any internal queries they have.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Mar 28, 2022

Our deprecation policy doesn't apply to the experimental directory, so I think we can just delete this.

We certainly can if we want to.

The original version was contributed by @dilanbhalla from Microsoft, but I believe it's only used by the queries contributed in the same PR and not by any internal queries they have.

This PR updates cpp/private-cleartext-write (via intermediate PrivateCleartextWrite.qll) to use the new library. If that's the only use of the experimental library, I agree we can delete rather than deprecate it.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Mar 29, 2022

I've just pushed some changes:

  • updated the approach from using match to regexpMatch, which allows slightly more accurate expressions and thus slightly more accurate results (at least on tests). A repeat LGTM run appears to have identical results. Local testing suggests performance hasn't been affected much.
  • I've added a couple more test cases around the ones for salary, I wasn't happy with them as they weren't really exercising the existing code properly.
  • I've added the CWE-359 tag to this query, as it now covers many of the types of information described in that CWE.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Mar 29, 2022

... and I've deleted the experimental PrivateData.qll now as well.

MathiasVP
MathiasVP approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@geoffw0
Copy link
Contributor Author

geoffw0 commented Mar 31, 2022

Ah, thanks for doing a DCA run. I was distracted yesterday and didn't get around to it.

  • there's a lost query result; it isn't from a query affected by PrivateData.qll or related changes here, so I think its wobble.
  • there are new extraction errors in microsoft__calculator; this PR contains no extractor changes and this project commonly wobbles, so again I think no cause for concern.
  • overall analysis time is almost unchanged.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Mar 31, 2022

Merging...

@geoffw0 geoffw0 merged commit 146318d into github:main Mar 31, 2022
@MathiasVP
Copy link
Contributor

MathiasVP commented Mar 31, 2022

Ah, thanks for doing a DCA run. I was distracted yesterday and didn't get around to it.

No worries. It was really for testing something completely different 😂. But I'm glad you found it useful!

This was referenced Mar 31, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MathiasVP 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.

3 participants

@geoffw0 @rdmarsh2 @MathiasVP