Skip to content

Commit 20c1033

Browse files
felickzCopilot
andcommitted
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>
1 parent b5bd7a4 commit 20c1033

6 files changed

Lines changed: 175 additions & 2 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: majorAnalysis
3+
---
4+
* 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.

‎csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,16 @@ class MicrosoftAspNetCoreMvcRazorPageBase extends Class {
491491

492492
/** Gets the `WriteLiteral` method. */
493493
Method getWriteLiteralMethod() { result = this.getAMethod("WriteLiteral") }
494+
495+
/** Gets the `BeginWriteTagHelperAttribute` method. */
496+
Method getBeginWriteTagHelperAttributeMethod() {
497+
result = this.getAMethod("BeginWriteTagHelperAttribute")
498+
}
499+
500+
/** Gets the `EndWriteTagHelperAttribute` method. */
501+
Method getEndWriteTagHelperAttributeMethod() {
502+
result = this.getAMethod("EndWriteTagHelperAttribute")
503+
}
494504
}
495505

496506
/** A class deriving from `Microsoft.AspNetCore.Http.HttpRequest`, implements `HttpRequest` in ASP.NET Core. */

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/Html.qll‎

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,14 +177,74 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink {
177177
}
178178
}
179179

180+
/**
181+
* Holds if `writeLiteral` is a call to `RazorPageBase.WriteLiteral` whose argument is captured
182+
* between a matching pair of `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()`
183+
* calls on `page`, in the same basic block, with no other such calls in between.
184+
*
185+
* The Razor source generator emits this bracketing for every literal or expression segment of an
186+
* HTML attribute value on an element that also carries a tag helper (for example `asp-for`). Such
187+
* a `WriteLiteral` call does not write directly, unencoded, to the response: `WriteLiteral`
188+
* appends to an internal string buffer, `EndWriteTagHelperAttribute()` returns that buffer, and
189+
* the buffered text is subsequently stored as a tag helper attribute value and HTML-attribute-
190+
* encoded when the tag helper's output is rendered. This is therefore not a real sink.
191+
*
192+
* Because a basic block cannot contain a branch, requiring `beginCall`, `writeLiteral`, and
193+
* `endCall` to appear (in that order) in the same basic block, with no other
194+
* `Begin`/`EndWriteTagHelperAttribute` call from `page` strictly between `beginCall` and
195+
* `writeLiteral`, or between `writeLiteral` and `endCall`, guarantees that `beginCall`/`endCall`
196+
* are the immediately enclosing bracket around `writeLiteral` on every path that reaches it (that
197+
* is, the bracket opened by `beginCall` is still open, and not yet closed by some other `endCall`,
198+
* at the point `writeLiteral` executes).
199+
*/
200+
private predicate isBracketedForTagHelperAttribute(Call writeLiteral) {
201+
exists(
202+
MicrosoftAspNetCoreMvcRazorPageBase page, Call beginCall, Call endCall, int i, int j, int k
203+
|
204+
writeLiteral = page.getWriteLiteralMethod().getACall() and
205+
beginCall = page.getBeginWriteTagHelperAttributeMethod().getACall() and
206+
endCall = page.getEndWriteTagHelperAttributeMethod().getACall() and
207+
writeLiteral.getBasicBlock().getNode(i) = beginCall.getControlFlowNode() and
208+
writeLiteral.getBasicBlock().getNode(j) = writeLiteral.getControlFlowNode() and
209+
writeLiteral.getBasicBlock().getNode(k) = endCall.getControlFlowNode() and
210+
i < j and
211+
j < k and
212+
not exists(int i2, Call other |
213+
(
214+
other = page.getBeginWriteTagHelperAttributeMethod().getACall() or
215+
other = page.getEndWriteTagHelperAttributeMethod().getACall()
216+
) and
217+
writeLiteral.getBasicBlock().getNode(i2) = other.getControlFlowNode() and
218+
i < i2 and
219+
i2 < j
220+
) and
221+
not exists(int k2, Call other |
222+
(
223+
other = page.getBeginWriteTagHelperAttributeMethod().getACall() or
224+
other = page.getEndWriteTagHelperAttributeMethod().getACall()
225+
) and
226+
writeLiteral.getBasicBlock().getNode(k2) = other.getControlFlowNode() and
227+
j < k2 and
228+
k2 < k
229+
)
230+
)
231+
}
232+
180233
/**
181234
* An expression that is used as an argument to `Page.WriteLiteral` in ASP.NET 6.0 razor page, typically in
182235
* a `.cshtml` file.
236+
*
237+
* `WriteLiteral` calls whose argument is captured for a tag helper attribute value (see
238+
* `isBracketedForTagHelperAttribute`) are excluded, since such values are HTML-attribute-encoded
239+
* later and are not written unencoded to the response.
183240
*/
184241
class MicrosoftAspNetRazorPageWriteLiteralSink extends AspNetCoreHtmlSink {
185242
MicrosoftAspNetRazorPageWriteLiteralSink() {
186-
this.getExpr() =
187-
any(MicrosoftAspNetCoreMvcRazorPageBase h).getWriteLiteralMethod().getACall().getAnArgument()
243+
exists(Call writeLiteral |
244+
writeLiteral = any(MicrosoftAspNetCoreMvcRazorPageBase h).getWriteLiteralMethod().getACall() and
245+
this.getExpr() = writeLiteral.getAnArgument() and
246+
not isBracketedForTagHelperAttribute(writeLiteral)
247+
)
188248
}
189249
}
190250

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
// empty
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// A hand-written test file that mimics the output of compiling a `.cshtml` file that has an
2+
// element with a tag helper (e.g. `asp-for`) and an HTML attribute whose value contains a
3+
// tainted expression, for example: <input asp-for="Value" value="@Model.Value">
4+
#pragma checksum "RazorTagHelperAttribute.cshtml" "{ff1816ec-aa5e-4d10-87f7-6f4963833460}" "c4ae76542f1958092cebd8f57beef899d20fc548"
5+
// <auto-generated/>
6+
#pragma warning disable 1591
7+
[assembly: global::Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemAttribute(typeof(dotnetweb.Pages.Pages_RazorTagHelperAttribute), @"mvc.1.0.razor-page", @"RazorTagHelperAttribute.cshtml")]
8+
namespace dotnetweb.Pages
9+
{
10+
#line hidden
11+
using System;
12+
using System.Collections.Generic;
13+
using System.Linq;
14+
using System.Threading.Tasks;
15+
using Microsoft.AspNetCore.Mvc;
16+
using Microsoft.AspNetCore.Mvc.Rendering;
17+
using Microsoft.AspNetCore.Mvc.ViewFeatures;
18+
#nullable restore
19+
using dotnetweb;
20+
21+
#line default
22+
#line hidden
23+
#nullable disable
24+
[global::Microsoft.AspNetCore.Razor.Hosting.RazorSourceChecksumAttribute(@"SHA1", @"c4ae76542f1958092cebd8f57beef899d20fc548", @"RazorTagHelperAttribute.cshtml")]
25+
public class Pages_RazorTagHelperAttribute : global::Microsoft.AspNetCore.Mvc.RazorPages.Page
26+
{
27+
#pragma warning disable 1998
28+
public async override global::System.Threading.Tasks.Task ExecuteAsync()
29+
{
30+
#nullable restore
31+
#line 3 "RazorTagHelperAttribute.cshtml"
32+
33+
var model = Request.Query["m"]; // $ Source=model
34+
35+
#line default
36+
#line hidden
37+
#nullable disable
38+
WriteLiteral("<input type=\"text\" ");
39+
// GOOD: this `WriteLiteral` call is generated for the value of an HTML attribute on
40+
// an element with a tag helper (e.g. `asp-for`). It writes into an internal string
41+
// buffer via `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()`, and the
42+
// buffered text is HTML-attribute-encoded later, when the tag helper attribute value
43+
// is rendered. It is not a direct, unencoded write, so it must not be flagged.
44+
BeginWriteTagHelperAttribute();
45+
WriteLiteral(model);
46+
var __tagHelperAttribute_1 = EndWriteTagHelperAttribute();
47+
WriteLiteral(" />");
48+
49+
// BAD: a `WriteLiteral` call that is not bracketed by
50+
// `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()` writes directly,
51+
// unencoded, to the response and must still be flagged.
52+
WriteLiteral(model); // $ Alert=model
53+
54+
// GOOD: two independent tag helper attribute brackets that occur one after another
55+
// in the same basic block must each be matched with their own nearest
56+
// `Begin`/`EndWriteTagHelperAttribute` call, and neither should leak into the other.
57+
BeginWriteTagHelperAttribute();
58+
WriteLiteral("literal-prefix-");
59+
var __tagHelperAttribute_2 = EndWriteTagHelperAttribute();
60+
BeginWriteTagHelperAttribute();
61+
WriteLiteral(model);
62+
var __tagHelperAttribute_3 = EndWriteTagHelperAttribute();
63+
64+
// BAD: a bare `WriteLiteral` call sandwiched between two unrelated tag helper
65+
// attribute brackets, in the same basic block, must not be mistaken for being
66+
// captured by either neighboring bracket.
67+
BeginWriteTagHelperAttribute();
68+
WriteLiteral("prefix");
69+
var __tagHelperAttribute_4 = EndWriteTagHelperAttribute();
70+
WriteLiteral(model); // $ Alert=model
71+
BeginWriteTagHelperAttribute();
72+
WriteLiteral("suffix");
73+
var __tagHelperAttribute_5 = EndWriteTagHelperAttribute();
74+
}
75+
#pragma warning restore 1998
76+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
77+
public global::Microsoft.AspNetCore.Mvc.ViewFeatures.IModelExpressionProvider ModelExpressionProvider { get; private set; }
78+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
79+
public global::Microsoft.AspNetCore.Mvc.IUrlHelper Url { get; private set; }
80+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
81+
public global::Microsoft.AspNetCore.Mvc.IViewComponentHelper Component { get; private set; }
82+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
83+
public global::Microsoft.AspNetCore.Mvc.Rendering.IJsonHelper Json { get; private set; }
84+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
85+
public global::Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper<string> Html { get; private set; }
86+
public global::Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary<string> ViewData => (global::Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary<string>)PageContext?.ViewData;
87+
}
88+
}
89+
#pragma warning restore 1591

‎csharp/ql/test/query-tests/Security Features/CWE-079/XSS/XSS.expected‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#select
22
| 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 |
3+
| 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 |
4+
| 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 |
35
| 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 |
46
| 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 |
57
| 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 @@
1315
edges
1416
| Index.cshtml:5:9:5:15 | access to local variable message : StringValues | Index.cshtml:14:16:14:22 | call to operator implicit conversion | provenance | |
1517
| Index.cshtml:5:19:5:31 | access to property Query : IQueryCollection | Index.cshtml:5:9:5:15 | access to local variable message : StringValues | provenance | |
18+
| 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 | |
19+
| 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 | |
20+
| RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | RazorTagHelperAttribute.cshtml:4:9:4:13 | access to local variable model : StringValues | provenance | |
1621
| XSSAspNet.cs:19:17:19:21 | access to local variable sayHi : String | XSSAspNet.cs:26:30:26:34 | access to local variable sayHi | provenance | |
1722
| XSSAspNet.cs:19:17:19:21 | access to local variable sayHi : String | XSSAspNet.cs:36:40:36:44 | access to local variable sayHi | provenance | |
1823
| 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
4954
| Index.cshtml:5:9:5:15 | access to local variable message : StringValues | semmle.label | access to local variable message : StringValues |
5055
| Index.cshtml:5:19:5:31 | access to property Query : IQueryCollection | semmle.label | access to property Query : IQueryCollection |
5156
| Index.cshtml:14:16:14:22 | call to operator implicit conversion | semmle.label | call to operator implicit conversion |
57+
| RazorTagHelperAttribute.cshtml.g.cs:52:26:52:30 | call to operator implicit conversion | semmle.label | call to operator implicit conversion |
58+
| RazorTagHelperAttribute.cshtml.g.cs:70:26:70:30 | call to operator implicit conversion | semmle.label | call to operator implicit conversion |
59+
| RazorTagHelperAttribute.cshtml:4:9:4:13 | access to local variable model : StringValues | semmle.label | access to local variable model : StringValues |
60+
| RazorTagHelperAttribute.cshtml:4:17:4:29 | access to property Query : IQueryCollection | semmle.label | access to property Query : IQueryCollection |
5261
| XSSAspNet.cs:19:17:19:21 | access to local variable sayHi : String | semmle.label | access to local variable sayHi : String |
5362
| XSSAspNet.cs:19:25:19:43 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
5463
| XSSAspNet.cs:19:25:19:52 | access to indexer : String | semmle.label | access to indexer : String |

0 commit comments

Comments
 (0)