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

More configurable CSRF diagnostic proposal #127

Conversation

@kevin-montrose
Copy link
Contributor

commented Jul 24, 2019

Draft PR, as this is proposing configuration changes and I'm sure there's gonna be some tweaks requested.

There are also some TODOs left, that I won't address until the general direction of this change is agreed upon.

Background

The Stack Overflow code base uses Attribute Based Routing, and has deviated considerably from the ASP.NET MVC defaults. Some of this is for good reason, and some of it is because our code dates back to the beta release of MVC 1, and we just made different choices than Microsoft over the years.

Some of our deviations are:

  • we use a single attribute for all routing configuration (so there are no HttpGet or HttpPost equivalents)
  • we allow multiple attributes on a method
  • we default to checking CSRF tokens for certain HTTP verbs
  • there's an explicit opt-out of CSRF token checking

An example:

[StackRoute("foo", HttpVerbs.Post)]
public ActionResult Foo() { ... } // this is _safe_

[StackRoute("bar", HttpVerbs.Put, EnsureXSRFSafe = false)]
public ActionResult Bar() { ... } // this should be _flagged_, it's not safe and needs a documented justification

This proposal seeks to add the flexibility we need, while keeping the out-of-the-box behavior of Security Code Scan unchanged.

Change

This replaces CsrfProtectionAttributes with CsrfProtection, which exposes many new options that replace the hardcoded type names in the current analyzers.

Namely, CsrfProtection exposes:

  • Name (required)
  • NameSpace (required)
  • ControllerName
  • NonActionAttributes (List)
  • AllowAnonymousAttributes (List)
  • VulnerableAttributes (List)
  • AntiCsrfAttributes (List)
  • IgnoreAttributes (List)
  • ActionAttributes (List)

Each attribute collection is made up of:

  • AttributeName (required)
  • Condition

Conditions are similar to Conditions on Behaviors, except they apply only to attribute constructors and named arguments (which are actually property setters) and the "then" branch is implicitly "consider this attribute" and thus not specified.

In Main.yml the declarations for ASP.NET MVC and ASP.NET Core MVC have been converted to this new format, and the format has been documented.

As a consequence of moving hardcoded values into Main.yml, this change also merged the two different analyzers into a single CsrfTokenDiagnosticAnalyzer.

A test has been added that demonstrates the Stack Overflow-style of routing, which illustrates the use of conditions.

All tests are passing locally.

Questions

  1. Is a proposal in this vein acceptable?

  2. How should the configuration section be structured? I don't love what's in this proposal, it's just adequate for my purposes.

  3. What names should be used for configuration? I just kind of made them up as I went along, or based them on the hardcoded variable names in the existing analyzers.

  4. How should removing/replacing the existing configs be done? It's easy enough to port them, but having two ways to configure CSRF protection may not be desirable.

  5. Is there any additional flexibility that should be included as part of this change?

…ble, combine Core and Mvc diagnostics using new flexibility, add conditional support, and add a test that demonstrates how to use new flexibility to check an attribute based routing style based on the Stack Overflow codebase.
…, rather than (or in additon to) a base class.
@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I held off on consolidating condition parsing (and bringing boolean conditions to Behaviors) as part of this, since it's already a big-ish proposal. It's kind of an obvious next step though.

@JarLob

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Hey, I didn't review it yet. But in general:
The change is welcome the last time I redesigned config I tried to make it flexible, but looks like not flexible enough. There should be only one way to configure CSRF protection, flexible enough for all cases.
I was thinking in the past about config upgrade policy, but for now I think it is just enough to bump config version number to 2.1 and require manual changes. There are three types of configuration types:

  1. Built-in rules. No version specified.
  2. Machine level rules. The file is located in %LocalAppData%\SecurityCodeScan\config-2.0.yml on
    Windows. The version number is in the file name.
  3. Config file included into project tree with a specific name SecurityCodeScan.config.yml. The file contains a section with the version.
    One thing that has to be checked if the exception that is thrown when loading old format is not too cryptic.
    Also the documentation (located in website folder) has to be updated.

Regarding the format, if I understand correctly you need two same Name entries here because of different namespaces:

  - Name: ASP.NET Core MVC
    NameSpace: Microsoft.AspNetCore.Mvc
...
  - Name: ASP.NET Core MVC
    NameSpace: Microsoft.AspNetCore.Authorization
    AllowAnonymousAttributes:
      - AttributeName: AllowAnonymousAttribute

How about introducing an optional AttributesNameSpace for attributes and keeping everything under same parent? Like:

  - Name: ASP.NET Core MVC
    NameSpace: Microsoft.AspNetCore.Mvc
...
    AllowAnonymousAttributes:
      - AttributeName: AllowAnonymousAttribute
      - AttributeNameSpace: Microsoft.AspNetCore.Authorization
@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

AttributesNameSpace is a good idea. I'll see if I can make that change (and bump version number) sometime Monday.

SecurityCodeScan/Analyzers/CsrfTokenAnalyzer.cs Outdated Show resolved Hide resolved
SecurityCodeScan/Analyzers/XssPreventionAnalyzer.cs Outdated Show resolved Hide resolved
SecurityCodeScan/Config/Main.yml Outdated Show resolved Hide resolved
SecurityCodeScan/Config/Main.yml Show resolved Hide resolved
SecurityCodeScan/Analyzers/CsrfTokenAnalyzer.cs Outdated Show resolved Hide resolved
SecurityCodeScan/Analyzers/CsrfTokenAnalyzer.cs Outdated Show resolved Hide resolved
SecurityCodeScan/Analyzers/CsrfTokenAnalyzer.cs Outdated Show resolved Hide resolved
@JarLob

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

I'll try to add more tests to improve existing analyzer coverage.

JarLob and others added 17 commits Jul 29, 2019
…usly instead of `continue` when iterating through methods.
…attribute is proportional to the number of conditions attached to it
@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Ok, I've got all tests passing. I reworked the logic in CsrfTokenAnalyzer in an effort to make it clearer, that probably needs some real scrutiny.

JarLob added 2 commits Jul 31, 2019
Copy link
Contributor

left a comment

Good work! Just some renaming is needed I think.

SecurityCodeScan/Config/Main.yml Outdated Show resolved Hide resolved
IgnoreAttributes:
- AttributeName: Microsoft.AspNetCore.Mvc.IgnoreAntiforgeryTokenAttribute
- AttributeName: Microsoft.AspNetCore.Mvc.FromBodyAttribute
AllowAnonymousAttributes:

This comment has been minimized.

Copy link
@JarLob

JarLob Jul 31, 2019

Contributor

It works, but the name of the section AllowAnonymousAttributes looks confusing with a ApiController name under it. Not sure how to name it. AllowAnonymousAttribute hints that vulnerabilities in the controller/action doesn't matter because user identity is not used, while ApiControllerAttribute just makes actions hardly vulnerable by default. However there is IgnoreAttributes section already.

This comment has been minimized.

Copy link
@kevin-montrose

kevin-montrose Aug 1, 2019

Author Contributor

Maybe changing both VulnerableAttributes and AllowAnonymousAttributes would be clearer?

Perhaps VulnerableAttributes -> IdentityDependent and AllowAnonymousAttributes -> IdentityAgnostic?

I played around with combining Ignore and AllowAnonymous too, but they do have slightly different semantics.

kevin-montrose and others added 2 commits Aug 1, 2019
@JarLob

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

I tried to simplify the format and implementation even further, moved auditing rules into configuration itself. Please see if it fits your needs.

@JarLob

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Since all the tests pass I assume it fits your needs. Sorry that it took so many commits, but in the end I believe we have much flexible, clear and easier to maintain analyzer. Also a bug was fixed on the way :) I'm going to merge it tomorrow.

@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

I just got back from a short vacation, sorry for delay in responding.

If the test pass, it'll probably work for us. I'll spend some time today verifying.

I think the new format looks much nicer than what I had.

@kevin-montrose

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Confirming, this works for the actual Stack Overflow projects.

@JarLob JarLob marked this pull request as ready for review Aug 8, 2019
@JarLob
JarLob approved these changes Aug 8, 2019
@JarLob JarLob merged commit 56857a9 into security-code-scan:vs2017 Aug 8, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
JarLob added a commit that referenced this pull request Aug 8, 2019
* Change configuration of csrf protection to be considerably more flexible, combine Core and Mvc diagnostics using new flexibility, add conditional support, and add a test that demonstrates how to use new flexibility to check an attribute based routing style based on the Stack Overflow codebase.

* Support XSRF configurations that use an attribute to annotate actions, rather than (or in addition to) a base class.

* Cover the case when first class member is not a method.

* CSRF FromForm test added.

* CSRF test for Audit when ApiController is applied on the class.

* Few more CSRF tests to cover the cases when `return` was done erroneously instead of `continue` when iterating through methods.

* Refactor CSRF rules format
@kevin-montrose kevin-montrose deleted the kevin-montrose:vs2017-conditional-csrf-attribute branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.