-
Notifications
You must be signed in to change notification settings - Fork 2.1k
C#: Fix cs/web/xss false positive on Razor tag-helper attribute values #22628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
felickz
wants to merge
4
commits into
github:main
Choose a base branch
from
forks-felickz:felickz-csharp-razor-tag-helper-xss-fp
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
20c1033
C#: Fix cs/web/xss false positive on Razor tag-helper attribute values
felickz ace3b3c
Address CCR: require implicit-this receiver for bracket calls
felickz 6a272fb
Trim change note to match repo conventions
felickz 459b9ff
Address CCR: don't claim guaranteed downstream encoding
felickz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
4 changes: 4 additions & 0 deletions
4
csharp/ql/lib/change-notes/2026-09-18-razor-tag-helper-attribute-xss-fp.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +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`) capture the value into an internal buffer instead of writing it directly to the response, so they are no longer treated as XSS sinks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -177,14 +177,83 @@ 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 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 | ||||||||||||||||||||||||
| * `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). | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * `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, 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 | ||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||
|
Comment on lines
+228
to
+237
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Shouldn't this suffice as well? |
||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * 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 calls buffer the value as a tag | ||||||||||||||||||||||||
| * helper attribute rather than writing it directly 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) | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
1 change: 1 addition & 0 deletions
1
csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| // empty |
89 changes: 89 additions & 0 deletions
89
csharp/ql/test/query-tests/Security Features/CWE-079/XSS/RazorTagHelperAttribute.cshtml.g.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: <input asp-for="Value" value="@Model.Value"> | ||
| #pragma checksum "RazorTagHelperAttribute.cshtml" "{ff1816ec-aa5e-4d10-87f7-6f4963833460}" "c4ae76542f1958092cebd8f57beef899d20fc548" | ||
| // <auto-generated/> | ||
| #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("<input type=\"text\" "); | ||
| // GOOD: this `WriteLiteral` call is generated for the value of an HTML attribute on | ||
| // an element with a tag helper (e.g. `asp-for`). It writes into an internal string | ||
| // buffer via `BeginWriteTagHelperAttribute()`/`EndWriteTagHelperAttribute()`, and the | ||
| // buffered text is HTML-attribute-encoded later, when the tag helper attribute value | ||
| // is rendered. It is not a direct, unencoded write, so it must not be flagged. | ||
| BeginWriteTagHelperAttribute(); | ||
| WriteLiteral(model); | ||
| var __tagHelperAttribute_1 = EndWriteTagHelperAttribute(); | ||
| 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<string> Html { get; private set; } | ||
| public global::Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary<string> ViewData => (global::Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary<string>)PageContext?.ViewData; | ||
| } | ||
| } | ||
| #pragma warning restore 1591 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps introduce a
BasicBlock bbas a part of theexistswithbb = writeLiteral.getBasicBlock(), then it is a bit easier to see that no other basic blocks are used by mistake.