C#: merge ServiceStack feature branch into main (+added support for remote flow sinks)
#5494
Conversation
Use fully qualified names for classes Make util predicate private Make naming more consistent with rest of ql libs
Only want returns in request methods Also care about non-string 1st args to HttpResult e.g. streams
Add provisional support for ServiceStack framework to feature branch
…function ServiceStack.IRestClient.Get()
csharp/ql/src/semmle/code/csharp/security/dataflow/flowsinks/Remote.qll
Outdated
Show resolved
Hide resolved
csharp/ql/src/semmle/code/csharp/security/dataflow/flowsinks/Remote.qll
Outdated
Show resolved
Hide resolved
…ort from Remote.qll, as it is un-necessary now.
|
@mr-sherman looks like something went wrong?
|
|
It was suggested I merge in main to see if it would resolve the failing checks, hence the 5000+ files changed. |
|
@mr-sherman do you intend to maintain the feature branch going forward, or would you like to have the feature branch merged into |
|
@yo-h I would like to get it merged into main, as the feature branch is in use at a customer and has been for almost two months. Should I cancel this PR and re-target main? |
|
Yes, re-targeting at |
|
OK, re-targeted to main from the feature branch. |
yo-h
changed the title
main (+added support for remote flow sinks)
|
Hi, is there an update on this PR? |
|
The C# team is currently on vacation, but it's on their todo list for when they return. |
There was a problem hiding this comment.
@mr-sherman I'm planning to work on this PR to be merged to main. As a first step I went through this PR and added some notes to myself, and one or two questions to you. Also, do you have some test projects to check if these changes do find the relevant problems?
| class ServiceStackRemoteRequestParameter extends ExternalLocationSink { | ||
| ServiceStackRemoteRequestParameter() { | ||
| exists(MethodCall mc | | ||
| mc.getTarget().getQualifiedName() in [ | ||
| "ServiceStack.IRestClient.Get", "ServiceStack.IRestClient.Put", | ||
| "ServiceStack.IRestClient.Post", "ServiceStack.IRestClient.Delete", | ||
| "ServiceStack.IRestClient.Patch", "ServiceStack.IRestClient.Send", | ||
| "ServiceStack.IRestClientAsync.GetAsync","ServiceStack.IRestClientAsync.DeleteAsync", | ||
| "ServiceStack.IRestClientAsync.PutAsync","ServiceStack.IRestClientAsync.PostAsync", | ||
| "ServiceStack.IRestClientAsync.PatchAsync","ServiceStack.IRestClientAsync.CustomMethodAsync" | ||
| ] and | ||
| this.asExpr() = mc.getAnArgument() |
There was a problem hiding this comment.
We can probably convert these to the new data-driven approach, lines would look something like:
ServiceStack;IRestClient;true;Get<>;;;Argument[0];external
| exists(ObjectCreation oc | | ||
| oc.getType().hasQualifiedName("ServiceStack.HttpResult") and | ||
| this.asExpr() = oc.getArgument(0) | ||
| ) |
There was a problem hiding this comment.
This can be converted to the CSV approach.
| class ServiceStackSink extends Sink { | ||
| ServiceStackSink() { | ||
| exists(MethodCall mc, Method m, int p | | ||
| (mc.getTarget() = m.getAnOverrider*() or mc.getTarget() = m.getAnImplementor*()) and | ||
| sqlSinkParam(m, p) and | ||
| mc.getArgument(p) = this.asExpr() | ||
| ) | ||
| } |
There was a problem hiding this comment.
I think we could cover all the below with CSV lines.
Using the debug query to find un-identified sinks, Untrusted Input to External API, we discovered some input being passed as parameters into the function ServiceStack.IRestClient.Get(). This is a sink for remote flow as input or is sometimes used to construct URLs for an API call.
This branch is used by CH Robinson, who notified us that our potential service stack implementation may have been incomplete.