C#: Add a few more sanitizers to cs/web/unvalidated-url-redirection
#15596
Conversation
There was a problem hiding this comment.
Thank you for doing this @erik-krogh !!
This looks plausible to me.
I have added some comments.
We should probably also
- Add a change note.
- Run DCA to check if there are any changes to performance or results.
|
|
||
| public class UrlRedirectHandler2 : IHttpHandler | ||
| { | ||
| private const String VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"; |
| // BAD: a request parameter is incorporated without validation into a URL redirect | ||
| ctx.Response.Redirect(ctx.Request.QueryString["page"]); | ||
|
|
||
| List<string> VALID_REDIRECTS = new List<string>{ "http://cwe.mitre.org/data/definitions/601.html", "http://cwe.mitre.org/data/definitions/79.html" }; |
There was a problem hiding this comment.
Consider moving this declaration to a private const field (it looks like this was the original intention :-) )
| @@ -139,6 +139,75 @@ class LocalUrlSanitizer extends Sanitizer { | |||
| LocalUrlSanitizer() { this = DataFlow::BarrierGuard<isLocalUrlSanitizer/3>::getABarrierNode() } | |||
| } | |||
|
|
|||
| /** | |||
| * A argument to a call to `List.Contains()` that is a sanitizer for URL redirects. | |||
There was a problem hiding this comment.
| * A argument to a call to `List.Contains()` that is a sanitizer for URL redirects. | |
| * An argument to a call to `List.Contains()` that is a sanitizer for URL redirects. |
| } | ||
|
|
||
| /** | ||
| * A URL argument to a call to `List.Contains()` that is a sanitizer for URL redirects. |
There was a problem hiding this comment.
| * A URL argument to a call to `List.Contains()` that is a sanitizer for URL redirects. | |
| * An URL argument to a call to `List.Contains()` that is a sanitizer for URL redirects. |
| private predicate isContainsUrlSanitizer(Guard guard, Expr e, AbstractValue v) { | ||
| exists(MethodCall method | method = guard | | ||
| exists(Method m | m = method.getTarget() | | ||
| m.hasName("Contains") and |
There was a problem hiding this comment.
This is any method with the name Contains. Should we narrow this to Contains methods defined on specific classes or interfaces? Did you intend to only have this to apply to the Contains method on ICollection<T>?
Maybe we should consider to leave it as is. There are lots of collection like interfaces and classes that have a Contains method. We can narrow this in case we get false negative reports.
There was a problem hiding this comment.
I thought a little about this, and I agree that we should keep it wide, so we also find Contains method in other interfaces.
I'll add a comment about it.
| exists(MethodCall method | method = guard | | ||
| exists(Method m | m = method.getTarget() | | ||
| m.hasName("Contains") and | ||
| e = method.getArgument(0) | ||
| ) and | ||
| v.(AbstractValues::BooleanValue).getValue() = true | ||
| ) |
There was a problem hiding this comment.
| exists(MethodCall method | method = guard | | |
| exists(Method m | m = method.getTarget() | | |
| m.hasName("Contains") and | |
| e = method.getArgument(0) | |
| ) and | |
| v.(AbstractValues::BooleanValue).getValue() = true | |
| ) | |
| guard = | |
| any(MethodCall method | | |
| exists(Method m | m = method.getTarget() | | |
| m.hasName("Contains") and | |
| e = method.getArgument(0) | |
| ) and | |
| v.(AbstractValues::BooleanValue).getValue() = true | |
| ) |
| exists(PropertyAccess access | access = guard | | ||
| access.getProperty().getName() = "IsAbsoluteUri" and | ||
| access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and | ||
| e = access.getQualifier() and | ||
| v.(AbstractValues::BooleanValue).getValue() = false | ||
| ) |
There was a problem hiding this comment.
| exists(PropertyAccess access | access = guard | | |
| access.getProperty().getName() = "IsAbsoluteUri" and | |
| access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and | |
| e = access.getQualifier() and | |
| v.(AbstractValues::BooleanValue).getValue() = false | |
| ) | |
| guard = | |
| any(PropertyAccess access | | |
| access.getProperty().hasFullyQualifiedName("System", "Uri", "IsAbsoluteUri") and | |
| e = access.getQualifier() and | |
| v.(AbstractValues::BooleanValue).getValue() = false | |
| ) |
| exists(EqualityOperation comparison | comparison = guard | | ||
| exists(PropertyAccess access | access = comparison.getAnOperand() | | ||
| access.getProperty().getName() = "Host" and | ||
| access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and | ||
| e = access.getQualifier() | ||
| ) and | ||
| if comparison instanceof EQExpr | ||
| then v.(AbstractValues::BooleanValue).getValue() = true | ||
| else v.(AbstractValues::BooleanValue).getValue() = false | ||
| ) |
There was a problem hiding this comment.
| exists(EqualityOperation comparison | comparison = guard | | |
| exists(PropertyAccess access | access = comparison.getAnOperand() | | |
| access.getProperty().getName() = "Host" and | |
| access.getQualifier().getType().getFullyQualifiedName() = "System.Uri" and | |
| e = access.getQualifier() | |
| ) and | |
| if comparison instanceof EQExpr | |
| then v.(AbstractValues::BooleanValue).getValue() = true | |
| else v.(AbstractValues::BooleanValue).getValue() = false | |
| ) | |
| guard = | |
| any(EqualityOperation comparison | | |
| exists(PropertyAccess access | access = comparison.getAnOperand() | | |
| access.getProperty().hasFullyQualifiedName("System", "Uri", "Host") and | |
| e = access.getQualifier() | |
| ) and | |
| if comparison instanceof EQExpr | |
| then v.(AbstractValues::BooleanValue).getValue() = true | |
| else v.(AbstractValues::BooleanValue).getValue() = false | |
| ) |
DCA looks fine: https://github.com/github/codeql-dca-main/tree/data/erik-krogh/PR-15596-0-csharp__3/reports (I had some trouble with an outdated local copy of DCA, that's why there are a bunch of failed experiments). |
Great - thank you @erik-krogh !! |
I was looking at the QHelp for
cs/web/unvalidated-url-redirection, but it was hard to write good fix examples that weren't flagged by the query.So I've added a few new sanitizers to the query.
I've worked very little with queries that use the shared dataflow library, and I think this might be my first "real" C# PR.
What I'm saying is to look closely, because I might have made some silly mistake.