Skip to content

C#: Blazor: Add non-local jump node for parameter passing #19122

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ public IEnumerable<string> RunSourceGenerator(IEnumerable<string> additionalFile
{
try
{
var relativePathToCsProj = Path.GetRelativePath(sourceDir, csprojFile);
var relativePathToCsProj = Path.GetRelativePath(sourceDir, csprojFile)
.Replace('\\', '/'); // Ensure we're generating the same hash regardless of the OS
var name = FileUtils.ComputeHash($"{relativePathToCsProj}\n{this.GetType().Name}");
using var tempDir = new TemporaryDirectory(Path.Join(FileUtils.GetTemporaryWorkingDirectory(out _), "source-generator"), "source generator temporary", logger);
var analyzerConfigPath = Path.Combine(tempDir.DirInfo.FullName, $"{name}.txt");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@
<MyOutput Value="@InputValue6" />
</div>

<div>
<MyOutput Value="@QueryParam" />
</div>

@code {

public class Container
Expand Down
18 changes: 18 additions & 0 deletions csharp/ql/integration-tests/all-platforms/blazor/XSS.expected
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#select
| BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | User-provided value |
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | User-provided value |
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | User-provided value |
edges
| BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | BlazorTest/obj/Debug/net9.0/generated/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:569:16:577:13 | call to method TypeCheck<String> : String | provenance | Src:MaD:2 MaD:3 |
| BlazorTest/obj/Debug/net9.0/generated/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:569:16:577:13 | call to method TypeCheck<String> : String | BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | provenance | Sink:MaD:1 |
models
| 1 | Sink: Microsoft.AspNetCore.Components; MarkupString; false; MarkupString; (System.String); ; Argument[0]; html-injection; manual |
| 2 | Source: Microsoft.AspNetCore.Components; SupplyParameterFromQueryAttribute; false; ; ; Attribute.Getter; ReturnValue; remote; manual |
| 3 | Summary: Microsoft.AspNetCore.Components.CompilerServices; RuntimeHelpers; false; TypeCheck<T>; (T); ; Argument[0]; ReturnValue; value; manual |
nodes
| BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | semmle.label | access to property Value |
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | semmle.label | access to property UrlParam |
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | semmle.label | access to property QueryParam |
| BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | semmle.label | access to property QueryParam : String |
| BlazorTest/obj/Debug/net9.0/generated/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:569:16:577:13 | call to method TypeCheck<String> : String | semmle.label | call to method TypeCheck<String> : String |
subpaths
2 changes: 2 additions & 0 deletions csharp/ql/integration-tests/all-platforms/blazor/XSS.qlref
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: Security Features/CWE-079/XSS.ql
postprocess: utils/test/PrettyPrintModels.ql
Comment on lines +1 to +2

Check warning

Code scanning / CodeQL

Query test without inline test expectations Warning

Query test does not use inline test expectations.
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@
<MyOutput Value="@InputValue6" />
</div>

<div>
<MyOutput Value="@QueryParam" />
</div>

@code {

public class Container
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#select
| BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | User-provided value |
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | User-provided value |
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | User-provided value |
edges
| BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | test-db/working/razor/AC613014E59A413B9538FF8068364499/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:569:16:577:13 | call to method TypeCheck<String> : String | provenance | Src:MaD:2 MaD:3 |
| test-db/working/razor/AC613014E59A413B9538FF8068364499/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:569:16:577:13 | call to method TypeCheck<String> : String | BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | provenance | Sink:MaD:1 |
models
| 1 | Sink: Microsoft.AspNetCore.Components; MarkupString; false; MarkupString; (System.String); ; Argument[0]; html-injection; manual |
| 2 | Source: Microsoft.AspNetCore.Components; SupplyParameterFromQueryAttribute; false; ; ; Attribute.Getter; ReturnValue; remote; manual |
| 3 | Summary: Microsoft.AspNetCore.Components.CompilerServices; RuntimeHelpers; false; TypeCheck<T>; (T); ; Argument[0]; ReturnValue; value; manual |
nodes
| BlazorTest/Components/MyOutput.razor:5:53:5:57 | access to property Value | semmle.label | access to property Value |
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | semmle.label | access to property UrlParam |
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | semmle.label | access to property QueryParam |
| BlazorTest/Components/Pages/TestPage.razor:85:23:85:32 | access to property QueryParam : String | semmle.label | access to property QueryParam : String |
| test-db/working/razor/AC613014E59A413B9538FF8068364499/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:569:16:577:13 | call to method TypeCheck<String> : String | semmle.label | call to method TypeCheck<String> : String |
subpaths
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: Security Features/CWE-079/XSS.ql
postprocess: utils/test/PrettyPrintModels.ql
Comment on lines +1 to +2

Check warning

Code scanning / CodeQL

Query test without inline test expectations Warning

Query test does not use inline test expectations.
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@
<MyOutput Value="@InputValue6" />
</div>

<div>
<MyOutput Value="@QueryParam" />
</div>

@code {

public class Container
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#select
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | User-provided value |
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | User-provided value |
edges
nodes
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | semmle.label | access to property UrlParam |
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | semmle.label | access to property QueryParam |
subpaths
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: Security Features/CWE-079/XSS.ql
postprocess: utils/test/PrettyPrintModels.ql
Comment on lines +1 to +2

Check warning

Code scanning / CodeQL

Query test without inline test expectations Warning

Query test does not use inline test expectations.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Modeled parameter passing between Blazor parent and child components.
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,16 @@ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) }
pragma[inline]
predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) }

/**
* A module importing the modules that provide non local jump node declarations,
* ensuring that they are visible to the taint tracking / data flow library.
*/
private module JumpNodes {
private import semmle.code.csharp.frameworks.microsoft.aspnetcore.Components
private import semmle.code.csharp.frameworks.Razor
private import semmle.code.csharp.frameworks.NHibernate
}

/**
* A data flow node that jumps between callables. This can be extended in
* framework code to add additional data flow steps.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,16 @@ class MicrosoftAspNetCoreComponentsComponent extends Class {
}
}

/**
* The `Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder::AddComponentParameter` method.
*/
private class MicrosoftAspNetCoreComponentsAddComponentParameterMethod extends Method {
MicrosoftAspNetCoreComponentsAddComponentParameterMethod() {
this.hasFullyQualifiedName("Microsoft.AspNetCore.Components.Rendering", "RenderTreeBuilder",
"AddComponentParameter")
}
}

private module Sources {
private import semmle.code.csharp.security.dataflow.flowsources.Remote

Expand All @@ -133,3 +143,44 @@ private module Sources {
override string getSourceType() { result = "ASP.NET Core component route parameter" }
}
}

private module JumpNodes {
/**
* A call to `Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder::AddComponentParameter` which
* sets the value of a parameter.
*/
private class ParameterPassingCall extends Call {
ParameterPassingCall() {
this.getTarget() instanceof MicrosoftAspNetCoreComponentsAddComponentParameterMethod
}

/**
* Gets the property whose value is being set.
*/
Property getParameterProperty() {
result.getAnAttribute() instanceof MicrosoftAspNetCoreComponentsParameterAttribute and
exists(NameOfExpr ne | ne = this.getArgument(1) | result.getAnAccess() = ne.getAccess())
}

/**
* Gets the value being set.
*/
Expr getParameterValue() { result = this.getArgument(2) }
}

private class ComponentParameterJump extends DataFlow::NonLocalJumpNode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NonLocalJumpNode is a bi-directional imported abstract class, but this is not that evident. I think it makes sense to explicitly make a private import module above the NonLocalJumpNode declaration that imports all file modules that extends the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the below?

--- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
+++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
@@ -147,6 +147,10 @@ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) }
 pragma[inline]
 predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) }
 
+private import semmle.code.csharp.frameworks.microsoft.aspnetcore.Components
+private import semmle.code.csharp.frameworks.Razor
+private import semmle.code.csharp.frameworks.NHibernate
+
 /**
  * A data flow node that jumps between callables. This can be extended in
  * framework code to add additional data flow steps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and just in a private module.

/**
 * A module importing the modules that provide non local jump node declarations,
 * ensuring that they are visible to the taint tracking / data flow library.
 */
private module JumpNodes {
  private import semmle.code.csharp.frameworks.microsoft.aspnetcore.Components
  private import semmle.code.csharp.frameworks.Razor
  private import semmle.code.csharp.frameworks.NHibernate
}

Property prop;

ComponentParameterJump() {
exists(ParameterPassingCall call |
prop = call.getParameterProperty() and
this.asExpr() = call.getParameterValue()
)
}

override DataFlow::Node getAJumpSuccessor(boolean preservesValue) {
preservesValue = true and
result.asExpr() = prop.getAnAccess()
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
namespace VulnerableBlazorApp.Components
{
using Microsoft.AspNetCore.Components;

public partial class Name : Microsoft.AspNetCore.Components.ComponentBase
{
protected override void BuildRenderTree(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder builder)
{
if (TheName is not null)
{
builder.OpenElement(0, "div");
builder.OpenElement(1, "p");
builder.AddContent(2, (MarkupString)TheName);
builder.CloseElement();
builder.CloseElement();
}
}

[Parameter]
public string TheName { get; set; }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
namespace VulnerableBlazorApp.Components
{
using System.Collections.Generic;
using Microsoft.AspNetCore.Components;

[RouteAttribute("/names/{name?}")]
public partial class NameList : Microsoft.AspNetCore.Components.ComponentBase
{
protected override void BuildRenderTree(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder builder)
{
if (Names is not null)
{
builder.OpenElement(0, "div");
builder.OpenElement(1, "ul");
foreach (var name in Names)
{
builder.OpenElement(2, "li");
builder.OpenComponent<VulnerableBlazorApp.Components.Name>(3);
builder.AddComponentParameter(4, nameof(VulnerableBlazorApp.Components.Name.TheName), name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the jump step ensures that we have data flow from name into the property VulnerableBlazorApp.Components.Name.TheName?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's flow from name to the property reads of VulnerableBlazorApp.Components.Name.TheName.

builder.CloseComponent();
builder.CloseElement();
}
builder.CloseElement();
builder.CloseElement();
}

builder.OpenElement(5, "div");
builder.OpenElement(6, "p");
builder.AddContent(7, "Name: ");
builder.OpenComponent<VulnerableBlazorApp.Components.Name>(8);
builder.AddComponentParameter(9, nameof(VulnerableBlazorApp.Components.Name.TheName), Name);
builder.CloseComponent();
builder.CloseElement();
}

[Parameter]
public string Name { get; set; }

protected override void OnParametersSet()
{
if (Name is not null)
{
Names.Add(Name);
}
}


public List<string> Names { get; set; } = new List<string>();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
edges
| NameList.cs:31:99:31:102 | access to property Name : String | Name.cs:13:53:13:59 | access to property TheName | provenance | Sink:MaD:149 |
nodes
| Components_Pages_TestPage_razor.g.cs:138:15:138:22 | access to property UrlParam | semmle.label | access to property UrlParam |
| Components_Pages_TestPage_razor.g.cs:188:18:188:27 | access to property QueryParam | semmle.label | access to property QueryParam |
| Name.cs:13:53:13:59 | access to property TheName | semmle.label | access to property TheName |
| NameList.cs:31:99:31:102 | access to property Name : String | semmle.label | access to property Name : String |
subpaths
#select
| Components_Pages_TestPage_razor.g.cs:138:15:138:22 | access to property UrlParam | Components_Pages_TestPage_razor.g.cs:138:15:138:22 | access to property UrlParam | Components_Pages_TestPage_razor.g.cs:138:15:138:22 | access to property UrlParam | $@ flows to here and is written to HTML or JavaScript. | Components_Pages_TestPage_razor.g.cs:138:15:138:22 | access to property UrlParam | User-provided value |
| Components_Pages_TestPage_razor.g.cs:188:18:188:27 | access to property QueryParam | Components_Pages_TestPage_razor.g.cs:188:18:188:27 | access to property QueryParam | Components_Pages_TestPage_razor.g.cs:188:18:188:27 | access to property QueryParam | $@ flows to here and is written to HTML or JavaScript. | Components_Pages_TestPage_razor.g.cs:188:18:188:27 | access to property QueryParam | User-provided value |
| Name.cs:13:53:13:59 | access to property TheName | NameList.cs:31:99:31:102 | access to property Name : String | Name.cs:13:53:13:59 | access to property TheName | $@ flows to here and is written to HTML or JavaScript. | NameList.cs:31:99:31:102 | access to property Name : String | User-provided value |
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Security Features/CWE-079/XSS.ql

Check warning

Code scanning / CodeQL

Query test without inline test expectations Warning test

Query test does not use inline test expectations.
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,6 @@
| Components_Pages_TestPage_razor.g.cs:138:15:138:22 | access to property UrlParam | ASP.NET Core component route parameter |
| Components_Pages_TestPage_razor.g.cs:176:1:176:10 | access to property QueryParam | external |
| Components_Pages_TestPage_razor.g.cs:188:18:188:27 | access to property QueryParam | external |
| NameList.cs:31:99:31:102 | access to property Name | ASP.NET Core component route parameter |
| NameList.cs:41:17:41:20 | access to property Name | ASP.NET Core component route parameter |
| NameList.cs:43:27:43:30 | access to property Name | ASP.NET Core component route parameter |