C#: Update the alert messages to better follow the style guide #10557
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.
erik-krogh
force-pushed
the
csharp-followMsg
branch
from
8d72ef7 to
ada1c28
Compare
There was a problem hiding this comment.
Found 14 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.
erik-krogh
force-pushed
the
csharp-followMsg
branch
from
8a950ee to
97a3734
Compare
There was a problem hiding this comment.
Found 13 potential problems in the proposed changes. Check the Files changed tab for more details.
| @@ -15,5 +15,5 @@ | |||
|
|
|||
| from RequestForgeryConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink | |||
| where c.hasFlowPath(source, sink) | |||
| select sink.getNode(), source, sink, "$@ flows to here and is used in a server side web request.", | |||
| source.getNode(), "User-provided value" | |||
| select sink.getNode(), source, sink, "The URL of this request depends on a $@.", source.getNode(), | |||
Check warning
Code scanning / CodeQL
Alert message style violation
There was a problem hiding this comment.
Yes, that's an FP from the QL-for-QL check.
But that because this query uses a DataFlow configuration, but the configuration also includes most of the taint-tracking steps, so the configuration is effectively a TaintTracking query.
Comment on lines
17
to
+19
| from Dereference d, Ssa::SourceVariable v | ||
| where d.isFirstAlwaysNull(v) | ||
| select d, "Variable $@ is always null here.", v, v.toString() | ||
| select d, "Variable $@ is always null at this dereference.", v, v.toString() |
Check warning
Code scanning / CodeQL
Consistent alert message
Comment on lines
78
to
115
| from Field f, FieldRead fa | ||
| where | ||
| f.fromSource() and | ||
| not extractionIsStandalone() and | ||
| not f.isReadOnly() and | ||
| not f.isConst() and | ||
| not f.getDeclaringType() instanceof Enum and | ||
| not f.getType() instanceof Struct and | ||
| not exists(Assignment ae, Field g | | ||
| ae.getLValue().(FieldAccess).getTarget() = g and | ||
| g.getUnboundDeclaration() = f and | ||
| not ae.getRValue() instanceof NullLiteral | ||
| ) and | ||
| not exists(MethodCall mc, int i, Field g | | ||
| exists(Parameter p | mc.getTarget().getParameter(i) = p | p.isOut() or p.isRef()) and | ||
| mc.getArgument(i) = g.getAnAccess() and | ||
| g.getUnboundDeclaration() = f | ||
| ) and | ||
| not isFieldExternallyInitialized(f) and | ||
| not exists(f.getAnAttribute()) and | ||
| not exists(Expr init, Field g | | ||
| g.getUnboundDeclaration() = f and | ||
| g.getInitializer() = init and | ||
| not init instanceof NullLiteral | ||
| ) and | ||
| not exists(AssignOperation ua, Field g | | ||
| ua.getLValue().(FieldAccess).getTarget() = g and | ||
| g.getUnboundDeclaration() = f | ||
| ) and | ||
| not exists(MutatorOperation op | | ||
| op.getAnOperand().(FieldAccess).getTarget().getUnboundDeclaration() = f | ||
| ) and | ||
| exists(Field g | | ||
| fa.getTarget() = g and | ||
| g.getUnboundDeclaration() = f | ||
| ) | ||
| select f, | ||
| "The field '" + f.getName() + "' is never explicitly assigned a value, yet it is read $@.", fa, | ||
| "here" | ||
| select f, "The field '" + f.getName() + "' is never explicitly assigned a value, yet $@.", fa, | ||
| "the field is read" |
Check warning
Code scanning / CodeQL
Consistent alert message
Comment on lines
18
to
21
| from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink | ||
| where c.hasFlowPath(source, sink) | ||
| select sink.getNode(), source, sink, "$@ flows to here and is used in an LDAP query.", | ||
| source.getNode(), "User-provided value" | ||
| select sink.getNode(), source, sink, "LDAP query depends on a $@.", source.getNode(), | ||
| "user-provided value" |
Check warning
Code scanning / CodeQL
Consistent alert message
Comment on lines
20
to
+23
| from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink | ||
| where config.hasFlowPath(source, sink) | ||
| select sink.getNode().(Sink).getSensitiveMethodCall(), source, sink, | ||
| "Sensitive method may not be executed depending on $@, which flows from $@.", sink.getNode(), | ||
| "this condition", source.getNode(), "user input" | ||
| select sink.getNode(), source, sink, "This condition guards a sensitive $@, but a $@ controls it.", | ||
| sink.getNode().(Sink).getSensitiveMethodCall(), "action", source.getNode(), "user-provided value" |
Check warning
Code scanning / CodeQL
Consistent alert message
Comment on lines
1
to
38
| /** | ||
| * @name Number of Solorigate-related command names in enum is above the threshold | ||
| * @description The enum contains several values that look similar to command and control command names, which may indicate that the code may have been tampered with by an external agent. | ||
| * It is recommended to review the code and verify that there is no unexpected code in this project. | ||
| * @kind problem | ||
| * @tags security | ||
| * solorigate | ||
| * @problem.severity warning | ||
| * @precision medium | ||
| * @id cs/solorigate/number-of-known-commands-in-enum-above-threshold | ||
| */ | ||
|
|
||
| import csharp | ||
| import Solorigate | ||
|
|
||
| /* | ||
| * Returns the total number of Solorigate-related commands in the given enum | ||
| * | ||
| * This command list is described at https://www.fireeye.com/blog/products-and-services/2020/12/global-intrusion-campaign-leverages-software-supply-chain-compromise.html | ||
| * and the enum names are based from https://github.com/ITAYC0HEN/SUNBURST-Cracked/tree/a01f358965525bee34ad026acd9dfda3d488fdd8 | ||
| */ | ||
|
|
||
| int countSolorigateCommandInEnum(Enum e) { | ||
| result = | ||
| count(string s, EnumConstant ec | | ||
| e.getAnEnumConstant() = ec and | ||
| s = ec.getName() and | ||
| s = solorigateSuspiciousCommandsInEnum() | ||
| ) | ||
| } | ||
|
|
||
| from Enum e, int total | ||
| where | ||
| total = countSolorigateCommandInEnum(e) and | ||
| total > 10 | ||
| select e, | ||
| "The enum $@ may be related to Solorigate. It matches " + total + | ||
| " of the values used for commands in the enum.", e, e.getName() | ||
| "The enum may be related to Solorigate. It matches " + total + | ||
| " of the values used for commands in the enum." |
Check warning
Code scanning / CodeQL
Missing security metadata
Comment on lines
1
to
33
| /** | ||
| * @name Number of Solorigate-related hashes as literals is above the threshold | ||
| * @description The total number of Solorigate-related hash literals found in the code is above a threshold, which may indicate that an external agent has tampered with the code. | ||
| * It is recommended to review the code and verify that there is no unexpected code in this project, however it is highly unlikely the hash values would be present coincidentally. | ||
| * @kind problem | ||
| * @tags security | ||
| * solorigate | ||
| * @problem.severity warning | ||
| * @precision medium | ||
| * @id cs/solorigate/number-of-known-hashes-above-threshold | ||
| */ | ||
|
|
||
| import csharp | ||
| import Solorigate | ||
|
|
||
| /* | ||
| * Returns the total number of Solorigate-related hashes found in the project | ||
| */ | ||
|
|
||
| int countSolorigateSuspiciousHash() { | ||
| result = | ||
| count(string s | exists(Literal l | s = l.getValue() and s = solorigateSuspiciousHashes())) | ||
| } | ||
|
|
||
| from Literal l, int total, int threshold | ||
| where | ||
| total = countSolorigateSuspiciousHash() and | ||
| threshold = 5 and // out of ~200 known literals | ||
| isSolorigateHash(l) and | ||
| total > threshold | ||
| select l, | ||
| "The Hash literal $@ may be related to the Solorigate campaign. Total count = " + total + | ||
| " is above the threshold " + threshold + ".", l, l.getValue() | ||
| "This Hash literal may be related to the Solorigate campaign. Total count = " + total + | ||
| " is above the threshold " + threshold + "." |
Check warning
Code scanning / CodeQL
Missing security metadata
Comment on lines
1
to
33
| /** | ||
| * @name Number of Solorigate-related literals is above the threshold | ||
| * @description The total number of Solorigate-related literals found in the code is above a threshold, which may indicate that an external agent has tampered with the code. | ||
| * It is recommended to review the code and verify that there is no unexpected code in this project. | ||
| * @kind problem | ||
| * @tags security | ||
| * solorigate | ||
| * @problem.severity warning | ||
| * @precision medium | ||
| * @id cs/solorigate/number-of-known-literals-above-threshold | ||
| */ | ||
|
|
||
| import csharp | ||
| import Solorigate | ||
|
|
||
| /* | ||
| * Returns the total number of Solorigate-related literals found in the project | ||
| */ | ||
|
|
||
| int countSolorigateSuspiciousLiterals() { | ||
| result = | ||
| count(string s | exists(Literal l | s = l.getValue() and s = solorigateSuspiciousLiterals())) | ||
| } | ||
|
|
||
| from Literal l, int total, int threshold | ||
| where | ||
| total = countSolorigateSuspiciousLiterals() and | ||
| threshold = 30 and // out of ~150 known literals | ||
| isSolorigateLiteral(l) and | ||
| total > threshold | ||
| select l, | ||
| "The literal $@ may be related to the Solorigate campaign. Total count = " + total + | ||
| " is above the threshold " + threshold + ".", l, l.getValue() | ||
| "This literal may be related to the Solorigate campaign. Total count = " + total + | ||
| " is above the threshold " + threshold + "." |
Check warning
Code scanning / CodeQL
Missing security metadata
csharp/ql/campaigns/Solorigate/src/NumberOfKnownMethodNamesAboveThreshold.ql
Fixed
Show fixed
Hide fixed
Comment on lines
1
to
31
| /** | ||
| * @name Detects a modified FNV function | ||
| * @description Possible indication of Solorigate. Detects a modified FNV1 function, where there is an additional xor using a literal after the regular FNV hash | ||
| * @kind problem | ||
| * @tags security | ||
| * solorigate | ||
| * @precision high | ||
| * @id cs/solorigate/modified-fnv-function-detection | ||
| * @problem.severity error | ||
| */ | ||
|
|
||
| import csharp | ||
| import Solorigate | ||
| import experimental.code.csharp.Cryptography.NonCryptographicHashes | ||
|
|
||
| from Variable v, Literal l, LoopStmt loop, Expr additional_xor | ||
| where | ||
| maybeUsedInFnvFunction(v, _, _, loop) and | ||
| ( | ||
| exists(BitwiseXorExpr xor2 | xor2.getAnOperand() = l and additional_xor = xor2 | | ||
| loop.getAControlFlowExitNode().getASuccessor*() = xor2.getAControlFlowNode() and | ||
| xor2.getAnOperand() = v.getAnAccess() | ||
| ) | ||
| or | ||
| exists(AssignXorExpr xor2 | xor2.getAnOperand() = l and additional_xor = xor2 | | ||
| loop.getAControlFlowExitNode().getASuccessor*() = xor2.getAControlFlowNode() and | ||
| xor2.getAnOperand() = v.getAnAccess() | ||
| ) | ||
| ) | ||
| select l, | ||
| "The variable $@ seems to be used as part of a FNV-like hash calculation, that is modified by an additional $@ expression using literal $@.", | ||
| v, v.toString(), additional_xor, "xor", l, l.toString() | ||
| select l, "This literal is used in an $@ after a FNV-like hash calculation with variable $@.", | ||
| additional_xor, "additional xor", v, v.toString() |
Check warning
Code scanning / CodeQL
Missing security metadata
erik-krogh
force-pushed
the
csharp-followMsg
branch
from
e305443 to
a771f6a
Compare
erik-krogh
force-pushed
the
csharp-followMsg
branch
from
a771f6a to
326666a
Compare
hvitved
requested changes
Sep 28, 2022
| select l, | ||
| "The variable $@ seems to be used as part of a FNV-like hash calculation, that is modified by an additional $@ expression using literal $@.", | ||
| v, v.toString(), additional_xor, "xor", l, l.toString() | ||
| select l, "This literal is used in an $@ after a FNV-like hash calculation with variable $@.", |
There was a problem hiding this comment.
May as well update to an FNV-like.
| @@ -28,5 +28,5 @@ where | |||
| isSolorigateSuspiciousMethodName(m) and | |||
| total > threshold | |||
| select m, | |||
| "The method $@ may be related to Solorigate. Total count = " + total + " is above the threshold " + | |||
| threshold + ".", m, m.getName() | |||
| "This method " + m.getName() + " may be related to Solorigate. Total count = " + total + | |||
There was a problem hiding this comment.
Remove the + m.getName() + bit?
| @@ -20,5 +20,5 @@ import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph | |||
|
|
|||
| from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink | |||
| where c.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(), | |||
There was a problem hiding this comment.
Here the message starts with "this", while it doesn't for CommandInjection.ql.
There was a problem hiding this comment.
Same for other queries; some start with "this" while others don't.
There was a problem hiding this comment.
I've gone for starting with "This " in most of the queries.
That way we are consistent about referencing the current location with "This".
| select sink.getNode(), source, sink, "$@ flows to here and is used in a command.", source.getNode(), | ||
| "Stored user-provided value" | ||
| select sink.getNode(), source, sink, "Command line depends on a $@.", source.getNode(), | ||
| "stored user-provided value" |
There was a problem hiding this comment.
actually, should be "stored value" or "stored (potentially user-provided) value".
There was a problem hiding this comment.
And same for other stored queries.
| select sink.getNode(), source, sink, "$@ flows to here and is used as a format string.", | ||
| source.getNode(), source.getNode().toString() | ||
| select sink.getNode(), source, sink, "Format string depends on a $@.", source.getNode(), | ||
| ("this" + getSourceType(source.getNode())) |
There was a problem hiding this comment.
Yep, there is an a too much.
I've removed the a such that the alert-message is similar to the one you use in cs/sql-injection.
| @@ -15,5 +15,5 @@ | |||
|
|
|||
| from RequestForgeryConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink | |||
| where c.hasFlowPath(source, sink) | |||
| select sink.getNode(), source, sink, "$@ flows to here and is used in a server side web request.", | |||
| source.getNode(), "User-provided value" | |||
| select sink.getNode(), source, sink, "The URL of this request depends on a $@.", source.getNode(), | |||
erik-krogh
changed the title
Comment on lines
1
to
32
| /** | ||
| * @name Number of Solorigate-related method names is above the threshold | ||
| * @description The total number of Solorigate-related method names found in the code is above a threshold, which may indicate that an external agent has tampered with the code. | ||
| * It is recommended to review the code and verify that there is no unexpected code in this project. | ||
| * @kind problem | ||
| * @tags security | ||
| * solorigate | ||
| * @problem.severity warning | ||
| * @precision medium | ||
| * @id cs/solorigate/number-of-known-method-names-above-threshold | ||
| */ | ||
|
|
||
| import csharp | ||
| import Solorigate | ||
|
|
||
| /* | ||
| * Returns the total number of Solorigate-related method names found in the project | ||
| */ | ||
|
|
||
| int countSolorigateSuspiciousMethodNames() { | ||
| result = count(string s | s = any(Method m | isSolorigateSuspiciousMethodName(m)).getName()) | ||
| } | ||
|
|
||
| from Method m, int total, int threshold | ||
| where | ||
| total = countSolorigateSuspiciousMethodNames() and | ||
| threshold = 50 and // out of ~ 100 known names | ||
| isSolorigateSuspiciousMethodName(m) and | ||
| total > threshold | ||
| select m, | ||
| "The method $@ may be related to Solorigate. Total count = " + total + " is above the threshold " + | ||
| threshold + ".", m, m.getName() | ||
| "This method may be related to Solorigate. Total count = " + total + " is above the threshold " + | ||
| threshold + "." |
Check warning
Code scanning / CodeQL
Missing security metadata
hvitved
previously approved these changes
Sep 30, 2022
hvitved
previously approved these changes
Sep 30, 2022
erik-krogh
force-pushed
the
csharp-followMsg
branch
from
b6e79f9 to
e2fe63f
Compare
hvitved
approved these changes
Oct 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I've been working on making the alert-messages consistent across languages, and some fan out work from that is ensuring that the alert-messages follow the style guide (which I've also revised).
Fixes done as part of this PR:
Some of the messages become inconsistent with other languages, but I'm updating one language at a time, so some will get out of sync.
Some other PRs in this series: JS, Py, Rb, Go, C, Java.
Style guide update.