New Hot Spot Rule: Require explicit authorization

Unauthorized access is security hot spot. Within .NET both Minimal API and (API) controller methods allow anonymous access by default. It might be worth (from a security perspective) to require explicitly specify authorization or anonymous access.

Non-compliant

class Controller : ControllerBase
{
    public int TheAnswer() => 42; // Noncompliant {{Explicitly specify authorization}}
}

void Main()
{
    app.MapGet("/answer", () => 42);
}

Compliant

class Controller : ControllerBase
{
    [AllowAnonymous]
    public int TheAnswer() => 42;
}

void Main()
{
    app.MapGet("/answer", () => 42)
        .AllowAnonymous();
}

or

class Controller : ControllerBase
{
    [Authorize("math-wizard")]
    public int TheAnswer() => 42;
}


void Main()
{
    app.MapGet("/answer", () => 42)
        .RequireAuthorization();
}

This rule is related (but different then) my other rule proposal: New Rule: Use [AllowAnonymous] or [Authorize]

Hi @Corniel, I see where you come from with this idea, as we already thought of this at Sonar’s AppSec team.

Unfortunately, we dropped the idea: We scan tones of different code bases, hundreds of different ways of writing code. And there are so many different ways of securing your code against BOLA and IDORS to the point where just explicitly flagging code when this flag is absent can be dangerous for the product: It could lead to a lot of FPs, which leads to unhappy users, which leads to unhappy business.

I am still going to consider this idea and experiment with it but yeah I do not have a good feeling for now. Thanks a lot for your two ideas, it’s always very cool to get ideas from users :smiley:

Cheers,
Loris

2 Likes

To me, there are two options:

  1. The project has no require authorization unless explicitly anonymous in which every report of this rule should be a True Positive.
  2. The project has a require authorization unless explicitly anonymous in which every report of this rule should be a False Positive, and the rule should be disabled.

I can not think of a situation in which some of reports are TP and some are FP, a situation that might be dangerous indeed.

Good news, I think I might have found a way to bring the topic of broken authZ to our R&D. We’ll see where this go :crossed_fingers:

To answer your question, what I meant is that we support tons of code bases here at Sonar, and there’s multiple ways to add permissions to code: different permission frameworks, middlewares, etc. And researching this could be too much of a hassle with unknown value.

However… I think I might have found a way to bring this to our leadership. I can’t disclose it but I’ll still keep your posts in my sight to show that we have user support for that :smiley:

Thanks a lot!

Regards

Loris

1 Like

I honestly don’t see how this answers my question. I stated that based on the framework of choice either all reports all FP’s (in which case the rule can be disabled) or all are TP’s in which the rule if great value. Unless you know a well known and advisable framework where some reports are FP’s and others are not.

Apologies for that, I did not say why the tons of different use cases mattered. Here is the “why” I implied:

In our work here, we have a finite number of resources and people so we aim for the best impact we could do.

Reporting something with a risk of flooding with FPs is taking the risk of making a lot of users unhappy and potentially come scream about it here or within sonarcloud feedback. This is a risk of

  • reputational damage
  • losing users
  • losing prospects
  • damaging how people trust our expertise

Disabling the rule after such a flood event does not cancel things out because the damage is done. On top of that, disabling the rule means less data for us to improve the rule.

As part of how we set our priorities, working on a delivery which has significant chances of having a negative impact on the product will always be unprioritized compared to work that we are sure will have 99% positive impact on the product.

This is why we dropped the idea in the past :+1:

Cheers,

Loris

1 Like