C#: Add query for Insecure Direct Object Reference

[joefarebrother]

Adds query for Insecure direct object reference.
Focuses on cases in which action modifies state; i.e. edits or deletes it.

[github-actions]

QHelp previews:

csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp

Insecure Direct Object Reference

When an action method accepts an ID parameter used to control which resource (e.g. a comment, a user profile, etc) is being accessed/modified, checks should me made to ensure that the current user is authorized to access that resource. Otherwise, an attacker could access an arbitrary resource by guessing the ID parameter.

Recommendation

Ensure that the current user is authorized to access the resource of the provided ID.

Example

In the following example, in the case marked BAD, there is no authorization check, so any user is able to edit any comment. In the case marked GOOD, there is a check that the current usr matches the author of the comment.

    // BAD - Any user can access this method.
    protected void btn1_Click(object sender, EventArgs e) {
        string commentId = Request.QueryString["Id"];
        Comment comment = getCommentById(commentId);
        comment.Body = inputCommentBody.Text;
    }

    // GOOD - The user ID is verified.
    protected void btn2_Click(object sender, EventArgs e) {
        string commentId = Request.QueryString["Id"];
        Comment comment = getCommentById(commentId);
        if (comment.AuthorName == User.Identity.Name){
            comment.Body = inputCommentBody.Text;
        }
    }

The following example shows a similar case for the ASP.NET Core framework.

public class CommentController : Controller {
    private readonly IAuthorizationService _authorizationService;
    private readonly IDocumentRepository _commentRepository;

    public CommentController(IAuthorizationService authorizationService,
                              ICommentRepository commentRepository)
    {
        _authorizationService = authorizationService;
        _commentRepository = commentRepository;
    }

    // BAD: Any user can access this.
    public async Task<IActionResult> Edit1(int commentId, string text) {
        Comment comment = _commentRepository.Find(commentId);
        
        comment.Text = text;

        return View();
    }

    // GOOD: An authorization check is made.
    public async Task<IActionResult> Edit2(int commentId, string text) {
        Comment comment = _commentRepository.Find(commentId);
        
        var authResult = await _authorizationService.AuthorizeAsync(User, Comment, "EditPolicy");

        if (authResult.Succeeded) {
            comment.Text = text;
            return View();
        }
        else {
            return ForbidResult();
        }
    }
}

References

• OWASP - Insecure Direct Object Refrences.
• OWASP - Testing for Insecure Direct Object References.
• Microsoft Learn = Resource-based authorization in ASP.NET Core.

[hvitved]

A few minor comments.

[michaelnebel]

Really nice!
Only some minor questions and comments.

[michaelnebel]

You could consider to do case-insensitive matching instead of explicitly converting the description to lowercase before matching: (eg. ".(edit|delete|modify|change)." -> "(?i).(edit|delete|modify|change).".
The same goes for all similar patterns in this file.

[mbg]

I am wondering if it would be possible to come up with a better strategy for detecting such stateful actions. Obviously the current approach is prone to miss things that are not in the list or where code bases use non-English names for things. Could we check for data / taint flow to known sinks like database access etc. instead? That might still not catch everything, but might be more accurate / inclusive of global customers than just looking at names.

[joefarebrother]

I did try a dataflow based approach to reduce dependence on method/parameter names, however I ran into issues (written up here) with it not working for the case in webgoat; essentially due to a framework-internal control flow step that would need to be modeled specially.

[mbg]

I don't think it is unreasonable to do some framework-specific modelling for common web frameworks. I wonder if @michaelnebel or @hvitved have any thoughts on how this particular data flow issue with WebForms could be modelled well?

[joefarebrother]

Since as you mention in another comment, WebForms is outdated and not commonly used, does it make sense to implement specific flow steps for it?

[michaelnebel]

This changed slightly when it was moved. Now this.getARoute() is being used instead of this.getDeclaringType().getFile().getRelativePath() (which is the same as long as getARoute is not overridden).
Is this a bug fix (this might change the description of WebFormActionMethod elements)?

[joefarebrother]

This is an intended fix (the route to a method should also contribute to knowledge about the names referring to that method to help determine what it does)

[mbg]

Thank you for working on this! Detecting insecure direct object reference would be a very valuable addition to our coverage, and it seems like a tricky query to implement well. I have added a few comments where I have questions about the possibility of FPs.

I am wondering to what extent it might be good to build this query in an extensible way so that:

• We can gradually add specific support for web frameworks / auth checks / stateful actions / etc.
• Customers could add support for their own auth mechanisms / stateful actions / etc.

Have you thought about approaching the query in that way? It seems ambitious to try and cover everything with a "closed" query like this.

[mbg]

I am concerned by the rate at which this may produce false positives. My understanding is that e.g. in ASP.NET the standard mechanism for managing authentication/authorisation is through attributes. The query does not seem to handle those (as well as auth code inherited from parent classes / any other form of auth), so would this query trigger alerts in those cases?

[joefarebrother]

Are authorization attributes alone able to manage authorization that's based on the request parameters?
In these MRVA results I don't see a lot of evidence of other auth mechanisms that were missed.

[mbg]

No, using attributes is only sufficient where authorisation is based on roles, policies, etc.

For resource-based authorization checks in the body of the handler are indeed necessary. Would the example in that article be identified by checksUser?

However, role-based or policy-based authorisation may be sufficient in a lot of cases, so we probably shouldn't flag up insecure direct object reference vulnerabilities where checks are performed in that way.

Based on your description of the MRVA experiment, it seems like the results are only for the top 100 repos? Are those the top 100 C# repos or the top 100 repos that use ASP.NET? In any case, I would probably use a larger sample. It is also likely that the different authorisation mechanisms offered by ASP.NET are more prevalent in enterprise codebases, rather than in open-source ones.

[joefarebrother]

It looks like the example in the article would be identified by checksUser, because it uses the User property.
This MRVA run with the checksUser condition reversed shows the auth methods that are being used.

However in this top 1000 MRVA run, it looks like there are indeed some FPs from role-based authentication for admin roles; so this is something that should be checked.
Should all instances of the Authorize attribute with Roles or Policy fields set be considered sufficient, or should a more complex check be made?

[mbg]

because it uses the User property

Ah, I don't think I had realised that properties count as Callables.

However in this top 1000 MRVA run, it looks like there are indeed some FPs from role-based authentication for admin roles; so this is something that should be checked.

Thank you for running that experiment! It does indeed seem to confirm my concerns here.

Should all instances of the Authorize attribute with Roles or Policy fields set be considered sufficient, or should a more complex check be made?

I think the logic is a little bit more complicated because the Authorize attribute may be placed on the method, class, or parent classes(?). So you need to check in all of these places for that attribute as well as any attributes which inherit from Authorize. Furthermore, you also need to check for the absence of the AllowAnonymous attribute, which may override any Authorize-like attribute that's placed further out. That's just off the top of my head, so there may be further subtleties. Microsoft's documentation should have the full details.

In terms of the arguments to the Authorize attribute, I don't think we should care at all. As long as there is some authorisation check, that should be considered a mitigation for the purpose of this query since we can't reason about what the intended audience for a particular endpoint is.

[joefarebrother]

It is my understanding that an Authorize attribute with no arguments means "Any logged in user can access this", whereas this query focuses on cases where we'd expect stronger access controls than that (e.g. deleting any resource). Is this correct?

[mbg]

Yes, although consider that there might be web applications where most visitors have no accounts and the only authorised users you'll get are administrators that should be able to perform such actions. (E.g. a blog, a simple CMS/news website, etc.) In those cases just being logged in may be sufficient access control.

Also, for classes that inherit from Authorize, we wouldn't necessarily expect any parameters either, e.g.:

class MyAuthorizeAttribute : AuthorizeAttribute
{
    public MyAuthorizeAttribute()
    {
        base.Policy = "Administrators";
    }
}

[mbg]

I am wondering if it would be possible to come up with a better strategy for detecting such stateful actions. Obviously the current approach is prone to miss things that are not in the list or where code bases use non-English names for things. Could we check for data / taint flow to known sinks like database access etc. instead? That might still not catch everything, but might be more accurate / inclusive of global customers than just looking at names.

[mbg]

This (or a similar) test should probably cover other methods of authorisation/authentication that are available in ASP.NET to make sure we don't generate too much noise with false positives.

[michaelnebel]

Good work @joefarebrother !

[mbg]

I see that the PR is now marked as ready for docs review, but the concerns I expressed in my initial review have not yet been addressed, so I'm explicitly requesting changes to make it clear that I don't believe this query should be merged as is.

My primary concern continues to be with the number of FPs we are likely to see as a result of the current implementation. In the extended MRVA experiment you ran, we can see a number of false positives: e.g. consider the 25 results for simplcommerce/SimplCommerce, they are all false positives since there are Authorize attributes on route handlers/controllers. Similarly for the 24 results for btcpayserver/btcpayserver etc.

Furthermore, we expect to see results for this query in applications, not so much in libraries. This is because we'd only really expect to see controllers and route handlers in applications. However, most of the repos in the top 1000 are likely libraries, not applications. In the above MRVA run, you can see that the repos with lots of FPs are applications of some sort. I'd therefore (both intuitively and based on the above evidence) expect FPs to be particularly prevalent in (private) application repos - think Microsoft who have a lot of codebases with C# web applications using ASP.NET that I'd expect to light up like a Christmas tree with this query if most of their routes get flagged erroneously in the same way that happens for the above public repositories.

As a minimum, the ASP.NET authorization attributes should be supported before we proceed with this.

[joefarebrother]

@mbg Sure; 97d2048 adds checks for Authorize attributes.
Here is an MRVA run with these changes.

I had some ideas for a more complex check involving checking for roles and policies on authorize attributes (including via a subclass or the attribute), and allowing plain authorize attributes only when no such role/policy based authorization exists. This should improve some of the FNs introduced by a blanket check on all authorize attributes while not introducing more FPs in simple cases where "logged in" is the only level of privilege. Would it be ok to save such changes for follow-up work?

[mbg]

Thanks for making these changes and adding some new tests as well! The MRVA results look a bit better, although there are a lot of FPs still left that are a bit more difficult to tackle. Since this is a medium-precision query, however, I am a lot happier now that we are at least dealing with the attributes.

I have some small niggles with the implementation that can easily be addressed, which I have commented on. You may also want to add a few more tests to check that the behaviour in the presence of inheritance is correct for methods/classes.

I wouldn't worry about FNs too much at this point. Reducing FPs to as close to zero as possible is more important than ruling out all FNs.

It's probably worth combing the ASP.NET docs more thoroughly and understanding all of the common authorisation patterns that exist in addition to the attributes (such as authorisation handlers installed into the services object in ConfigureServices). I suspect dealing with those will rule out some more FPs that we can. Do you think you could have a look at that to see if there is anything that can be addressed as part of this work?

See e.g. https://github.com/elsa-workflows/elsa-core/blob/master/src/samples/aspnet/Elsa.Samples.HttpEndpointSecurity/Startup.cs which is one of the projects that comes up in the MRVA results, but where a bunch of authorisation-related configuration is taking place on application startup.

[mbg]

I am wondering about the correctness of this predicate with different permutations of these attributes in the inheritance chain. E.g. suppose you have a base class with a method that has the AllowAnonymous attribute and then override that method in a child class, where you give it an explicit Authorize attribute. What effect would that have in ASP.NET? Does this predicate correctly model that behaviour?

[joefarebrother]

Hmm; I'm not sure what ASP.NET would do in that situation.
How much do we need to be concerned with correctly covering every edge case? As it seems like these kinds of situations are unlikely to come up in real codebases.

[mbg]

I do not know off the top of my head what ASP.NET would do in that case, so I'd need to try it with a sample project as well. I am happy if we postpone looking into this and don't block this PR on addressing this detail.

[mbg]

As discussed, there is a bunch of work we are deferring to next quarter and I am happy to proceed given that this is marked as a medium precision query.

I have commented on a few minor points here that can probably be addressed quickly. In any case, I think this can move ahead into docs review since I don't think there are any problems with the documentation / nothing is likely to change there.

[github-actions]

QHelp previews:

csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp

Insecure Direct Object Reference

Resources like comments or user profiles can be accessed and modified through an action method. To target a certain resource, the action method accepts an ID parameter pointing to that specific resource. If the methods do not check that the current user is authorized to access the specified resource, an attacker can access a resource by guessing or otherwise determining the linked ID parameter.

Recommendation

Ensure that the current user is authorized to access the resource of the provided ID.

Example

In the following example, in the "BAD" case, there is no authorization check, so any user can edit any comment for which they guess or determine the ID parameter. The "GOOD" case includes a check that the current user matches the author of the comment, preventing unauthorized access.

    // BAD - Any user can access this method.
    protected void btn1_Click(object sender, EventArgs e) {
        string commentId = Request.QueryString["Id"];
        Comment comment = getCommentById(commentId);
        comment.Body = inputCommentBody.Text;
    }

    // GOOD - The user ID is verified.
    protected void btn2_Click(object sender, EventArgs e) {
        string commentId = Request.QueryString["Id"];
        Comment comment = getCommentById(commentId);
        if (comment.AuthorName == User.Identity.Name){
            comment.Body = inputCommentBody.Text;
        }
    }

The following example shows a similar scenario for the ASP.NET Core framework. As above, the "BAD" case provides an example with no authorization check, and the first "GOOD" case provides an example with a check that the current user authored the specified comment. Additionally, in the second "GOOD" case, the `Authorize` attribute is used to restrict the method to administrators, who are expected to be able to access arbitrary resources.

public class CommentController : Controller {
    private readonly IAuthorizationService _authorizationService;
    private readonly IDocumentRepository _commentRepository;

    public CommentController(IAuthorizationService authorizationService,
                              ICommentRepository commentRepository)
    {
        _authorizationService = authorizationService;
        _commentRepository = commentRepository;
    }

    // BAD: Any user can access this.
    public async Task<IActionResult> Edit1(int commentId, string text) {
        Comment comment = _commentRepository.Find(commentId);
        
        comment.Text = text;

        return View();
    }

    // GOOD: An authorization check is made.
    public async Task<IActionResult> Edit2(int commentId, string text) {
        Comment comment = _commentRepository.Find(commentId);
        
        var authResult = await _authorizationService.AuthorizeAsync(User, Comment, "EditPolicy");

        if (authResult.Succeeded) {
            comment.Text = text;
            return View();
        }
        else {
            return ForbidResult();
        }
    }

    // GOOD: Only users with the `admin` role can access this method.
    [Authorize(Roles="admin")]
    public async Task<IActionResult> Edit3(int commentId, string text) {
        Comment comment = _commentRepository.Find(commentId);
        
        comment.Text = text;

        return View();
    }
}

References

• OWASP: Insecure Direct Object Refrences.
• OWASP: Testing for Insecure Direct Object References.
• Microsoft Learn: Resource-based authorization in ASP.NET Core.

[joefarebrother]

Performance on the latest DCA run finally looks good.
@mbg additional test cases and an additional example have also been added.
This should now be ready for docs review.

[sabrowning1]

👋🏼 @joefarebrother, thanks for your work on the user-facing text here! I've left a few comments below; let me know if you have any questions or feedback 🙂

[sabrowning1]

User-facing text looks good! 🚀