Skip to content

Enhanced LogForgingQuery to treat C# Enums as simple types. #17866

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

Closed
ianroof wants to merge 3 commits into from
Closed

Enhanced LogForgingQuery to treat C# Enums as simple types. #17866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f21f72
Enhanced LogForgingQuery to treat C# Enums as simple types.
ianroof Sep 26, 2024
61f042e
Merge branch 'main' into ianroof/csharp-log-forging-query-enums
ianroof Oct 29, 2024
e9692cc
Merge branch 'main' into ianroof/csharp-log-forging-query-enums
ianroof Oct 29, 2024
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
3 changes: 2 additions & 1 deletion csharp/ql/lib/semmle/code/csharp/security/Sanitizers.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@michaelnebel michaelnebel Oct 30, 2024

Choose a reason for hiding this comment

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

@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)?

)
}
}
Expand Down
4 changes: 4 additions & 0 deletions csharp/ql/src/change-notes/2024-09-18-csharp-log-forging-enum.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: fix
Copy link
Contributor

@michaelnebel michaelnebel Oct 30, 2024

Choose a reason for hiding this comment

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

Suggested change
category: fix
category: minorAnalysis

Copy link
Contributor

@michaelnebel michaelnebel Oct 30, 2024

Choose a reason for hiding this comment

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

This is as such not a "fix" but rather a change in, how we perceive enums.

Copy link
Contributor Author

@ianroof ianroof Oct 30, 2024

Choose a reason for hiding this comment

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

That makes sense.

---
* Enhanced LogForgingQuery to treat C# Enums as simple types.
46 changes: 46 additions & 0 deletions csharp/ql/test/query-tests/Security Features/CWE-117/LogForgingSimpleTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
using System;
using System.Diagnostics;
using System.IO;
using System.Net;
using System.Web;
using Microsoft.Extensions.Logging;

class ILogger
{
public void Warn(string message) { }
}

enum TestEnum
{
TestEnumValue
}

public class LogForgingSimpleTypes
Copy link
Contributor

@michaelnebel michaelnebel Oct 30, 2024

Choose a reason for hiding this comment

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

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 Warn calls.

{
public void Execute(HttpContext ctx)
Copy link
Contributor

@michaelnebel michaelnebel Oct 30, 2024

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@ianroof ianroof Oct 30, 2024

Choose a reason for hiding this comment

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

Interestingly, I have seen hardcoded Enums trigger this, but I will address this to use a true data flow source.

{
// GOOD: int
logger.Warn("Logging simple type (int):" 1);
Copy link
Contributor

@michaelnebel michaelnebel Oct 30, 2024

Choose a reason for hiding this comment

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

Did you intend to concatenate? "Logging simple type (int):" + 1


// GOOD: long
logger.Warn("Logging simple type (int):" 1L);

// GOOD: float
logger.Warn("Logging simple type (float):" 1.1);

// GOOD: double
logger.Warn("Logging simple type (double):" 1.1d);

// GOOD: decimal
logger.Warn("Logging simple type (double):" 1.1m);

// GOOD: Enum
logger.Warn("Logging simple type (Enum):" TestEnum.TestEnumVAlue);

// GOOD: DateTime
logger.Warn("Logging simple type (int):" new DateTime());

// GOOD: DateTimeOffset
logger.Warn("Logging simple type (int):" DateTimeOffset.UtcNow);
}
}