Skip to content

GO: make the alert messages of taint-tracking queries more consistent #10413

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

Merged
merged 7 commits into from
Sep 21, 2022
Merged

GO: make the alert messages of taint-tracking queries more consistent #10413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2a41cf
fix most ql/alert-message-style-violation
erik-krogh Sep 13, 2022
3cf5516
make the alert messages of taint-tracking queries more consistent
erik-krogh Sep 14, 2022
2602a38
update expected test output
erik-krogh Sep 14, 2022
c241185
avoid more instances of "this location" and "here" in alert-messages
erik-krogh Sep 20, 2022
1be1496
use "depends to" for a taint-tracking query
erik-krogh Sep 20, 2022
83bedc0
be more specific about what the source is in `go/suspicious-character…
erik-krogh Sep 20, 2022
175d3ac
reword alert-message `go/user-controlled-bypass` to avoid using "here"
erik-krogh Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions go/ql/src/InconsistentCode/ConstantLengthComparison.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,5 @@ where
cond.dominates(idx.getBasicBlock()) and
// and that check happens inside the loop body
cond.getCondition().getParent+() = fs
select cond.getCondition(),
"This checks the length against a constant, but it is indexed using a variable $@.", idx, "here"
select cond.getCondition(), "This checks the length against a constant, but it $@.", idx,
"is indexed using a variable"
4 changes: 2 additions & 2 deletions go/ql/src/InconsistentCode/MissingErrorCheck.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,5 +116,5 @@ where
// `deref` dereferences `ptr`
deref.getOperand() = ptr.getAUse()
select deref.getOperand(),
ptr.getSourceVariable() + " may be nil here, because $@ may not have been checked.", err,
err.getSourceVariable().toString()
"$@ may be nil at this dereference because $@ may not have been checked.", ptr,
ptr.getSourceVariable().toString(), err, err.getSourceVariable().toString()
2 changes: 1 addition & 1 deletion go/ql/src/RedundantCode/CompareIdenticalValues.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ where
cmp.getAnOperand() = decl.getAReference() and
cmp.getAnOperand() instanceof BasicLit
)
select cmp, "This expression compares $@ to itself.", cmp.getLeftOperand(), "an expression"
select cmp, "This expression compares an $@ to itself.", cmp.getLeftOperand(), "expression"
2 changes: 1 addition & 1 deletion go/ql/src/RedundantCode/DuplicateCondition.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,4 @@ GVN conditionGvn(IfStmt is, int i, Expr e) {

from IfStmt is, Expr e, Expr f, int i, int j
where conditionGvn(is, i, e) = conditionGvn(is, j, f) and i < j
select f, "This condition is a duplicate of $@.", e, "an earlier condition"
select f, "This condition is a duplicate of an $@.", e, "earlier condition"
2 changes: 1 addition & 1 deletion go/ql/src/RedundantCode/DuplicateSwitchCase.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ GVN switchCaseGvn(SwitchStmt switch, int i, Expr e) {

from SwitchStmt switch, int i, Expr e, int j, Expr f
where switchCaseGvn(switch, i, e) = switchCaseGvn(switch, j, f) and i < j
select f, "This case is a duplicate of $@.", e, "an earlier case"
select f, "This case is a duplicate of an $@.", e, "earlier case"
2 changes: 1 addition & 1 deletion go/ql/src/RedundantCode/SelfAssignment.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ from PotentialSelfAssignment assgn, HashableNode rhs
where
rhs = assgn.getRhs() and
rhs.hash() = assgn.getLhs().(HashableNode).hash()
select assgn, "This statement assigns $@ to itself.", rhs, "an expression"
select assgn, "This statement assigns an $@ to itself.", rhs, "expression"
2 changes: 1 addition & 1 deletion go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,4 @@ from Config c, DataFlow::PathNode source, DataFlow::PathNode sink, string hostPa
where c.hasFlowPath(source, sink) and c.isSource(source.getNode(), hostPart)
select source, source, sink,
"This regular expression has an unescaped dot before '" + hostPart + "', " +
"so it might match more hosts than expected when used $@.", sink, "here"
"so it might match more hosts than expected when $@.", sink, "the regular expression is used"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-020/SuspiciousCharacterInRegexp.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,5 @@ class Config extends DataFlow::Configuration {

from Config c, DataFlow::PathNode source, DataFlow::PathNode sink, string report
where c.hasFlowPath(source, sink) and c.isSource(source.getNode(), report)
select source, source, sink, "$@ used $@ contains " + report, source, "A regular expression", sink,
"here"
select source, source, sink, "$@ that is $@ contains " + report, source, "A string literal", sink,
"used as a regular expression"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-022/TaintedPath.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ import DataFlow::PathGraph

from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "$@ flows to here and is used in a path.", source.getNode(),
"User-provided value"
select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
"user-provided value"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-078/CommandInjection.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ from
CommandInjection::Configuration cfg, CommandInjection::DoubleDashSanitizingConfiguration cfg2,
DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink) or cfg2.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "This command depends on $@.", source.getNode(),
"a user-provided value"
select sink.getNode(), source, sink, "This command depends on a $@.", source.getNode(),
"user-provided value"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-078/StoredCommand.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ import DataFlow::PathGraph

from StoredCommand::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "This command depends on $@.", source.getNode(),
"a stored value"
select sink.getNode(), source, sink, "This command depends on a $@.", source.getNode(),
"stored value"
5 changes: 2 additions & 3 deletions go/ql/src/Security/CWE-079/ReflectedXss.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@ where
(
exists(string kind | kind = sink.getNode().(SharedXss::Sink).getSinkKind() |
kind = "rawtemplate" and
msg =
"Cross-site scripting vulnerability due to $@. This template argument is instantiated raw $@." and
part = "here"
msg = "Cross-site scripting vulnerability due to $@. The value is $@." and
part = "instantiated as a raw template"
)
or
not exists(sink.getNode().(SharedXss::Sink).getSinkKind()) and
Expand Down
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-089/SqlInjection.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ import DataFlow::PathGraph

from SqlInjection::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "This query depends on $@.", source.getNode(),
"a user-provided value"
select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(),
"user-provided value"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-117/LogInjection.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ import DataFlow::PathGraph

from LogInjection::Configuration c, DataFlow::PathNode source, DataFlow::PathNode sink
where c.hasFlowPath(source, sink)
select sink, source, sink, "This log write receives unsanitized user input from $@.",
source.getNode(), "here"
select sink.getNode(), source, sink, "Log entry depends on a $@.", source.getNode(),
"user-provided value"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-190/AllocationSizeOverflow.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ where
cfg.hasFlowPath(source, sink) and
cfg.isSink(sink.getNode(), allocsz)
select sink, source, sink,
"This operation, which is used in an $@, involves a potentially large $@ and might overflow.",
allocsz, "allocation", source, "value"
"This operation, which is used in an $@, involves a $@ and might overflow.", allocsz,
"allocation", source, "potentially large value"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-209/StackTraceExposure.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,5 +77,5 @@ class StackTraceExposureConfig extends TaintTracking::Configuration {
from StackTraceExposureConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink,
"Stack trace information from $@ may be exposed to an external user here.", source.getNode(),
"here"
"HTTP response depends on $@ and may be exposed to an external user.", source.getNode(),
"stack trace information"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-312/CleartextLogging.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ import DataFlow::PathGraph

from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Sensitive data returned by $@ is logged here.",
source.getNode(), source.getNode().(Source).describe()
select sink.getNode(), source, sink, "$@ flows to a logging call.", source.getNode(),
"Sensitive data returned by " + source.getNode().(Source).describe()
2 changes: 1 addition & 1 deletion go/ql/src/Security/CWE-601/OpenUrlRedirect.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@ where
// this excludes flow from safe parts of request URLs, for example the full URL when the
// doing a redirect from `http://<path>` to `https://<path>`
not scfg.hasFlow(_, sink.getNode())
select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(),
select sink.getNode(), source, sink, "Untrusted URL redirection depends on a $@.", source.getNode(),

Check warning

Code scanning / CodeQL

Alert message style violation

Use "flows to" instead of "depends on" in data flow queries.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A false positive. It's a data-flow query, but the configuration includes basically all the taint-tracking steps.

"user-provided value"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-643/XPathInjection.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@ predicate isStringOrByte(DataFlow::PathNode node) {

from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink) and isStringOrByte(sink)
select sink.getNode(), source, sink, "$@ flows to here and is used in an XPath expression.",
source.getNode(), "User-provided value"
select sink.getNode(), source, sink, "XPath expression depends on a $@.", source.getNode(),
"user-provided value"
4 changes: 2 additions & 2 deletions go/ql/src/Security/CWE-918/RequestForgery.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ where
request = sink.getNode().(Sink).getARequest() and
// this excludes flow from safe parts of request URLs, for example the full URL
not scfg.hasFlow(_, sink.getNode())
select request, source, sink, "The $@ of this request depends on $@.", sink.getNode(),
sink.getNode().(Sink).getKind(), source, "a user-provided value"
select request, source, sink, "The $@ of this request depends on a $@.", sink.getNode(),
sink.getNode().(Sink).getKind(), source, "user-provided value"
4 changes: 2 additions & 2 deletions go/ql/src/experimental/CWE-090/LDAPInjection.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ import DataFlow::PathGraph

from LdapInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "LDAP query parameter is derived from $@.", source.getNode(),
"a user-provided value"
select sink.getNode(), source, sink, "LDAP query parameter depends on a $@.", source.getNode(),
"user-provided value"
5 changes: 2 additions & 3 deletions go/ql/src/experimental/CWE-840/ConditionalBypass.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,5 @@ where
rhs.getNode().asExpr() = c.getRightOperand() and
config.hasFlowPath(lhsSource, lhs) and
lhs.getNode().asExpr() = c.getLeftOperand()
select c,
"This comparison compares user-controlled values from $@ and $@, and hence can be bypassed.",
lhsSource, "here", rhsSource, "here"
select c, "This comparison of a $@ with another $@ can be bypassed by a malicious user.",
lhsSource.getNode(), "user-controlled value", rhsSource.getNode(), "user-controlled value"
22 changes: 11 additions & 11 deletions go/ql/test/experimental/CWE-090/LDAPInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ nodes
| LDAPInjection.go:81:25:81:33 | untrusted | semmle.label | untrusted |
subpaths
#select
| LDAPInjection.go:59:3:59:11 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:59:3:59:11 | untrusted | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:61:3:61:51 | ...+... | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:61:3:61:51 | ...+... | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:62:3:62:33 | slice literal | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:62:3:62:33 | slice literal | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:66:3:66:11 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:66:3:66:11 | untrusted | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:68:3:68:51 | ...+... | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:68:3:68:51 | ...+... | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:69:3:69:33 | slice literal | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:69:3:69:33 | slice literal | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:73:3:73:11 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:73:3:73:11 | untrusted | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:75:3:75:51 | ...+... | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:75:3:75:51 | ...+... | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:76:3:76:33 | slice literal | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:76:3:76:33 | slice literal | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:80:22:80:30 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:80:22:80:30 | untrusted | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:81:25:81:33 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:81:25:81:33 | untrusted | LDAP query parameter is derived from $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | a user-provided value |
| LDAPInjection.go:59:3:59:11 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:59:3:59:11 | untrusted | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:61:3:61:51 | ...+... | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:61:3:61:51 | ...+... | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:62:3:62:33 | slice literal | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:62:3:62:33 | slice literal | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:66:3:66:11 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:66:3:66:11 | untrusted | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:68:3:68:51 | ...+... | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:68:3:68:51 | ...+... | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:69:3:69:33 | slice literal | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:69:3:69:33 | slice literal | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:73:3:73:11 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:73:3:73:11 | untrusted | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:75:3:75:51 | ...+... | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:75:3:75:51 | ...+... | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:76:3:76:33 | slice literal | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:76:3:76:33 | slice literal | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:80:22:80:30 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:80:22:80:30 | untrusted | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
| LDAPInjection.go:81:25:81:33 | untrusted | LDAPInjection.go:57:15:57:29 | call to UserAgent : string | LDAPInjection.go:81:25:81:33 | untrusted | LDAP query parameter depends on a $@. | LDAPInjection.go:57:15:57:29 | call to UserAgent | user-provided value |
6 changes: 3 additions & 3 deletions go/ql/test/experimental/CWE-840/ConditionalBypass.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
| ConditionalBypassBad.go:9:5:9:46 | ...!=... | This comparison compares user-controlled values from $@ and $@, and hence can be bypassed. | ConditionalBypassBad.go:9:5:9:12 | selection of Header : Header | here | ConditionalBypassBad.go:9:41:9:46 | selection of Host : string | here |
| condition.go:9:5:9:46 | ...!=... | This comparison compares user-controlled values from $@ and $@, and hence can be bypassed. | condition.go:9:5:9:12 | selection of Header : Header | here | condition.go:9:41:9:46 | selection of Host : string | here |
| condition.go:16:5:16:62 | ...!=... | This comparison compares user-controlled values from $@ and $@, and hence can be bypassed. | condition.go:16:5:16:12 | selection of Header : Header | here | condition.go:16:41:16:48 | selection of Header : Header | here |
| ConditionalBypassBad.go:9:5:9:46 | ...!=... | This comparison of a $@ with another $@ can be bypassed by a malicious user. | ConditionalBypassBad.go:9:5:9:12 | selection of Header | user-controlled value | ConditionalBypassBad.go:9:41:9:46 | selection of Host | user-controlled value |
| condition.go:9:5:9:46 | ...!=... | This comparison of a $@ with another $@ can be bypassed by a malicious user. | condition.go:9:5:9:12 | selection of Header | user-controlled value | condition.go:9:41:9:46 | selection of Host | user-controlled value |
| condition.go:16:5:16:62 | ...!=... | This comparison of a $@ with another $@ can be bypassed by a malicious user. | condition.go:16:5:16:12 | selection of Header | user-controlled value | condition.go:16:41:16:48 | selection of Header | user-controlled value |
Loading