Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix S2092 FP: When the "Secure" field is set in a conditional (may also impact S3330) #7935

Closed
pierre-loup-tristant-sonarsource opened this issue Aug 31, 2023 · 3 comments
Assignees
Labels
Area: C# C# rules related issues. Area: Security Related to Vulnerability and Security Hotspot rules Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Milestone

Comments

@pierre-loup-tristant-sonarsource
Copy link

pierre-loup-tristant-sonarsource commented Aug 31, 2023

Rule S2092 is not supposed to trigger if the Secure field of the HttpCookie is set to true (see this unit test).

For some reason a FP appear for a similar test case in the Juliet C# benchmark
https://peach.sonarsource.com/security_hotspots?id=juliet-suite-csharp&hotspots=AYly6kSJ4i6poAEDgZv7

private void Good1(HttpRequest req, HttpResponse resp)
{
  HttpCookie cookie = new HttpCookie("SecretMessage", "Drink your Ovaltine"); // Compliant: FP here
  if (req.IsSecureConnection)
  {
    cookie.Secure = true; // This should prevent the rule from raising an issue
    resp.Cookies.Add(cookie);
  }
}

This leads to 31 FPs in the Juliet C# benchmark

Incidentally, similar FPs also impact S3330.

@pierre-loup-tristant-sonarsource pierre-loup-tristant-sonarsource changed the title Fix S2092 FP: When the "Secure" field is set in a conditionnal Fix S2092 FP: When the "Secure" field is set in a conditional (also impact S3330) Sep 1, 2023
@pierre-loup-tristant-sonarsource pierre-loup-tristant-sonarsource changed the title Fix S2092 FP: When the "Secure" field is set in a conditional (also impact S3330) Fix S2092 FP: When the "Secure" field is set in a conditional (may also impact S3330) Sep 1, 2023
@andrei-epure-sonarsource andrei-epure-sonarsource added the Area: C# C# rules related issues. label Sep 8, 2023
@andrei-epure-sonarsource
Copy link
Contributor

@pierre-loup-tristant-sonarsource

This leads to 31 FNs in the Juliet C# benchmark

So it this an FP or an FN?

@andrei-epure-sonarsource andrei-epure-sonarsource added Type: False Positive Rule IS triggered when it shouldn't be. Area: Security Related to Vulnerability and Security Hotspot rules labels Sep 8, 2023
@pierre-loup-tristant-sonarsource
Copy link
Author

@andrei-epure-sonarsource it's an FP ticket. I fixed the typo.

@costin-zaharia-sonarsource costin-zaharia-sonarsource added the Sprint: Hardening Fix FPs/FNs/improvements label Oct 13, 2023
@costin-zaharia-sonarsource
Copy link
Member

Hi @pierre-loup-tristant-sonarsource, in the example you provided we have a branch where the cookie is created and it's not secure.

In order to always have a secure cookie, the cookie should be created inside the if block:

            HttpCookie cookie;
            if (req.IsSecureConnection)
            {
                cookie = new HttpCookie("SecretMessage", "Drink your Ovaltine");
                cookie.Secure = true;
                resp.Cookies.Add(cookie);
            }

Once the cookie is created in the correct scope, the rule will not raise issues anymore.
It also does not make sense to create the cookie if it's not needed/used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: Security Related to Vulnerability and Security Hotspot rules Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

No branches or pull requests

4 participants