security-code-scan icon indicating copy to clipboard operation
security-code-scan copied to clipboard

No SCS0029 (XSS) on return from an action

Open dan-neumegen-xero opened this issue 4 years ago • 3 comments

Environment (please complete the following information):

  • Version 3.5.3/5.0.0
  • Branch: SecurityCodeScan/SecurityCodeScan 2019?
  • Visual Studi2019 (16.8.3) and Rider
  • OS: Windows

Describe the bug Hi there, we have a project currently consuming SecurityCodeScan version 3.5.3. I was looking at upgrading to SecurityCodeScan 2019 version 5.0.0 and did a quick test. I arbitrary choose SCS0029 to test and placed the following code into a controller:

    [HttpGet(""{myParam}"")]
    public string Get(string myParam)
    {
        return "value " + myParam;
    }

Using the old SecurityCodeScan version 3.5.3 this is reported as a vulnerability, However after upgrading to SecurityCodeScan 2019 version 5.0.0 this is no longer reported as an issue anymore.

Repro Add the following code:

    [HttpGet(""{myParam}"")]
    public string Get(string myParam)
    {
        return "value " + myParam;
    }

to a project which references SecurityCodeScan version 3.5.3. Confirm it is registered as a problem.

Upgrade to SecurityCodeScan 2019 version 5.0.0, and see that it is no longer shown as a problem.

dan-neumegen-xero avatar Mar 03 '21 18:03 dan-neumegen-xero

Yes, the tests are disabled for this case, for example: https://github.com/security-code-scan/security-code-scan/blob/5fecefe194ce3b1519d52cd4b758e40a38bf5af6/SecurityCodeScan.Test/Tests/XssPreventionAnalyzerTest.cs#L123-L125

There are two reasons for this:

  1. Technical. Current taint tracking engine doesn't support sink on return. It possible to define a sink as a method or constructor call, property assignment, etc. but not as return statement. It is a challenge to implement it.
  2. Non technical. I'm not sure if this is correct way to catch possible XSS vulnerabilities, even if it was how 3.5.3 worked, because the output may be later html-encoded in a view. However it is still works for Response.Write(input), so probably this introduces inconsistences.

So currently it is not clear which direction to choose.

JarLob avatar Mar 03 '21 19:03 JarLob

Sounds like some complex problems to deal! Good that you are aware of it though!

dan-neumegen-xero avatar Mar 03 '21 19:03 dan-neumegen-xero

I'll keep the issue as a work item additionally to the todo. Ideally all todo in the code have to have corresponding issues. :)

JarLob avatar Mar 03 '21 19:03 JarLob