JS: change alert messages of path queries to use the same template #10286

erik-krogh · 2022-09-02T19:25:34Z

I just did this for Python: #10252, now I'm doing the same thing for JavaScript.

I'm rewriting alert messages for the taint tracking queries:

I tried to fit the message to the template: This path depends on a user-provided value.
If that didn't work I tried User-provided value flows to this location and is used in a path.

I also generally tried to use "this location" instead of "here".
I tried to formulate messages to use "depends on" and use the term "a user-provided value".

The previous change-note for changing the alert messages still cover this PR.

martin389 · 2022-09-06T04:44:05Z

Hello from this week's docs first responder 👋 Thanks for creating this PR! I've added it to our review board for another writer to look at. They'll be able to advise on any additional considerations for this change.

…0314

felicitymay

@erik-krogh - many thanks for continuing to improve the consistency and clarity of our alert comments ✨

As you work on this, it would be great if you could update the Style guide with the patterns you've implemented for common situations.

This generally looks good to me, as far as I can interpret the statements. I've added a few questions and comments for your consideration.

felicitymay · 2022-09-12T08:51:34Z

javascript/ql/src/Security/CWE-078/CommandInjection.ql

@@ -28,5 +28,5 @@ where
    else highlight = sink.getNode()
  ) and
  sourceNode = source.getNode()
-select highlight, source, sink, "$@ flows to here and is used in a command.", source.getNode(),
-  sourceNode.getSourceType()
+select highlight, source, sink, "This command line depends on $@.", source.getNode(),


I notice that for this query you include "This" but that this is omitted in TemplateObjectInjection.ql. Do you have a rule of thumb for deciding when to include "This" and when to omit it.

I'm wondering if we should update https://github.com/github/codeql/blob/main/docs/query-metadata-style-guide.md#alert-messages, based on your work on select statements.

I notice that for this query you include "This" but that this is omitted in TemplateObjectInjection.ql. Do you have a rule of thumb for deciding when to include "This" and when to omit it.

I don't have a rule of thumb, I think it was a coincidence.
I think I prefer the more concise version without "This", so I'll remove the "This" from CommandInjection.ql.

I'm wondering if we should update https://github.com/github/codeql/blob/main/docs/query-metadata-style-guide.md#alert-messages, based on your work on select statements.

We should! I'll get on that when I rewrite the alert-messages for the next language (after merging this PR).

felicitymay · 2022-09-12T08:55:48Z

javascript/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql

+select sinkNode.getAlertLocation(), source, sink, "$@ which depends on $@ is later used in $@.",
  sinkNode.getAlertLocation(), sinkNode.getSinkType(), source.getNode(), "library input",
  sinkNode.getCommandExecution(), "shell command"


Maybe this should contain an article, similar to other select statements? This suggestion is something of a guess.

Suggested change

select sinkNode.getAlertLocation(), source, sink, "$@ which depends on $@ is later used in $@.",

sinkNode.getAlertLocation(), sinkNode.getSinkType(), source.getNode(), "library input",

sinkNode.getCommandExecution(), "shell command"

select sinkNode.getAlertLocation(), source, sink, "$@ which depends on $@ is later used in $@.",

sinkNode.getAlertLocation(), sinkNode.getSinkType(), source.getNode(), "library input",

sinkNode.getCommandExecution(), "a shell command"

felicitymay · 2022-09-12T08:58:56Z

javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql

@@ -18,6 +18,6 @@ import semmle.javascript.security.dataflow.UnsafeHtmlConstructionQuery

 from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
 where cfg.hasFlowPath(source, sink) and sink.getNode() = sinkNode
-select sinkNode, source, sink, "$@ based on $@ might later cause $@.", sinkNode,
+select sinkNode, source, sink, "$@ which depends on $@ might later cause $@.", sinkNode,


Does this actually cause cross-site scripting, or might it be vulnerable to cross-site scripting?

I see that UnsafeDynamicMethodAccess.ql below uses: "may lead to remote code execution".

The query detects something that appears unsafe, but that's not a full exploitable vulnerability.
The full vulnerability requires that the library (flagged by the query) is used with unsafe inputs in some other project.

So that's why there is a "might" in the alert message.

I'd suggest replacing "cause" since the code doesn't cause cross-site scripting, it seems more as if it "allows" a hacker to perform cross-site scripting.

Suggested change

select sinkNode, source, sink, "$@ which depends on $@ might later cause $@.", sinkNode,

select sinkNode, source, sink, "$@ which depends on $@ might later allow $@.", sinkNode,

felicitymay · 2022-09-12T09:45:21Z

javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql

@@ -18,4 +18,4 @@ from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)
 select sink, source, sink,
  "Invocation of method derived from $@ may lead to remote code execution.", source.getNode(),


Should this be more like:

Suggested change

"Invocation of method derived from $@ may lead to remote code execution.", source.getNode(),

"This method is invoked using $@, which may allow remote code execution.", source.getNode(),

javascript/ql/src/Security/CWE-312/CleartextLogging.ql

felicitymay · 2022-09-12T09:51:29Z

javascript/ql/src/Security/CWE-400/RemotePropertyInjection.ql

@@ -18,5 +18,5 @@ import DataFlow::PathGraph

 from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "A $@ is used as" + sink.getNode().(Sink).getMessage(),
+select sink.getNode(), source, sink, sink.getNode().(Sink).getMessage() + " depends on $@.",
  source.getNode(), "user-provided value"


Suggested change

source.getNode(), "user-provided value"

source.getNode(), "a user-provided value"

felicitymay · 2022-09-12T09:55:40Z

javascript/ql/src/Security/CWE-834/LoopBoundInjection.ql

  "Iterating over user-controlled object with a potentially unbounded .length property from $@.",
-  source, "here"
+  source, "a user-provided value"


Maybe more like this (it won't let me add a suggestion).

"Iteration over a user-controlled object with a potentially unbounded .length property from $@.", source, "a user-provided value"

felicitymay

Thanks for the updates. Happy for this to be merged once it's okayed by the JavaScript team.

github-actions bot added the JS label Sep 2, 2022

erik-krogh force-pushed the js-followMsg branch from 8450042 to e3bd767 Compare September 2, 2022 20:51

make the alert messages of taint-tracking queries more consistent

aa56ca3

erik-krogh force-pushed the js-followMsg branch from e3bd767 to aa56ca3 Compare September 5, 2022 12:05

erik-krogh added the no-change-note-required This PR does not need a change note label Sep 5, 2022

erik-krogh marked this pull request as ready for review September 5, 2022 12:22

erik-krogh requested a review from a team as a code owner September 5, 2022 12:22

erik-krogh added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Sep 5, 2022

erik-krogh mentioned this pull request Sep 5, 2022

RB: make the alert messages of taint-tracking queries more consistent #10304

Merged

erik-krogh added 2 commits September 6, 2022 12:25

update alert-messages of the sensitive data queries to match github#1…

24f2e3c

…0314

Merge branch 'main' into js-followMsg

a35fe1f

felicitymay previously approved these changes Sep 12, 2022

View reviewed changes

apply suggestions from doc review

6ec03d4

erik-krogh dismissed felicitymay’s stale review via 6ec03d4 September 12, 2022 11:16

Merge branch 'main' into js-followMsg

afcb767

calumgrant requested a review from asgerf September 12, 2022 12:45

apply another suggestion from doc review

87fb01d

felicitymay approved these changes Sep 12, 2022

View reviewed changes

erik-krogh removed the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Sep 12, 2022

This was referenced Sep 13, 2022

update the style guide on alert-messages #10405

Merged

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

Merged

asgerf approved these changes Sep 20, 2022

View reviewed changes

erik-krogh merged commit 0720fa7 into github:main Sep 20, 2022

This was referenced Sep 22, 2022

CPP: Make more alert-messages follow the style guide #10507

Merged

C#: Update the alert messages to better follow the style guide #10557

Merged

Java: Update the alert messages to better follow the style guide #10528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: change alert messages of path queries to use the same template #10286

JS: change alert messages of path queries to use the same template #10286

erik-krogh commented Sep 2, 2022 •

edited

Loading

martin389 commented Sep 6, 2022

felicitymay left a comment

felicitymay Sep 12, 2022

erik-krogh Sep 12, 2022

felicitymay Sep 12, 2022

felicitymay Sep 12, 2022

erik-krogh Sep 12, 2022

felicitymay Sep 12, 2022

felicitymay Sep 12, 2022

felicitymay Sep 12, 2022

felicitymay Sep 12, 2022

felicitymay left a comment

	select sinkNode, source, sink, "$@ which depends on $@ might later cause $@.", sinkNode,
	select sinkNode, source, sink, "$@ which depends on $@ might later allow $@.", sinkNode,

	"Invocation of method derived from $@ may lead to remote code execution.", source.getNode(),
	"This method is invoked using $@, which may allow remote code execution.", source.getNode(),

	source.getNode(), "user-provided value"
	source.getNode(), "a user-provided value"

JS: change alert messages of path queries to use the same template #10286

JS: change alert messages of path queries to use the same template #10286

Conversation

erik-krogh commented Sep 2, 2022 • edited Loading

martin389 commented Sep 6, 2022

felicitymay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felicitymay left a comment

Choose a reason for hiding this comment

erik-krogh commented Sep 2, 2022 •

edited

Loading