Fix S2068 FN: add support for SecureString

[egon-okerman-sonarsource]

Why

We are working to improve the detection of issues in the Juliet benchmark. It heavily relies on SecureString for a lot of its tests.

There currently are 284 FNs that relate to hardcoded passwords (S2068) and use SecureString.AppendChar(char c). Therefore, we want to make sure that methods using SecureString with a constant value are correctly detected.

Detection Logic

An issue should be triggered if SecureString.AppendChar(char c) is called with a hardcoded char. Since this method only accepts one char, an issue should also be raised if c is one character of a hardcoded string instead (see Example.)

Example

Sensitive

(This example is copied from the Juliet benchmark)

private void Bla()
{
    using (SecureString securePwd = new SecureString())
    {
        for (int i = 0; i < "AP@ssw0rd".Length; i++)
        {
            securePwd.AppendChar("AP@ssw0rd"[i]); // Sensitive
        }

        // Do something with securePwd
    }
}

Compliant

private void Bla(String password)
{
    using (SecureString securePwd = new SecureString())
    {
        for (int i = 0; i < password.Length; i++)
        {
            securePwd.AppendChar(password[i]); // Compliant
        }

        // Do something with securePwd
    }
}

[martin-strecker-sonarsource]

See also this comment from the research ticket:

Since SecureString has been discouraged from use for about 6 years, I have kept the implementation ticket for S2068 as minimal as possible. Perhaps it would be better to make a rule that warns against the use of SecureString in general 😅.

Source

Hint: DE0001 is a rule provided by the .Net platform team. There is no need to reimplement it.

[martin-strecker-sonarsource]

In the investigation ticket, there is an advice about the scope of the change:

SecureString has several methods that can be used to change its internal value, however, Juliet only uses one: SecureString.AppendChar(char c).

We should ignore the constructor and the other modification methods. It is not worth investing too much in a deprecated API.

[martin-strecker-sonarsource]

The rule is about a data flow of a constant string, which is deconstructed into its characters and passed to SecureString.AppendChar. While it could be implemented via symbolic execution, there are some problems with this:

• S2068 already supports a lot of scenarios but is not based on SE. Therefore, supporting this single case with the help of SE seems to be a lot of work. Rewriting the rule based on SE is another alternative, but we should have a dedicated sprint if we want to do that.
• The data flow is quite complex, and the SE needs to learn "char is const" from "string is const," which requires learning about list content. This is quite complex, and nothing we have done so far in the engine.
• The scope of what we are supposed to detect is well-defined and limited. We can most likely do so in the existing rules fashion.

[pavel-mikula-sonarsource]

I think SE is not needed to detect this. Using node.FindStringConstant(model) should give good-enough result for most of the cases. It has some basic value-propagation tracking capabilities for linear cases.
Like

var inVariable = FromConst
var propagated=inVariable
propagated // Will work