From 20c1033079e0d882ac7e38db0f886d50691fd47e Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:33:07 -0400 Subject: [PATCH 1/6] C#: Fix cs/web/xss false positive on Razor tag-helper attribute values Razor's source generator brackets WriteLiteral calls that populate an HTML attribute value on a tag-helper-enabled element (e.g. sp-for) between BeginWriteTagHelperAttribute()/EndWriteTagHelperAttribute() calls. The captured text is buffered internally and HTML-attribute- encoded before being rendered, so it is not a real XSS sink, but cs/web/xss previously flagged it as one. - Add getBeginWriteTagHelperAttributeMethod() / getEndWriteTagHelperAttributeMethod() to MicrosoftAspNetCoreMvcRazorPageBase. - Add isBracketedForTagHelperAttribute() to Html.qll and use it to exclude bracketed WriteLiteral calls from MicrosoftAspNetRazorPageWriteLiteralSink, using same-basic-block, immediately-adjacent-bracket matching so unrelated bracket pairs cannot "adopt" an unbracketed call. - Add RazorTagHelperAttribute.cshtml(.g.cs) test coverage: a suppressed bracketed write, an unbracketed positive control, two independent brackets in one basic block, and a bare write sandwiched between brackets. - Add change note (majorAnalysis). Diagnosed and validated against the real customer reproduction that motivated this fix (field-security-codeql#257 / #261): the false positive (ChangeAccountInfo.cshtml) is no longer reported, while the genuine Html.Raw-based positive control (ProfileSummary.cshtml) remains reported. Full CWE-079 test suite (6 tests) passes with no regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...09-18-razor-tag-helper-attribute-xss-fp.md | 4 + .../frameworks/microsoft/AspNetCore.qll | 10 +++ .../security/dataflow/flowsinks/Html.qll | 64 ++++++++++++- .../XSS/RazorTagHelperAttribute.cshtml | 1 + .../XSS/RazorTagHelperAttribute.cshtml.g.cs | 89 +++++++++++++++++++ .../CWE-079/XSS/XSS.expected | 9 ++ 6 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml.g.cs diff --git a/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md b/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md new file mode 100644 index 000000000000..72774940ee3a --- /dev/null +++ b/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md @@ -0,0 +1,4 @@ +--- +category: majorAnalysis +--- +* The `cs/web/xss` query no longer flags `WriteLiteral` calls that the Razor source generator emits for the value of an HTML attribute on an element that also has a tag helper (for example, an attribute populated via `asp-for`). Such values are captured into a string buffer by matching `BeginWriteTagHelperAttribute`/`EndWriteTagHelperAttribute` calls and are HTML-attribute-encoded before being rendered, so they are not a real cross-site scripting sink. This fixes a false positive that could previously be reported for any tainted value bound to an HTML attribute on a tag-helper-enabled element. diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll index 37b0ff2884f9..f14eb2f96fa9 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll @@ -491,6 +491,16 @@ class MicrosoftAspNetCoreMvcRazorPageBase extends Class { /** Gets the `WriteLiteral` method. */ Method getWriteLiteralMethod() { result = this.getAMethod("WriteLiteral") } + + /** Gets the `BeginWriteTagHelperAttribute` method. */ + Method getBeginWriteTagHelperAttributeMethod() { + result = this.getAMethod("BeginWriteTagHelperAttribute") + } + + /** Gets the `EndWriteTagHelperAttribute` method. */ + Method getEndWriteTagHelperAttributeMethod() { + result = this.getAMethod("EndWriteTagHelperAttribute") + } } /** A class deriving from `Microsoft.AspNetCore.Http.HttpRequest`, implements `HttpRequest` in ASP.NET Core. */ diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll index 362a993e5321..747ee2b3a11c 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll @@ -177,14 +177,74 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { } } +/** + * Holds if `writeLiteral` is a call to `RazorPageBase.WriteLiteral` whose argument is captured + * between a matching pair of `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()` + * calls on `page`, in the same basic block, with no other such calls in between. + * + * The Razor source generator emits this bracketing for every literal or expression segment of an + * HTML attribute value on an element that also carries a tag helper (for example `asp-for`). Such + * a `WriteLiteral` call does not write directly, unencoded, to the response: `WriteLiteral` + * appends to an internal string buffer, `EndWriteTagHelperAttribute()` returns that buffer, and + * the buffered text is subsequently stored as a tag helper attribute value and HTML-attribute- + * encoded when the tag helper's output is rendered. This is therefore not a real sink. + * + * Because a basic block cannot contain a branch, requiring `beginCall`, `writeLiteral`, and + * `endCall` to appear (in that order) in the same basic block, with no other + * `Begin`/`EndWriteTagHelperAttribute` call from `page` strictly between `beginCall` and + * `writeLiteral`, or between `writeLiteral` and `endCall`, guarantees that `beginCall`/`endCall` + * are the immediately enclosing bracket around `writeLiteral` on every path that reaches it (that + * is, the bracket opened by `beginCall` is still open, and not yet closed by some other `endCall`, + * at the point `writeLiteral` executes). + */ +private predicate isBracketedForTagHelperAttribute(Call writeLiteral) { + exists( + MicrosoftAspNetCoreMvcRazorPageBase page, Call beginCall, Call endCall, int i, int j, int k + | + writeLiteral = page.getWriteLiteralMethod().getACall() and + beginCall = page.getBeginWriteTagHelperAttributeMethod().getACall() and + endCall = page.getEndWriteTagHelperAttributeMethod().getACall() and + writeLiteral.getBasicBlock().getNode(i) = beginCall.getControlFlowNode() and + writeLiteral.getBasicBlock().getNode(j) = writeLiteral.getControlFlowNode() and + writeLiteral.getBasicBlock().getNode(k) = endCall.getControlFlowNode() and + i < j and + j < k and + not exists(int i2, Call other | + ( + other = page.getBeginWriteTagHelperAttributeMethod().getACall() or + other = page.getEndWriteTagHelperAttributeMethod().getACall() + ) and + writeLiteral.getBasicBlock().getNode(i2) = other.getControlFlowNode() and + i < i2 and + i2 < j + ) and + not exists(int k2, Call other | + ( + other = page.getBeginWriteTagHelperAttributeMethod().getACall() or + other = page.getEndWriteTagHelperAttributeMethod().getACall() + ) and + writeLiteral.getBasicBlock().getNode(k2) = other.getControlFlowNode() and + j < k2 and + k2 < k + ) + ) +} + /** * An expression that is used as an argument to `Page.WriteLiteral` in ASP.NET 6.0 razor page, typically in * a `.cshtml` file. + * + * `WriteLiteral` calls whose argument is captured for a tag helper attribute value (see + * `isBracketedForTagHelperAttribute`) are excluded, since such values are HTML-attribute-encoded + * later and are not written unencoded to the response. */ class MicrosoftAspNetRazorPageWriteLiteralSink extends AspNetCoreHtmlSink { MicrosoftAspNetRazorPageWriteLiteralSink() { - this.getExpr() = - any(MicrosoftAspNetCoreMvcRazorPageBase h).getWriteLiteralMethod().getACall().getAnArgument() + exists(Call writeLiteral | + writeLiteral = any(MicrosoftAspNetCoreMvcRazorPageBase h).getWriteLiteralMethod().getACall() and + this.getExpr() = writeLiteral.getAnArgument() and + not isBracketedForTagHelperAttribute(writeLiteral) + ) } } diff --git a/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml b/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml new file mode 100644 index 000000000000..8b1a393741c9 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml @@ -0,0 +1 @@ +// empty diff --git a/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml.g.cs b/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml.g.cs new file mode 100644 index 000000000000..12c2b389709a --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml.g.cs @@ -0,0 +1,89 @@ +// A hand-written test file that mimics the output of compiling a `.cshtml` file that has an +// element with a tag helper (e.g. `asp-for`) and an HTML attribute whose value contains a +// tainted expression, for example: +#pragma checksum "RazorTagHelperAttribute.cshtml" "{ff1816ec-aa5e-4d10-87f7-6f4963833460}" "c4ae76542f1958092cebd8f57beef899d20fc548" +// +#pragma warning disable 1591 +[assembly: global::Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemAttribute(typeof(dotnetweb.Pages.Pages_RazorTagHelperAttribute), @"mvc.1.0.razor-page", @"RazorTagHelperAttribute.cshtml")] +namespace dotnetweb.Pages +{ + #line hidden + using System; + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using Microsoft.AspNetCore.Mvc; + using Microsoft.AspNetCore.Mvc.Rendering; + using Microsoft.AspNetCore.Mvc.ViewFeatures; +#nullable restore +using dotnetweb; + +#line default +#line hidden +#nullable disable + [global::Microsoft.AspNetCore.Razor.Hosting.RazorSourceChecksumAttribute(@"SHA1", @"c4ae76542f1958092cebd8f57beef899d20fc548", @"RazorTagHelperAttribute.cshtml")] + public class Pages_RazorTagHelperAttribute : global::Microsoft.AspNetCore.Mvc.RazorPages.Page + { + #pragma warning disable 1998 + public async override global::System.Threading.Tasks.Task ExecuteAsync() + { +#nullable restore +#line 3 "RazorTagHelperAttribute.cshtml" + + var model = Request.Query["m"]; // $ Source=model + +#line default +#line hidden +#nullable disable + WriteLiteral(""); + + // BAD: a `WriteLiteral` call that is not bracketed by + // `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()` writes directly, + // unencoded, to the response and must still be flagged. + WriteLiteral(model); // $ Alert=model + + // GOOD: two independent tag helper attribute brackets that occur one after another + // in the same basic block must each be matched with their own nearest + // `Begin`/`EndWriteTagHelperAttribute` call, and neither should leak into the other. + BeginWriteTagHelperAttribute(); + WriteLiteral("literal-prefix-"); + var __tagHelperAttribute_2 = EndWriteTagHelperAttribute(); + BeginWriteTagHelperAttribute(); + WriteLiteral(model); + var __tagHelperAttribute_3 = EndWriteTagHelperAttribute(); + + // BAD: a bare `WriteLiteral` call sandwiched between two unrelated tag helper + // attribute brackets, in the same basic block, must not be mistaken for being + // captured by either neighboring bracket. + BeginWriteTagHelperAttribute(); + WriteLiteral("prefix"); + var __tagHelperAttribute_4 = EndWriteTagHelperAttribute(); + WriteLiteral(model); // $ Alert=model + BeginWriteTagHelperAttribute(); + WriteLiteral("suffix"); + var __tagHelperAttribute_5 = EndWriteTagHelperAttribute(); + } + #pragma warning restore 1998 + [global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute] + public global::Microsoft.AspNetCore.Mvc.ViewFeatures.IModelExpressionProvider ModelExpressionProvider { get; private set; } + [global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute] + public global::Microsoft.AspNetCore.Mvc.IUrlHelper Url { get; private set; } + [global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute] + public global::Microsoft.AspNetCore.Mvc.IViewComponentHelper Component { get; private set; } + [global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute] + public global::Microsoft.AspNetCore.Mvc.Rendering.IJsonHelper Json { get; private set; } + [global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute] + public global::Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper Html { get; private set; } + public global::Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary ViewData => (global::Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary)PageContext?.ViewData; + } +} +#pragma warning restore 1591 diff --git a/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/XSS.expected b/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/XSS.expected index fe184fdff75a..f560bb101456 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/XSS.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-079/XSS/XSS.expected @@ -1,5 +1,7 @@ #select | Index.cshtml:14:16:14:22 | call to operator implicit conversion | Index.cshtml:5:19:5:31 | access to property Query : IQueryCollection | Index.cshtml:14:16:14:22 | call to operator implicit conversion | $@ flows to here and is written to HTML or JavaScript: Microsoft.AspNetCore.Mvc.ViewFeatures.HtmlHelper.Raw() method. | Index.cshtml:5:19:5:31 | access to property Query : IQueryCollection | User-provided value | +| RazorTagHelperAttribute.cshtml.g.cs:52:26:52:30 | call to operator implicit conversion | RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | RazorTagHelperAttribute.cshtml.g.cs:52:26:52:30 | call to operator implicit conversion | $@ flows to here and is written to HTML or JavaScript: Microsoft.AspNetCore.Mvc.Razor.RazorPageBase.WriteLiteral() method. | RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | User-provided value | +| RazorTagHelperAttribute.cshtml.g.cs:70:26:70:30 | call to operator implicit conversion | RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | RazorTagHelperAttribute.cshtml.g.cs:70:26:70:30 | call to operator implicit conversion | $@ flows to here and is written to HTML or JavaScript: Microsoft.AspNetCore.Mvc.Razor.RazorPageBase.WriteLiteral() method. | RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | User-provided value | | XSSAspNet.cs:26:30:26:34 | access to local variable sayHi | XSSAspNet.cs:19:25:19:43 | access to property QueryString : NameValueCollection | XSSAspNet.cs:26:30:26:34 | access to local variable sayHi | $@ flows to here and is written to HTML or JavaScript: System.Web.WebPages.WebPage.WriteLiteral() method. | XSSAspNet.cs:19:25:19:43 | access to property QueryString : NameValueCollection | User-provided value | | XSSAspNet.cs:36:40:36:44 | access to local variable sayHi | XSSAspNet.cs:19:25:19:43 | access to property QueryString : NameValueCollection | XSSAspNet.cs:36:40:36:44 | access to local variable sayHi | $@ flows to here and is written to HTML or JavaScript: System.Web.WebPages.WebPage.WriteLiteralTo() method. | XSSAspNet.cs:19:25:19:43 | access to property QueryString : NameValueCollection | User-provided value | | XSSAspNet.cs:44:28:44:33 | access to local variable sayHi2 | XSSAspNet.cs:43:26:43:44 | access to property QueryString : NameValueCollection | XSSAspNet.cs:44:28:44:33 | access to local variable sayHi2 | $@ flows to here and is written to HTML or JavaScript. | XSSAspNet.cs:43:26:43:44 | access to property QueryString : NameValueCollection | User-provided value | @@ -13,6 +15,9 @@ edges | Index.cshtml:5:9:5:15 | access to local variable message : StringValues | Index.cshtml:14:16:14:22 | call to operator implicit conversion | provenance | | | Index.cshtml:5:19:5:31 | access to property Query : IQueryCollection | Index.cshtml:5:9:5:15 | access to local variable message : StringValues | provenance | | +| RazorTagHelperAttribute.cshtml:4:9:4:13 | access to local variable model : StringValues | RazorTagHelperAttribute.cshtml.g.cs:52:26:52:30 | call to operator implicit conversion | provenance | | +| RazorTagHelperAttribute.cshtml:4:9:4:13 | access to local variable model : StringValues | RazorTagHelperAttribute.cshtml.g.cs:70:26:70:30 | call to operator implicit conversion | provenance | | +| RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | RazorTagHelperAttribute.cshtml:4:9:4:13 | access to local variable model : StringValues | provenance | | | XSSAspNet.cs:19:17:19:21 | access to local variable sayHi : String | XSSAspNet.cs:26:30:26:34 | access to local variable sayHi | provenance | | | XSSAspNet.cs:19:17:19:21 | access to local variable sayHi : String | XSSAspNet.cs:36:40:36:44 | access to local variable sayHi | provenance | | | XSSAspNet.cs:19:25:19:43 | access to property QueryString : NameValueCollection | XSSAspNet.cs:19:17:19:21 | access to local variable sayHi : String | provenance | | @@ -49,6 +54,10 @@ nodes | Index.cshtml:5:9:5:15 | access to local variable message : StringValues | semmle.label | access to local variable message : StringValues | | Index.cshtml:5:19:5:31 | access to property Query : IQueryCollection | semmle.label | access to property Query : IQueryCollection | | Index.cshtml:14:16:14:22 | call to operator implicit conversion | semmle.label | call to operator implicit conversion | +| RazorTagHelperAttribute.cshtml.g.cs:52:26:52:30 | call to operator implicit conversion | semmle.label | call to operator implicit conversion | +| RazorTagHelperAttribute.cshtml.g.cs:70:26:70:30 | call to operator implicit conversion | semmle.label | call to operator implicit conversion | +| RazorTagHelperAttribute.cshtml:4:9:4:13 | access to local variable model : StringValues | semmle.label | access to local variable model : StringValues | +| RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | semmle.label | access to property Query : IQueryCollection | | XSSAspNet.cs:19:17:19:21 | access to local variable sayHi : String | semmle.label | access to local variable sayHi : String | | XSSAspNet.cs:19:25:19:43 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection | | XSSAspNet.cs:19:25:19:52 | access to indexer : String | semmle.label | access to indexer : String | From ace3b3c9a1150e8fff2c4caac1627c889d830ebb Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:41:57 -0400 Subject: [PATCH 2/6] Address CCR: require implicit-this receiver for bracket calls Copilot Code Review correctly flagged that isBracketedForTagHelperAttribute never related the receivers of beginCall/writeLiteral/endCall, so a bracket on one page instance could theoretically be mistaken for a bracket around a WriteLiteral call on a different page (e.g. otherPage.BeginWriteTagHelperAttribute(); this.WriteLiteral(model); otherPage.EndWriteTagHelperAttribute();). Require all three calls to have an implicit his qualifier, which is how the Razor source generator always emits them, guaranteeing they act on the same page instance. Re-verified: XSS.ql compiles, all 6 CWE-079 tests pass, and the real customer reproduction database still shows the FP suppressed and the genuine Html.Raw positive control still reported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../code/csharp/security/dataflow/flowsinks/Html.qll | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll index 747ee2b3a11c..2934c5c123de 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll @@ -196,14 +196,23 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { * are the immediately enclosing bracket around `writeLiteral` on every path that reaches it (that * is, the bracket opened by `beginCall` is still open, and not yet closed by some other `endCall`, * at the point `writeLiteral` executes). + * + * `beginCall`, `writeLiteral`, and `endCall` are additionally required to have an implicit `this` + * qualifier, which is how the Razor source generator always emits these calls. This ensures all + * three calls act on the same page instance, so a bracket on one page cannot be mistaken for a + * bracket around a `WriteLiteral` call on a different page. */ private predicate isBracketedForTagHelperAttribute(Call writeLiteral) { exists( - MicrosoftAspNetCoreMvcRazorPageBase page, Call beginCall, Call endCall, int i, int j, int k + MicrosoftAspNetCoreMvcRazorPageBase page, MethodCall beginCall, MethodCall endCall, int i, + int j, int k | writeLiteral = page.getWriteLiteralMethod().getACall() and beginCall = page.getBeginWriteTagHelperAttributeMethod().getACall() and endCall = page.getEndWriteTagHelperAttributeMethod().getACall() and + writeLiteral.(QualifiableExpr).hasImplicitThisQualifier() and + beginCall.hasImplicitThisQualifier() and + endCall.hasImplicitThisQualifier() and writeLiteral.getBasicBlock().getNode(i) = beginCall.getControlFlowNode() and writeLiteral.getBasicBlock().getNode(j) = writeLiteral.getControlFlowNode() and writeLiteral.getBasicBlock().getNode(k) = endCall.getControlFlowNode() and From 6a272fb020caa15752caf5cd20902da5151c8473 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Fri, 18 Sep 2026 16:22:41 -0400 Subject: [PATCH 3/6] Trim change note to match repo conventions Shortened to a single terse sentence, matching the depth/style of other recent change-notes (one bullet, no implementation detail), and called out the ASP.NET Core Razor Pages/MVC scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../2026-09-18-razor-tag-helper-attribute-xss-fp.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md b/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md index 72774940ee3a..266ef985cd47 100644 --- a/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md +++ b/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md @@ -1,4 +1,4 @@ --- category: majorAnalysis --- -* The `cs/web/xss` query no longer flags `WriteLiteral` calls that the Razor source generator emits for the value of an HTML attribute on an element that also has a tag helper (for example, an attribute populated via `asp-for`). Such values are captured into a string buffer by matching `BeginWriteTagHelperAttribute`/`EndWriteTagHelperAttribute` calls and are HTML-attribute-encoded before being rendered, so they are not a real cross-site scripting sink. This fixes a false positive that could previously be reported for any tainted value bound to an HTML attribute on a tag-helper-enabled element. +* Fixed a false positive in `cs/web/xss` for ASP.NET Core Razor Pages/MVC views: `WriteLiteral` calls generated for tag helper attribute values (for example, `asp-for`) are HTML-attribute-encoded before being rendered, so they are no longer treated as XSS sinks. From 459b9ffe049e21158dfeb022e54c56013e6033bf Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:59:30 -0400 Subject: [PATCH 4/6] Address CCR: don't claim guaranteed downstream encoding Copilot Code Review correctly pointed out that bracketing between BeginWriteTagHelperAttribute()/EndWriteTagHelperAttribute() only proves the value is captured into a buffer rather than written directly to the response; it does not, by itself, guarantee that every tag helper later HTML-attribute-encodes that buffer. Reworded the doc comments and the change note to justify the exclusion on "not a direct write to the response" rather than on assumed downstream encoding. No logic change; XSS.ql compiles and the CWE-079/XSS test still passes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../2026-09-18-razor-tag-helper-attribute-xss-fp.md | 2 +- .../code/csharp/security/dataflow/flowsinks/Html.qll | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md b/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md index 266ef985cd47..4cbe16158328 100644 --- a/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md +++ b/csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md @@ -1,4 +1,4 @@ --- category: majorAnalysis --- -* Fixed a false positive in `cs/web/xss` for ASP.NET Core Razor Pages/MVC views: `WriteLiteral` calls generated for tag helper attribute values (for example, `asp-for`) are HTML-attribute-encoded before being rendered, so they are no longer treated as XSS sinks. +* Fixed a false positive in `cs/web/xss` for ASP.NET Core Razor Pages/MVC views: `WriteLiteral` calls generated for tag helper attribute values (for example, `asp-for`) capture the value into an internal buffer instead of writing it directly to the response, so they are no longer treated as XSS sinks. diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll index 2934c5c123de..8434f3cd0109 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll @@ -184,10 +184,10 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { * * The Razor source generator emits this bracketing for every literal or expression segment of an * HTML attribute value on an element that also carries a tag helper (for example `asp-for`). Such - * a `WriteLiteral` call does not write directly, unencoded, to the response: `WriteLiteral` - * appends to an internal string buffer, `EndWriteTagHelperAttribute()` returns that buffer, and - * the buffered text is subsequently stored as a tag helper attribute value and HTML-attribute- - * encoded when the tag helper's output is rendered. This is therefore not a real sink. + * a `WriteLiteral` call does not write directly to the response: `WriteLiteral` appends to an + * internal string buffer, and `EndWriteTagHelperAttribute()` returns that buffer as a tag helper + * attribute value rather than as page markup. This is therefore not a direct-write sink, unlike an + * unbracketed `WriteLiteral` call, whose argument is written straight to the response. * * Because a basic block cannot contain a branch, requiring `beginCall`, `writeLiteral`, and * `endCall` to appear (in that order) in the same basic block, with no other @@ -244,8 +244,8 @@ private predicate isBracketedForTagHelperAttribute(Call writeLiteral) { * a `.cshtml` file. * * `WriteLiteral` calls whose argument is captured for a tag helper attribute value (see - * `isBracketedForTagHelperAttribute`) are excluded, since such values are HTML-attribute-encoded - * later and are not written unencoded to the response. + * `isBracketedForTagHelperAttribute`) are excluded, since such calls buffer the value as a tag + * helper attribute rather than writing it directly to the response. */ class MicrosoftAspNetRazorPageWriteLiteralSink extends AspNetCoreHtmlSink { MicrosoftAspNetRazorPageWriteLiteralSink() { From befe578e8286d06de5e027e9005275a0a3767876 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Tue, 22 Sep 2026 09:45:22 -0400 Subject: [PATCH 5/6] Simplify isBracketedForTagHelperAttribute per review feedback Consolidate the two separate not-exists checks (]i,j[ and ]j,k[) into a single check over the whole open interval ]i,k[. Since no other Begin/EndWriteTagHelperAttribute call can share writeLiteral's own control flow node index j, checking the combined interval is equivalent to checking both sides separately, but simpler. Also bind BasicBlock bb once via writeLiteral.getBasicBlock() and reuse it, instead of calling it independently for each of i, j, and k. No behavior change: all CWE-079/XSS tests still pass. --- .../security/dataflow/flowsinks/Html.qll | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll index 8434f3cd0109..3212855845fb 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll @@ -192,10 +192,12 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { * Because a basic block cannot contain a branch, requiring `beginCall`, `writeLiteral`, and * `endCall` to appear (in that order) in the same basic block, with no other * `Begin`/`EndWriteTagHelperAttribute` call from `page` strictly between `beginCall` and - * `writeLiteral`, or between `writeLiteral` and `endCall`, guarantees that `beginCall`/`endCall` - * are the immediately enclosing bracket around `writeLiteral` on every path that reaches it (that - * is, the bracket opened by `beginCall` is still open, and not yet closed by some other `endCall`, - * at the point `writeLiteral` executes). + * `endCall`, guarantees that `beginCall`/`endCall` are the immediately enclosing bracket around + * `writeLiteral` on every path that reaches it (that is, the bracket opened by `beginCall` is + * still open, and not yet closed by some other `endCall`, at the point `writeLiteral` executes). + * No other such call can coincide with `writeLiteral` itself, so checking the whole open interval + * between `beginCall` and `endCall` is equivalent to checking it on both sides of `writeLiteral` + * separately. * * `beginCall`, `writeLiteral`, and `endCall` are additionally required to have an implicit `this` * qualifier, which is how the Razor source generator always emits these calls. This ensures all @@ -204,37 +206,29 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { */ private predicate isBracketedForTagHelperAttribute(Call writeLiteral) { exists( - MicrosoftAspNetCoreMvcRazorPageBase page, MethodCall beginCall, MethodCall endCall, int i, - int j, int k + MicrosoftAspNetCoreMvcRazorPageBase page, MethodCall beginCall, MethodCall endCall, + BasicBlock bb, int i, int j, int k | + bb = writeLiteral.getBasicBlock() and writeLiteral = page.getWriteLiteralMethod().getACall() and beginCall = page.getBeginWriteTagHelperAttributeMethod().getACall() and endCall = page.getEndWriteTagHelperAttributeMethod().getACall() and writeLiteral.(QualifiableExpr).hasImplicitThisQualifier() and beginCall.hasImplicitThisQualifier() and endCall.hasImplicitThisQualifier() and - writeLiteral.getBasicBlock().getNode(i) = beginCall.getControlFlowNode() and - writeLiteral.getBasicBlock().getNode(j) = writeLiteral.getControlFlowNode() and - writeLiteral.getBasicBlock().getNode(k) = endCall.getControlFlowNode() and + bb.getNode(i) = beginCall.getControlFlowNode() and + bb.getNode(j) = writeLiteral.getControlFlowNode() and + bb.getNode(k) = endCall.getControlFlowNode() and i < j and j < k and - not exists(int i2, Call other | + not exists(int l, Call other | ( other = page.getBeginWriteTagHelperAttributeMethod().getACall() or other = page.getEndWriteTagHelperAttributeMethod().getACall() ) and - writeLiteral.getBasicBlock().getNode(i2) = other.getControlFlowNode() and - i < i2 and - i2 < j - ) and - not exists(int k2, Call other | - ( - other = page.getBeginWriteTagHelperAttributeMethod().getACall() or - other = page.getEndWriteTagHelperAttributeMethod().getACall() - ) and - writeLiteral.getBasicBlock().getNode(k2) = other.getControlFlowNode() and - j < k2 and - k2 < k + bb.getNode(l) = other.getControlFlowNode() and + i < l and + l < k ) ) } From 70d76cc63f7a1f23c729626c82b350d09319ad49 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:58:54 -0400 Subject: [PATCH 6/6] Eliminate redundant per-page-class join in isBracketedForTagHelperAttribute The shared 'page' variable forced the evaluator to redundantly repeat the expensive same-basic-block join once per RazorPageBase-derived class (e.g. 525 classes in one real-world repo) instead of once per distinct shared WriteLiteral/Begin/EndWriteTagHelperAttribute Method. Resolving each call independently is semantically equivalent (the per-instance correlation is already guaranteed by hasImplicitThisQualifier() + same BasicBlock) and removes an O(page-classes x calls) blowup that caused unbounded runtime on Razor-heavy codebases. Validated: direct DB runs against 3 real repos (before/after result counts unchanged, cache-cleared A/B equivalence test), plus MRVA runs on 2-core (45+min hang -> 4m22s) and 4-core (3m35s) GitHub Actions runners for the same repo batch. No change note: this predicate was introduced by this same unmerged PR and has never shipped, so this is pre-release refinement, not a behavior/perf fix to released code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 463cbe9c-9db5-425f-8784-63ea319314b9 --- .../security/dataflow/flowsinks/Html.qll | 47 +++++++++++++------ 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll index 3212855845fb..bcc36a54abad 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll @@ -180,7 +180,7 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { /** * Holds if `writeLiteral` is a call to `RazorPageBase.WriteLiteral` whose argument is captured * between a matching pair of `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()` - * calls on `page`, in the same basic block, with no other such calls in between. + * calls, in the same basic block, with no other such calls in between. * * The Razor source generator emits this bracketing for every literal or expression segment of an * HTML attribute value on an element that also carries a tag helper (for example `asp-for`). Such @@ -191,8 +191,8 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { * * Because a basic block cannot contain a branch, requiring `beginCall`, `writeLiteral`, and * `endCall` to appear (in that order) in the same basic block, with no other - * `Begin`/`EndWriteTagHelperAttribute` call from `page` strictly between `beginCall` and - * `endCall`, guarantees that `beginCall`/`endCall` are the immediately enclosing bracket around + * `Begin`/`EndWriteTagHelperAttribute` call strictly between `beginCall` and `endCall`, + * guarantees that `beginCall`/`endCall` are the immediately enclosing bracket around * `writeLiteral` on every path that reaches it (that is, the bracket opened by `beginCall` is * still open, and not yet closed by some other `endCall`, at the point `writeLiteral` executes). * No other such call can coincide with `writeLiteral` itself, so checking the whole open interval @@ -200,19 +200,30 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink { * separately. * * `beginCall`, `writeLiteral`, and `endCall` are additionally required to have an implicit `this` - * qualifier, which is how the Razor source generator always emits these calls. This ensures all - * three calls act on the same page instance, so a bracket on one page cannot be mistaken for a - * bracket around a `WriteLiteral` call on a different page. + * qualifier, which is how the Razor source generator always emits these calls. Combined with all + * three calls being required to lie in the same basic block (and hence the same method body), + * this ensures all three calls act on the same page instance, so a bracket on one page cannot be + * mistaken for a bracket around a `WriteLiteral` call on a different page. + * + * The `WriteLiteral`/`BeginWriteTagHelperAttribute`/`EndWriteTagHelperAttribute` methods are + * looked up via `any(MicrosoftAspNetCoreMvcRazorPageBase page).get...Method()` rather than + * through a single shared `page` variable bound across `writeLiteral`, `beginCall`, and + * `endCall`. These methods are inherited (not overridden) from the shared `RazorPageBase` + * framework type, so every generated Razor page class resolves to the same handful of `Method` + * entities; binding a single `page` variable across all three calls would force the join to be + * repeated once per generated page class in the codebase, rather than once per distinct `Method`, + * causing severe performance degradation on codebases with many Razor views. */ private predicate isBracketedForTagHelperAttribute(Call writeLiteral) { - exists( - MicrosoftAspNetCoreMvcRazorPageBase page, MethodCall beginCall, MethodCall endCall, - BasicBlock bb, int i, int j, int k - | + exists(MethodCall beginCall, MethodCall endCall, BasicBlock bb, int i, int j, int k | bb = writeLiteral.getBasicBlock() and - writeLiteral = page.getWriteLiteralMethod().getACall() and - beginCall = page.getBeginWriteTagHelperAttributeMethod().getACall() and - endCall = page.getEndWriteTagHelperAttributeMethod().getACall() and + writeLiteral = any(MicrosoftAspNetCoreMvcRazorPageBase page).getWriteLiteralMethod().getACall() and + beginCall = + any(MicrosoftAspNetCoreMvcRazorPageBase page) + .getBeginWriteTagHelperAttributeMethod() + .getACall() and + endCall = + any(MicrosoftAspNetCoreMvcRazorPageBase page).getEndWriteTagHelperAttributeMethod().getACall() and writeLiteral.(QualifiableExpr).hasImplicitThisQualifier() and beginCall.hasImplicitThisQualifier() and endCall.hasImplicitThisQualifier() and @@ -223,8 +234,14 @@ private predicate isBracketedForTagHelperAttribute(Call writeLiteral) { j < k and not exists(int l, Call other | ( - other = page.getBeginWriteTagHelperAttributeMethod().getACall() or - other = page.getEndWriteTagHelperAttributeMethod().getACall() + other = + any(MicrosoftAspNetCoreMvcRazorPageBase page) + .getBeginWriteTagHelperAttributeMethod() + .getACall() or + other = + any(MicrosoftAspNetCoreMvcRazorPageBase page) + .getEndWriteTagHelperAttributeMethod() + .getACall() ) and bb.getNode(l) = other.getControlFlowNode() and i < l and