From 48b90b28c70dd19d228d01f56fb03679f4846b62 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 00:01:08 -0500 Subject: [PATCH 01/11] Component parameter passing step --- .../microsoft/aspnetcore/Components.qll | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) 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 6e37fc0480fb..806ac7fa9030 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 @@ -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 @@ -133,3 +143,42 @@ 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().(MemberAccess) + ) + } + + /** + * Gets the value being set. + */ + Expr getParameterValue() { result = this.getArgument(2) } + } + + private class ComponentParameterJump extends DataFlow::NonLocalJumpNode { + ParameterPassingCall call; + + ComponentParameterJump() { this.asExpr() = call.getParameterValue() } + + override DataFlow::Node getAJumpSuccessor(boolean preservesValue) { + preservesValue = false and + result.asExpr() = call.getParameterProperty().getAnAccess() + } + } +} + From 0463f48565a95b0f43ac331db02a65889d9f98ee Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 00:37:33 -0500 Subject: [PATCH 02/11] Add Name and NameList test classes --- .../microsoft/aspnetcore/blazor/Name.cs | 22 ++++++++ .../microsoft/aspnetcore/blazor/NameList.cs | 50 +++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Name.cs create mode 100644 csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList.cs diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Name.cs b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Name.cs new file mode 100644 index 000000000000..a9d098470e44 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Name.cs @@ -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; } + } +} \ No newline at end of file diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList.cs b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList.cs new file mode 100644 index 000000000000..ceffb35303e5 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/NameList.cs @@ -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(3); + builder.AddComponentParameter(4, nameof(VulnerableBlazorApp.Components.Name.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, 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 Names { get; set; } = new List(); + } +} \ No newline at end of file From 17da291910fe6838f14002470fa2daffb3e7a430 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 00:50:19 -0500 Subject: [PATCH 03/11] fixup! Component parameter passing step --- .../frameworks/microsoft/aspnetcore/Components.qll | 9 +++++++-- 1 file changed, 7 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 806ac7fa9030..f468487498cc 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 @@ -172,11 +172,16 @@ private module JumpNodes { private class ComponentParameterJump extends DataFlow::NonLocalJumpNode { ParameterPassingCall call; + Property prop; - ComponentParameterJump() { this.asExpr() = call.getParameterValue() } + ComponentParameterJump() { + prop = call.getParameterProperty() and + // this.(DataFlowPrivate::PostUpdateNode).getPreUpdateNode().asExpr() = call.getParameterValue() + this.asExpr() = call.getParameterValue() + } override DataFlow::Node getAJumpSuccessor(boolean preservesValue) { - preservesValue = false and + preservesValue = true and result.asExpr() = call.getParameterProperty().getAnAccess() } } From 824b182ca5de47e9f7c807fa9112b3200b75484e Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 00:50:42 -0500 Subject: [PATCH 04/11] fixup! Add Name and NameList test classes --- .../microsoft/aspnetcore/blazor/remoteFlowSource.expected | 2 ++ 1 file changed, 2 insertions(+) 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 2c845e8e4001..fc334e8885a3 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,3 +2,5 @@ | 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:33:17:33:20 | access to property Name | ASP.NET Core component route parameter | +| NameList.cs:35:27:35:30 | access to property Name | ASP.NET Core component route parameter | From 97e00ae053c6b64d33a7589ec0850ced9b1a53ed Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 00:58:15 -0500 Subject: [PATCH 05/11] Fix formatting --- .../code/csharp/frameworks/microsoft/aspnetcore/Components.qll | 1 - 1 file changed, 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 f468487498cc..e9e2d1d2debf 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 @@ -186,4 +186,3 @@ private module JumpNodes { } } } - From 8ea697486859cce94b46d2802e7ed472754d2228 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 00:59:51 -0500 Subject: [PATCH 06/11] XSS qlref --- .../frameworks/microsoft/aspnetcore/blazor/Xss.qlref | 1 + 1 file changed, 1 insertion(+) create mode 100644 csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.qlref diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.qlref b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.qlref new file mode 100644 index 000000000000..faad1d6403c1 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.qlref @@ -0,0 +1 @@ +Security Features/CWE-079/XSS.ql \ No newline at end of file From 22e958b24566f70c8d7677408368aef9ade6c7b7 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 01:08:45 -0500 Subject: [PATCH 07/11] Fix jump node by using associated property --- .../code/csharp/frameworks/microsoft/aspnetcore/Components.qll | 2 +- 1 file changed, 1 insertion(+), 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 e9e2d1d2debf..045e8aaf6719 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 @@ -182,7 +182,7 @@ private module JumpNodes { override DataFlow::Node getAJumpSuccessor(boolean preservesValue) { preservesValue = true and - result.asExpr() = call.getParameterProperty().getAnAccess() + result.asExpr() = prop.getAnAccess() } } } From 133c6fa40048132ba120bee6718d43f5603242d6 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 01:09:19 -0500 Subject: [PATCH 08/11] Fix test expectations --- .../microsoft/aspnetcore/blazor/Xss.expected | 12 ++++++++++++ .../aspnetcore/blazor/remoteFlowSource.expected | 5 +++-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.expected 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 new file mode 100644 index 000000000000..951269f2b580 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/microsoft/aspnetcore/blazor/Xss.expected @@ -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 | 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 fc334e8885a3..2a9268cf01e3 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,5 +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:33:17:33:20 | access to property Name | ASP.NET Core component route parameter | -| NameList.cs:35:27:35: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 a0fe7d6a1a72c317732c7d1569bfe2d8ab990137 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 11:04:41 -0500 Subject: [PATCH 09/11] Remove unused line --- .../code/csharp/frameworks/microsoft/aspnetcore/Components.qll | 1 - 1 file changed, 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 045e8aaf6719..d5782b26851a 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 @@ -176,7 +176,6 @@ private module JumpNodes { ComponentParameterJump() { prop = call.getParameterProperty() and - // this.(DataFlowPrivate::PostUpdateNode).getPreUpdateNode().asExpr() = call.getParameterValue() this.asExpr() = call.getParameterValue() } From e2f0a61f89725ff5693c64761f504e6356743112 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 12:40:02 -0500 Subject: [PATCH 10/11] Add XSS test to integration tests --- csharp/ql/integration-tests/all-platforms/blazor/XSS.qlref | 1 + .../all-platforms/blazor_build_mode_none/XSS.qlref | 1 + csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.qlref | 1 + 3 files changed, 3 insertions(+) create mode 100644 csharp/ql/integration-tests/all-platforms/blazor/XSS.qlref create mode 100644 csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/XSS.qlref create mode 100644 csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.qlref diff --git a/csharp/ql/integration-tests/all-platforms/blazor/XSS.qlref b/csharp/ql/integration-tests/all-platforms/blazor/XSS.qlref new file mode 100644 index 000000000000..faad1d6403c1 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/blazor/XSS.qlref @@ -0,0 +1 @@ +Security Features/CWE-079/XSS.ql \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/XSS.qlref b/csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/XSS.qlref new file mode 100644 index 000000000000..faad1d6403c1 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/XSS.qlref @@ -0,0 +1 @@ +Security Features/CWE-079/XSS.ql \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.qlref b/csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.qlref new file mode 100644 index 000000000000..faad1d6403c1 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/blazor_net_8/XSS.qlref @@ -0,0 +1 @@ +Security Features/CWE-079/XSS.ql \ No newline at end of file From ca14c5722d1d2560be60f5c470e144b0340df593 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 5 Mar 2025 12:40:26 -0500 Subject: [PATCH 11/11] Add likely XSS case to integration tests --- .../blazor/BlazorTest/Components/Pages/TestPage.razor | 4 ++++ .../BlazorTest/Components/Pages/TestPage.razor | 4 ++++ .../blazor_net_8/BlazorTest/Components/Pages/TestPage.razor | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/csharp/ql/integration-tests/all-platforms/blazor/BlazorTest/Components/Pages/TestPage.razor b/csharp/ql/integration-tests/all-platforms/blazor/BlazorTest/Components/Pages/TestPage.razor index 39238d724298..ac3ccbe19207 100644 --- a/csharp/ql/integration-tests/all-platforms/blazor/BlazorTest/Components/Pages/TestPage.razor +++ b/csharp/ql/integration-tests/all-platforms/blazor/BlazorTest/Components/Pages/TestPage.razor @@ -81,6 +81,10 @@ +
+ +
+ @code { public class Container diff --git a/csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/BlazorTest/Components/Pages/TestPage.razor b/csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/BlazorTest/Components/Pages/TestPage.razor index 39238d724298..ac3ccbe19207 100644 --- a/csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/BlazorTest/Components/Pages/TestPage.razor +++ b/csharp/ql/integration-tests/all-platforms/blazor_build_mode_none/BlazorTest/Components/Pages/TestPage.razor @@ -81,6 +81,10 @@ +
+ +
+ @code { public class Container diff --git a/csharp/ql/integration-tests/all-platforms/blazor_net_8/BlazorTest/Components/Pages/TestPage.razor b/csharp/ql/integration-tests/all-platforms/blazor_net_8/BlazorTest/Components/Pages/TestPage.razor index 39238d724298..ac3ccbe19207 100644 --- a/csharp/ql/integration-tests/all-platforms/blazor_net_8/BlazorTest/Components/Pages/TestPage.razor +++ b/csharp/ql/integration-tests/all-platforms/blazor_net_8/BlazorTest/Components/Pages/TestPage.razor @@ -81,6 +81,10 @@ +
+ +
+ @code { public class Container