Skip to content
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

Go: Adds sources and sinks to go/clear-text-logging #15268

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Go: Adds sources and sinks to go/clear-text-logging #15268

merged 4 commits into from
Jan 10, 2024

Conversation

atorralba
Copy link
Contributor

@atorralba atorralba commented Jan 9, 2024 •
edited
Loading

Handles log.Output as a logging sink, and secretkey (case insensitive) as a potentially sensitive variable name. It also adds a barrier-in for sources to go/clear-text-logging, so that overlapping paths are removed.

This adds coverage for CVE-2023-46742.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024 •
edited
Loading

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. A recent commit removed the previously reported differences.

@atorralba atorralba force-pushed the atorralba/go/cleartext-logging-src-and-sink-improvs branch from 837a6bb to e587ca7 Compare January 9, 2024 16:42
owen-mc
owen-mc reviewed Jan 10, 2024
View reviewed changes
go/ql/lib/semmle/go/frameworks/stdlib/Log.qll Outdated
@@ -8,7 +8,7 @@ import go
module Log {
private class LogFunction extends Function {
LogFunction() {
exists(string fn | fn.matches(["Fatal%", "Panic%", "Print%"]) |
exists(string fn | fn.matches(["Fatal%", "Panic%", "Print%", "Output"]) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that, because of this line, any argument is considered a message component (and hence a sink to two queries), including calldepth int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I had to follow a slightly more complicated approach and introduce a new class instead of modifying the existing one to take this into account. See cf94554.

Copy link
Contributor

Choose a reason for hiding this comment

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

One downside of the way you've done it is that there are now lots more instances of the LogCall class. An approach we've used elsewhere is to add a field containing the index of the first message component, and then getAMessageComponent by getSyntacticArgument(i) where i >= this index. This code is a good example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. What about now?

owen-mc
owen-mc reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Ideally you would update the tests to cover the new source, new sink and new barrier

@atorralba atorralba force-pushed the atorralba/go/cleartext-logging-src-and-sink-improvs branch from e587ca7 to cf94554 Compare January 10, 2024 11:32
@atorralba
Copy link
Contributor Author

I added a bunch of missing tests, not only for my new sink. The new barrier is hard to test because it only affects sources, but the test expectations only care about sinks currently (although we lose a few edges in certain test cases, so that may count as a test?). Also, it felt slightly overkill to test all the possible variations of SensitiveActions::HeuristicNames::maybePassword since that apparently isn't tested anywhere else. And just adding a test for the new secretKey without all the rest seemed off as well, but I can still do it if you want.

atorralba and others added 3 commits January 10, 2024 13:33
@atorralba atorralba force-pushed the atorralba/go/cleartext-logging-src-and-sink-improvs branch from cf94554 to ea21829 Compare January 10, 2024 12:33
@atorralba atorralba marked this pull request as ready for review January 10, 2024 12:34
@atorralba atorralba requested a review from a team as a code owner January 10, 2024 12:34
owen-mc
owen-mc requested changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

I was actually thinking you could put Output as part of LogFunction and have firstPrintedArg be a field on that, with a getter I suppose, and use the getter in LogCall. Then you don't need a new class just for Output.

go/ql/lib/semmle/go/frameworks/stdlib/Log.qll Outdated
@@ -16,16 +16,32 @@ module Log {
}
}

private class LogOutput extends Method {
LogOutput() { this.hasQualifiedName("log", "Logger", "Output") }
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also model (and test) the top-level version.

@atorralba atorralba force-pushed the atorralba/go/cleartext-logging-src-and-sink-improvs branch from ea21829 to 5e8c63c Compare January 10, 2024 13:12
@atorralba
Copy link
Contributor Author

I was actually thinking you could put Output as part of LogFunction and have firstPrintedArg be a field on that, with a getter I suppose, and use the getter in LogCall. Then you don't need a new class just for Output.

It seemed weird to reference an *Arg in a Function, where we should be talking about parameters instead, but I guess that is more in line with the other example you gave. Also I completely missed the top level of Output, so I thought it would be too much work to create a base abstract class that LogFunction and LogOutput would need to extend so that they shared the getter. But since there's a top level as well it's easier to combine them into one class.

Anyway, done in 5e8c63c.

owen-mc
owen-mc approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for improving the tests.

@atorralba atorralba merged commit 52d3e3d into github:main Jan 10, 2024
10 checks passed
@atorralba atorralba deleted the atorralba/go/cleartext-logging-src-and-sink-improvs branch January 10, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owen-mc owen-mc owen-mc approved these changes

Assignees
No one assigned
Labels
documentation Go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@atorralba @owen-mc