C#: Add flow steps for View calls refering to Razor pages by joefarebrother · Pull Request #14343 · github/codeql

joefarebrother · 2023-09-29T16:51:58Z

Adds a flow step from the model parameter of a View call to the Razor page it refers to using this view discovery system, when this can be determined statically.

michaelnebel

Very nice work @joefarebrother !

hvitved · 2023-10-27T11:48:14Z

+    result = this.getNameArgument()
+    or
+    not exists(this.getNameArgument()) and
+    result = this.getActionMethod().getName()


Can action methods be generic? If so, we will probably need to strip away the <,...,> suffix.

According to this SO question, action methods cannot be generic.

tamasvajk

Added some comments/questions mostly related to "MVC" vs "Razor Pages".

Also, a more general question:
Wouldn't we need to model flow from View("path", model) to an ExecuteAsync invocation of an instance of the page that has "path" after its Model property has been set to model?

tamasvajk · 2023-10-30T10:19:14Z

+  /**
+   * Gets the filepath of the source file that this class was generated from,
+   * relative to the application root.
+   */
+  string getSourceFilepath() { result = attr.getArgument(2).(StringLiteral).getValue() }


I think we should remove the "relative to the application root" from the QLDoc. In case of standalone extraction the path is absolute. This probably also means that some improvements would be needed for matching the relative path in a View call and the absolute path in RazorCompiledItemAttribute.

Do you have an example of a database for which the generated files contain absolute paths?

Any standalone extracted web project will have that. So running the following produces one:

dotnet new mvc --name abc CODEQL_EXTRACTOR_CSHARP_STANDALONE_EXTRACT_WEB_VIEWS=true codeql database create DB_standalone --language=csharp --source-root=abc -Obuildless=true

The DB contains A/DB_standalone/[...]/DB_standalone/working/razor/907F0B60B9944B50BE8465FB6805A19A/Microsoft.NET.Sdk.Razor.SourceGenerators/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/[...]_abc_Views_Home_Index_cshtml.g.cs which has the attribute with the absolute path:

[assembly: global::Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemAttribute(typeof(AspNetCoreGeneratedDocument.[...]_abc_Views_Home_Index), @"mvc.1.0.view", @"[...]/abc/Views/Home/Index.cshtml")]

tamasvajk · 2023-10-30T10:41:35Z

+---
+category: minorAnalysis
+---
+* Modelled additional flow steps to track flow from a `View` call in an MVC controller to the corresponding Razor page, which may result in additional results for queries such as `cs/web/xss`.


I would avoid using the term "Razor page". "Razor Pages" is a product name within the ASP.NET Core world. See https://learn.microsoft.com/en-us/aspnet/core/razor-pages/.

What term should be used instead?

How about "Razor view", "Razor view file", or simply "view file"? The latter seems to be used in the docs.

tamasvajk · 2023-10-30T10:55:29Z

+}
+
+/** A compiler-generated Razor page. */
+class RazorPage extends Class {


Same comment as below, I would avoid using "Razor page". Also, I would call a .cshtml or .razor file a RazorPage, which is not the case here. Should we make the naming a bit more explicit to show that this is the generated RazorPageClass?

Also, should we follow model the class hierarchy used by ASP.NET Core? See the comment on the charpred.

tamasvajk · 2023-11-15T10:05:27Z

+    attr.getFile() = this.getFile() and
+    attr.getType()
+        .hasQualifiedName("Microsoft.AspNetCore.Razor.Hosting", "RazorCompiledItemAttribute")


Would looking for an RazorCompiledItemMetadataAttribute targeting the class instead work? So something like

attr.getTarget() = this and attr.getType() .hasQualifiedName("Microsoft.AspNetCore.Razor.Hosting", "RazorCompiledItemMetadataAttribute")

I have tried that at some point, and it seemed to not work due to that attribute not appearing in (non-standalone) DBs.

That's odd and unfortunate. I guess it could also depend on the dotnet version being used. I'm somewhat worried that #14792 doesn't fix the assembly attribute extraction, and then we wouldn't find the RazorCompiledItemAttribute attributes.

In my integration tests, the RazorCompiledItemAttribute attributes are successfully found.

+// import PathGraph // exclude query predicates with output dependant on the absolute filepath the tests are run in
+from XssNode source, XssNode sink, string message
+where xssFlow(source, sink, message)
+select sink, source, sink, "$@ flows to here and " + message, source, "User-provided value"


tamasvajk

LGTM. Let's wait for feedback from the others too.

Have you checked the new alerts in tobyash86/WebGoat.NET, are those true positive new findings?

joefarebrother · 2023-11-22T13:53:04Z

Yes, the new results in tobyash86/WebGoat.NET look like TPs.

michaelnebel

LGTM! 👍

joefarebrother · 2023-11-23T09:52:42Z

Should I worry about the failing integration tests? They don't appear to be related to these changes, but I've tied rerunning them a few times.

michaelnebel · 2023-11-23T10:05:45Z

Should I worry about the failing integration tests? They don't appear to be related to these changes, but I've tied rerunning them a few times.

Yeah, we need them to pass.
Try and rebase again - they were just fixed a couple of days ago.

…ml files.

…eOptions

…e to RazorPageClass

…ute paths

github-actions Bot added the C# label Sep 29, 2023

joefarebrother force-pushed the csharp-xss-flow-step branch from db1e1e1 to f3b5d34 Compare October 23, 2023 20:22

joefarebrother mentioned this pull request Oct 24, 2023

Shared: Add library for filepath normalization #14500

Merged

github-actions Bot added the documentation label Oct 24, 2023

github-advanced-security AI found potential problems Oct 24, 2023

View reviewed changes

Comment thread csharp/ql/lib/semmle/code/csharp/security/dataflow/XSSFlowSteps.qll Fixed

joefarebrother marked this pull request as ready for review October 24, 2023 16:26

joefarebrother requested a review from a team as a code owner October 24, 2023 16:26

joefarebrother changed the title ~~[Draft] C#: Add XSS flow steps for View calls refering to Razor pages~~ C#: Add XSS flow steps for View calls refering to Razor pages Oct 24, 2023

joefarebrother changed the title ~~C#: Add XSS flow steps for View calls refering to Razor pages~~ C#: Add flow steps for View calls refering to Razor pages Oct 26, 2023

hvitved mentioned this pull request Oct 27, 2023

C#: Include AttributeUsages in stub generator #14609

Merged

michaelnebel self-requested a review October 27, 2023 07:21

michaelnebel reviewed Oct 27, 2023

View reviewed changes

Comment thread csharp/ql/test/query-tests/Security Features/CWE-079/XSSRazorPages/gen_files.py

michaelnebel reviewed Oct 27, 2023

View reviewed changes

hvitved reviewed Oct 27, 2023

View reviewed changes

joefarebrother force-pushed the csharp-xss-flow-step branch from ce47027 to db82c52 Compare October 27, 2023 13:46

tamasvajk reviewed Oct 30, 2023

View reviewed changes

michaelnebel reviewed Nov 1, 2023

View reviewed changes

Comment thread csharp/ql/test/query-tests/Security Features/CWE-079/XSSRazorPages/gen_files.py Outdated

github-advanced-security AI found potential problems Nov 7, 2023

View reviewed changes

Comment thread csharp/ql/lib/semmle/code/csharp/frameworks/Razor.qll Fixed

tamasvajk reviewed Nov 15, 2023

View reviewed changes

github-advanced-security AI found potential problems Nov 15, 2023

View reviewed changes

joefarebrother force-pushed the csharp-xss-flow-step branch from e4bffe7 to 89df162 Compare November 16, 2023 10:08

tamasvajk approved these changes Nov 22, 2023

View reviewed changes

michaelnebel reviewed Nov 23, 2023

View reviewed changes

joefarebrother added 4 commits November 23, 2023 10:56

Implement xss flow step for absolute filepath case

40a7223

Add relative filepath lookup

12a579e

Unit tests - Write script to aid generating necessary code from .csht…

ac3f642

…ml files.

Add unit tests + fix issue in stubs

4f5ecb8

joefarebrother added 24 commits November 23, 2023 10:56

Add support for view locations defined in code through RazoeViewEngin…

f84b2a9

…eOptions

Add additional test cases

7691cbc

Add areas

7194113

Use shared filepath normalization libary

f1b0f1a

Fix tests

7bd7cc5

Add tests for area cases

f2c3d83

Separate area view discovery list for increased precision

826111d

Add change note

7371751

Update qldoc

0ed7b3c

Make the additional flow steps generally applicible to all queries

047f8e4

Review suggestions - make import private and update change note

2416040

Remove unnecessary check for the name parameter as parameter 1

ef15980

Review suggestions - Remove unneeded import in tests, rename RazorPag…

96bddde

…e to RazorPageClass

Fix issue in genfiles.py + add help text

052166f

Convert flow steps to value steps

9af44ed

Remove AdditionalTaintStep (redundant with NonLocalJumpNode)

7a098dd

Remove redundant import

e2e4642

Minor refactoring

26c048a

Handle standalone extraction case in which generated files list absol…

82fbae3

…ute paths

Add integration tests

33186ac

Fix standalone tests

aa3fd6a

Rename Razor Page class to Razor View class

f24c042

Update to hasFullyQualifiedName

e4edb19

Fix integration tests for windows

befb1cc

joefarebrother force-pushed the csharp-xss-flow-step branch from 7ef3af4 to befb1cc Compare November 23, 2023 10:56

joefarebrother merged commit 561b769 into github:main Nov 23, 2023

Conversation

joefarebrother commented Sep 29, 2023

Uh oh!

Uh oh!

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check warning

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

joefarebrother commented Nov 22, 2023

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

joefarebrother commented Nov 23, 2023

Uh oh!

michaelnebel commented Nov 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants