From 52b889f008c310ac3bc03f56d4310559f6d5a342 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Sat, 8 Mar 2025 17:16:22 -0500 Subject: [PATCH 1/7] Support when a property is specified by a string literal instead of a `nameof` expression In earlier versions of the Razor generator, a string literal was used instead of a `nameof` expression in order to indicate the name of the property being modified. This means we need to look up the property by name instead of using a more explicit access. --- .../csharp/frameworks/microsoft/aspnetcore/Components.qll | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll index be937661b477..5fb79418c27e 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll @@ -159,7 +159,13 @@ private module JumpNodes { */ Property getParameterProperty() { result.getAnAttribute() instanceof MicrosoftAspNetCoreComponentsParameterAttribute and - exists(NameOfExpr ne | ne = this.getArgument(1) | result.getAnAccess() = ne.getAccess()) + ( + exists(NameOfExpr ne | ne = this.getArgument(1) | result.getAnAccess() = ne.getAccess()) + or + exists(string propertyName | propertyName = this.getArgument(1).(StringLiteral).getValue() | + result.hasName(propertyName) + ) + ) } /** From 3d0a85b3cd5a608e964e865dffe27d16a0f636c8 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Sat, 8 Mar 2025 17:18:15 -0500 Subject: [PATCH 2/7] Add test case using string literal in property name --- .../microsoft/aspnetcore/blazor/NameList2.cs | 50 +++++++++++++++++++ .../microsoft/aspnetcore/blazor/Xss.expected | 3 ++ .../blazor/remoteFlowSource.expected | 3 ++ 3 files changed, 56 insertions(+) create mode 100644 csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList2.cs diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList2.cs b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList2.cs new file mode 100644 index 000000000000..d27d6f2dcde9 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList2.cs @@ -0,0 +1,50 @@ +namespace VulnerableBlazorApp.Components +{ + using System.Collections.Generic; + using Microsoft.AspNetCore.Components; + + [RouteAttribute("/names2/{name?}")] + public partial class NameList2 : 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(3); + builder.AddComponentParameter(4, "TheName", name); + builder.CloseComponent(); + builder.CloseElement(); + } + builder.CloseElement(); + builder.CloseElement(); + } + + builder.OpenElement(5, "div"); + builder.OpenElement(6, "p"); + builder.AddContent(7, "Name: "); + builder.OpenComponent(8); + builder.AddComponentParameter(9, "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 Names { get; set; } = new List(); + } +} \ No newline at end of file diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.expected b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.expected index 951269f2b580..3b9a54fafb3d 100644 --- a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.expected +++ b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.expected @@ -1,12 +1,15 @@ edges +| NameList2.cs:31:57:31:60 | access to property Name : String | Name.cs:13:53:13:59 | access to property TheName | provenance | Sink:MaD:149 | | 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 | +| NameList2.cs:31:57:31:60 | access to property Name : String | semmle.label | access to property Name : String | | 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 | NameList2.cs:31:57:31:60 | 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. | NameList2.cs:31:57:31:60 | access to property Name : String | 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 | diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/remoteFlowSource.expected b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/remoteFlowSource.expected index 2a9268cf01e3..e1b3724f44d6 100644 --- a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/remoteFlowSource.expected +++ b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/remoteFlowSource.expected @@ -2,6 +2,9 @@ | 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 | +| NameList2.cs:31:57:31:60 | access to property Name | ASP.NET Core component route parameter | +| NameList2.cs:41:17:41:20 | access to property Name | ASP.NET Core component route parameter | +| NameList2.cs:43:27:43:30 | access to property Name | ASP.NET Core component route parameter | | 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 | From d601c26355f27c2079fa38fbec99199cdbc20e77 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Sat, 8 Mar 2025 17:24:49 -0500 Subject: [PATCH 3/7] [change-note] Blazor parameter passing string literal --- .../2025-03-08-blazor-parameter-passing-string-literal.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/lib/change-notes/2025-03-08-blazor-parameter-passing-string-literal.md diff --git a/csharp/ql/lib/change-notes/2025-03-08-blazor-parameter-passing-string-literal.md b/csharp/ql/lib/change-notes/2025-03-08-blazor-parameter-passing-string-literal.md new file mode 100644 index 000000000000..66ebd26f653d --- /dev/null +++ b/csharp/ql/lib/change-notes/2025-03-08-blazor-parameter-passing-string-literal.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Blazor support can now better recognize when a property being set is specified with a string literal, rather than referenced in a `nameof` expression. \ No newline at end of file From 72fb6ed078c3de87871a87a9fe8315f9eb10b4d9 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Fri, 28 Mar 2025 11:52:52 +0100 Subject: [PATCH 4/7] Restrict name based property lookup to opened component types --- .../microsoft/aspnetcore/Components.qll | 75 ++++++++++++++++++- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll index 5fb79418c27e..8cf5ce659e43 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll @@ -122,6 +122,38 @@ private class MicrosoftAspNetCoreComponentsAddComponentParameterMethod extends M } } +/** + * The `Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder::OpenComponent` method. + */ +private class MicrosoftAspNetCoreComponentsOpenComponentTComponentMethod extends Method { + MicrosoftAspNetCoreComponentsOpenComponentTComponentMethod() { + this.hasFullyQualifiedName("Microsoft.AspNetCore.Components.Rendering", "RenderTreeBuilder", + "OpenComponent`1") and + this.getNumberOfParameters() = 1 + } +} + +/** + * The `Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder::OpenComponent` method. + */ +private class MicrosoftAspNetCoreComponentsOpenComponentMethod extends Method { + MicrosoftAspNetCoreComponentsOpenComponentMethod() { + this.hasFullyQualifiedName("Microsoft.AspNetCore.Components.Rendering", "RenderTreeBuilder", + "OpenComponent") and + this.getNumberOfParameters() = 2 + } +} + +/** + * The `Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder::CloseComponent` method. + */ +private class MicrosoftAspNetCoreComponentsCloseComponentMethod extends Method { + MicrosoftAspNetCoreComponentsCloseComponentMethod() { + this.hasFullyQualifiedName("Microsoft.AspNetCore.Components.Rendering", "RenderTreeBuilder", + "CloseComponent") + } +} + private module Sources { private import semmle.code.csharp.security.dataflow.flowsources.Remote @@ -144,6 +176,38 @@ private module Sources { } } +/** + * Holds for matching `RenderTreeBuilder.OpenComponent` and `RenderTreeBuilder.CloseComponent` calls with index `openCallIndex` and `closeCallIndex` respectively + * within the `enclosing` enclosing callabale. The `componentType` is the type of the component that is being opened and closed. + */ +private predicate matchingOpenCloseComponentCalls( + MethodCall openCall, int openCallIndex, MethodCall closeCall, int closeCallIndex, + Callable enclosing, Type componentType +) { + ( + openCall.getTarget().getUnboundDeclaration() instanceof + MicrosoftAspNetCoreComponentsOpenComponentTComponentMethod and + openCall.getTarget().(ConstructedGeneric).getTypeArgument(0) = componentType + or + openCall.getTarget() instanceof MicrosoftAspNetCoreComponentsOpenComponentMethod and + openCall.getArgument(1).(TypeofExpr).getTypeAccess().getTarget() = componentType + ) and + openCall.getEnclosingCallable() = enclosing and + closeCall.getTarget() instanceof MicrosoftAspNetCoreComponentsCloseComponentMethod and + closeCall.getEnclosingCallable() = enclosing and + closeCall.getParent().getParent() = openCall.getParent().getParent() and + openCall.getParent().getIndex() = openCallIndex and + closeCall.getParent().getIndex() = closeCallIndex and + closeCallIndex > openCallIndex and + not exists(int k, MethodCall otherCloseCall | + k in [openCallIndex + 1 .. closeCallIndex - 1] and + otherCloseCall.getTarget() instanceof MicrosoftAspNetCoreComponentsCloseComponentMethod and + otherCloseCall.getEnclosingCallable() = enclosing and + otherCloseCall.getParent().getParent() = openCall.getParent().getParent() and + otherCloseCall.getParent().getIndex() = k + ) +} + private module JumpNodes { /** * A call to `Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder::AddComponentParameter` which @@ -162,8 +226,15 @@ private module JumpNodes { ( exists(NameOfExpr ne | ne = this.getArgument(1) | result.getAnAccess() = ne.getAccess()) or - exists(string propertyName | propertyName = this.getArgument(1).(StringLiteral).getValue() | - result.hasName(propertyName) + exists( + string propertyName, MethodCall openComponent, int i, MethodCall closeComponent, int j + | + propertyName = this.getArgument(1).(StringLiteral).getValue() and + result.hasName(propertyName) and + matchingOpenCloseComponentCalls(openComponent, i, closeComponent, j, + this.getEnclosingCallable(), result.getDeclaringType()) and + this.getParent().getParent() = openComponent.getParent().getParent() and + this.getParent().getIndex() in [i + 1 .. j - 1] ) ) } From 32448c14bd9253f55afc9d0e92370b598f5dc2d7 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Fri, 28 Mar 2025 13:06:36 +0100 Subject: [PATCH 5/7] Adjust expected test file --- .../all-platforms/blazor_net_8/XSS.expected | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.expected b/csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.expected index 204c31945956..dbf056053b30 100644 --- a/csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.expected +++ b/csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.expected @@ -1,8 +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/net8.0/generated/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:497:59:505:13 | call to method TypeCheck : String | provenance | Src:MaD:2 MaD:3 | +| BlazorTest/obj/Debug/net8.0/generated/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:497:59:505:13 | call to method TypeCheck : 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); ; 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/net8.0/generated/Microsoft.CodeAnalysis.Razor.Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_TestPage_razor.g.cs:497:59:505:13 | call to method TypeCheck : String | semmle.label | call to method TypeCheck : String | subpaths From 398f0414642e54ff69de04cac9aab10960daae71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tam=C3=A1s=20Vajk?= Date: Tue, 1 Apr 2025 10:18:09 +0200 Subject: [PATCH 6/7] Update csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll Co-authored-by: Michael Nebel --- .../frameworks/microsoft/aspnetcore/Components.qll | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll index 8cf5ce659e43..3c0a0940e067 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll @@ -197,15 +197,11 @@ private predicate matchingOpenCloseComponentCalls( closeCall.getEnclosingCallable() = enclosing and closeCall.getParent().getParent() = openCall.getParent().getParent() and openCall.getParent().getIndex() = openCallIndex and - closeCall.getParent().getIndex() = closeCallIndex and - closeCallIndex > openCallIndex and - not exists(int k, MethodCall otherCloseCall | - k in [openCallIndex + 1 .. closeCallIndex - 1] and - otherCloseCall.getTarget() instanceof MicrosoftAspNetCoreComponentsCloseComponentMethod and - otherCloseCall.getEnclosingCallable() = enclosing and - otherCloseCall.getParent().getParent() = openCall.getParent().getParent() and - otherCloseCall.getParent().getIndex() = k - ) + closeCallIndex = + min(int closeCallIndex0 | + closeCall.getParent().getIndex() = closeCallIndex0 and + closeCallIndex0 > openCallIndex + ) } private module JumpNodes { From a570a728bdf7c65749582d1489d0a5708b37763f Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Tue, 1 Apr 2025 10:29:55 +0200 Subject: [PATCH 7/7] Fix code quality --- .../microsoft/aspnetcore/Components.qll | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll index 3c0a0940e067..1fa427f4d19d 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/aspnetcore/Components.qll @@ -195,13 +195,16 @@ private predicate matchingOpenCloseComponentCalls( openCall.getEnclosingCallable() = enclosing and closeCall.getTarget() instanceof MicrosoftAspNetCoreComponentsCloseComponentMethod and closeCall.getEnclosingCallable() = enclosing and - closeCall.getParent().getParent() = openCall.getParent().getParent() and - openCall.getParent().getIndex() = openCallIndex and - closeCallIndex = - min(int closeCallIndex0 | - closeCall.getParent().getIndex() = closeCallIndex0 and - closeCallIndex0 > openCallIndex - ) + exists(BlockStmt block | + block = closeCall.getParent().getParent() and + block = openCall.getParent().getParent() and + block.getChildStmt(openCallIndex) = openCall.getParent() and + closeCallIndex = + min(int closeCallIndex0 | + block.getChildStmt(closeCallIndex0) = closeCall.getParent() and + closeCallIndex0 > openCallIndex + ) + ) } private module JumpNodes { @@ -223,14 +226,17 @@ private module JumpNodes { exists(NameOfExpr ne | ne = this.getArgument(1) | result.getAnAccess() = ne.getAccess()) or exists( - string propertyName, MethodCall openComponent, int i, MethodCall closeComponent, int j + string propertyName, MethodCall openComponent, BlockStmt block, int openIdx, int closeIdx, + int thisIdx | propertyName = this.getArgument(1).(StringLiteral).getValue() and result.hasName(propertyName) and - matchingOpenCloseComponentCalls(openComponent, i, closeComponent, j, + matchingOpenCloseComponentCalls(openComponent, openIdx, _, closeIdx, this.getEnclosingCallable(), result.getDeclaringType()) and - this.getParent().getParent() = openComponent.getParent().getParent() and - this.getParent().getIndex() in [i + 1 .. j - 1] + block = this.getParent().getParent() and + block = openComponent.getParent().getParent() and + block.getChildStmt(thisIdx) = this.getParent() and + thisIdx in [openIdx + 1 .. closeIdx - 1] ) ) }