TaintedDataAnalysis discussion

[dotpaul]

Opening an issue for discussion of tainted data analysis improvements.

[dotpaul]

Since you are here. Is there a way to define a return from such method as a sink? Like when it returns a non html encoded string.

Originally posted by @JarLob in https://github.com/dotnet/roslyn-analyzers/pull/3938#issuecomment-669850790

---

public class MyController : Controller
{
    public string DoSomething(string input) 
    {           
        return input; // << input is tainted, treat return as sink
    }
}

Originally posted by @JarLob in https://github.com/dotnet/roslyn-analyzers/pull/3938#issuecomment-670033085

---

I forget if I answered this already, but you could do something similar with what you're doing for parameter references and SourceInfos in PR #3966. Make SinkInfo have a way of checking if an IMethodSymbols return values should be checked, and in TaintedDataOperationVisitor, implement a ProcessReturnValue() to check (PropertySetDataFlowOperationVisitor.ProcessReturnValue() does something similar).

Edit: And inside ProcessReturnValue(), this.DataFlowAnalysisContext.OwningSymbol would be the potential IMethodSymbol.

[dotpaul]

I have a question. Is there a way to define a validator? Like:

if (IsLocalUrl(url))
    ;// assume the url is sanitized/safe here
else
    ;// url is not safe

For the beginning I have decided to ignore the return value and define a sanitizer that outputs to the same argument like:

                    (
                        (methodName, arguments) => methodName == "IsLocalUrl",
                        new[] { ("url", "url") }
                    ),

so this code should not warn:

IsLocalUrl(url);
Redirect(url);

Unfortunately it didn't work as expected, the url is still tainted.

Originally posted by @JarLob in https://github.com/dotnet/roslyn-analyzers/pull/3951#issuecomment-669914335

---

Hey @JarLob, if you haven't already figured this out, could you push your changes up to your GitHub fork somewhere, and I can try it out?

[JarLob]

Hey @JarLob, if you haven't already figured this out, could you push your changes up to your GitHub fork somewhere, and I can try it out?

I have pushed the changes and three tests to https://github.com/JarLob/roslyn-analyzers/tree/sanitizers. Two of them pass and the Redirect_IsLocalUrl fails. The branch is the merge of the sanitizers brach with the source arguments branch, because it looks like something is wrong with "untainting" the arguments. If the taint source is a result of property or function return it works correctly.

[dotpaul]

@JarLob Oh hrm, I think the problem is, as it is now, every IParameterReferenceOperation is creating a tainted abstract value. Might have to rethink the overall approach in #3966. Maybe only want to taint parameters at the beginning of method.

[dotpaul]

Example:

        [Fact]
        public async Task Redirect_IsLocalUrl_NotParameterReference()
        {
            await VerifyCSharpWithDependenciesAsync(@"
using System;
using Microsoft.AspNetCore.Mvc;

class TestClass : Controller
{
    public void SomeAction(string url)
    {
        string s = url;
        if (Url.IsLocalUrl(s))   // IsLocalUrl untaints the string pointed to by 's' and 'url'.
            Redirect(s);    // Since 's' isn't a IParameterReferenceOperation, doesn't create a new tainted value.
    }
}");
        }

Also, only need rethink #3966 for supporting validators. I suspect a lot of TaintedDataOperationVisitor would have to change to support validators.

[JarLob]

@JarLob Oh hrm, I think the problem is, as it is now, every IParameterReferenceOperation is creating a tainted abstract value. Might have to rethink the overall approach in #3966.

Bummer :( So it can be reproduced without sanitizers:

        [Fact]
        public async Task TaintFunctionArguments_Reassignment_NoDiagnostic()
        {
            await VerifyCSharpWithDependenciesAsync(@"
using System.Data.SqlClient;
using Microsoft.AspNetCore.Mvc;

public class MyController
{
    public void DoSomething(string input)
    {
        input = """";
        new SqlCommand(input);
    }
}");
        }

Maybe only want to taint parameters at the beginning of method.

Any idea how to do it? I looked into OperationKind.MethodBodyOperation in debugger, but it's weird: I can see method arguments in Syntax property preview, but don't see how to access them from IOperation. I found the way, see the PR.

[dotpaul]

Bummer :( So it can be reproduced without sanitizers:

Ah, good catch!

[JarLob]

So I tried with latest changes and the unconditional validator (defined as sanitizer) works as expected. The only failing test from https://github.com/JarLob/roslyn-analyzers/commit/0ac982e36a667a26ea1ec9e00d370e97fe903931 is:

using System;
using Microsoft.AspNetCore.Mvc;
class TestClass : Controller
{
    public void SomeAction(string url)
    {
        if (Url.IsLocalUrl(url))
            ;
        else
            Redirect(url);
    }
}

Do I understand correctly that it requires a lot of changes to TaintedDataOperationVisitor to support these conditional branches? BTW I noticed an already existing test https://github.com/dotnet/roslyn-analyzers/blob/825e2ea0a06ddf33e83596c71de147cb1be2e532/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Security/ReviewCodeForOpenRedirectVulnerabilitiesTests.cs#L86-L90 that I believe was an initial attempt to test a conditional branch, but later was rewritten into something that doesn't make sense (because there is no need to check the input if all it does a test for constant passed to Redirect.

[JarLob]

@dotpaul One more question. Let's say I need to define a sanitizer that works only if a certain value is passed to one of the arguments. The sanitizer in question is Uri.TryCreate(inputString, UriKind.Relative, out var uri). The output uri can be treated as local to the server if the second argument is UriKind.Relative and the function succeeds. I can check for a constant value in method matcher, but it is limited and won't resolve the value if it is assigned to an intermediate variable. Is there a better way? I see there are some kind of value flow analyzers...

[dotpaul]

Do I understand correctly that it requires a lot of changes to TaintedDataOperationVisitor to support these conditional branches?

Yep, probably. ParameterValidationAnalysis might have give some ideas on how to go about implementing it if you want to try. @mavasani, do you have any pointers?

BTW I noticed an already existing test

https://github.com/dotnet/roslyn-analyzers/blob/825e2ea0a06ddf33e83596c71de147cb1be2e532/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Security/ReviewCodeForOpenRedirectVulnerabilitiesTests.cs#L86-L90

that I believe was an initial attempt to test a conditional branch, but later was rewritten into something that doesn't make sense (because there is no need to check the input if all it does a test for constant passed to Redirect.

Sorry, nope. :smile: That was just testing an example solution in the docs.

@dotpaul One more question. Let's say I need to define a sanitizer that works only if a certain value is passed to one of the arguments. The sanitizer in question is Uri.TryCreate(inputString, UriKind.Relative, out var uri). The output uri can be treated as local to the server if the second argument is UriKind.Relative and the function succeeds. I can check for a constant value in method matcher, but it is limited and won't resolve the value if it is assigned to an intermediate variable. Is there a better way? I see there are some kind of value flow analyzers...

ValueContentAnalysis is what you're looking. It can track the integer values of enums. https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Writing%20dataflow%20analysis%20based%20analyzers.md#well-known-flow-analyses

Some other analyses make use of ValueContentAnalysis.