Enhanced LogForgingQuery to treat C# Enums as simple types. #17866
Conversation
There was a problem hiding this comment.
Thank you very much for the contribution! This looks like a very reasonable change.
I have added some review comments - please let me know, if you have any questions or need any help.
If you have the CodeQL CLI installed on your machine you can execute the log forging tests by
codeql test run <path_to_folder_with_test> --show-extractor-output
Example
codeql test run csharp/ql/test/query-tests/Security\ Features/CWE-117/ --show-extractor-output
After the review comments have been addressed, we
- Trigger the CI (to see if there is any other impact)
- Run our queries on a larger set of repos (DCA) to see which effect it has to consider enums as simple type sanitizers.
| @@ -0,0 +1,4 @@ | |||
| --- | |||
| category: fix | |||
There was a problem hiding this comment.
| category: fix | |
| category: minorAnalysis |
There was a problem hiding this comment.
This is as such not a "fix" but rather a change in, how we perceive enums.
| TestEnumValue | ||
| } | ||
|
|
||
| public class LogForgingSimpleTypes |
There was a problem hiding this comment.
The test code doesn't compile. Consider adding the test code in https://github.com/github/codeql/blob/main/csharp/ql/test/query-tests/Security%20Features/CWE-117/LogForging.cs
Compilation issues:
- Duplicate declaration of
ILogger(as this class also exists in the file mentioned above). - Arguments to the
Warncalls.
| public void Execute(HttpContext ctx) | ||
| { | ||
| // GOOD: int | ||
| logger.Warn("Logging simple type (int):" 1); |
There was a problem hiding this comment.
Did you intend to concatenate? "Logging simple type (int):" + 1
|
|
||
| public class LogForgingSimpleTypes | ||
| { | ||
| public void Execute(HttpContext ctx) |
There was a problem hiding this comment.
The test cases doesn't test the simple type sanitization as the input to the logger are hardcoded constants and not a data flow source for the log forging query (maybe use the same pattern as in https://github.com/github/codeql/blob/main/csharp/ql/test/query-tests/Security%20Features/CWE-117/LogForging.cs)
There was a problem hiding this comment.
Interestingly, I have seen hardcoded Enums trigger this, but I will address this to use a true data flow source.
| @@ -57,7 +57,8 @@ class SimpleTypeSanitizedExpr extends DataFlow::ExprNode { | |||
| SimpleTypeSanitizedExpr() { | |||
| exists(Type t | t = this.getType() or t = this.getType().(NullableType).getUnderlyingType() | | |||
| t instanceof SimpleType or | |||
| t instanceof SystemDateTimeStruct | |||
| t instanceof SystemDateTimeStruct or | |||
| t instanceof Enum | |||
There was a problem hiding this comment.
@hvitved : Do you know of any historical reason why we haven't considered enums as being sanitizers (I think it looks very reasonable that they should be considered sanitizers - but just checkin with you as this change has a wide impact)?
Pull Request checklist
All query authors
.qhelp. See the documentation in this repository.Internal query authors only
.ql,.qll, or.qhelpfiles. See the documentation (internal access required).