Skip to content

Commit 826111d

Browse files
joefarebrotherjoefarebrother
committed
Separate area view discovery list for increased precision
1 parent f2c3d83 commit 826111d

File tree

7 files changed

+237
-52
lines changed

7 files changed

+237
-52
lines changed

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/XSSFlowSteps.qll

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,9 @@ private class ViewCall extends MethodCall {
6868
result = attr.getArgument(0).(StringLiteral).getValue()
6969
)
7070
}
71+
72+
/** `result` is `true` if this cal is from a controller that is an an Area, `false` otherwise. */
73+
boolean hasArea() { if exists(this.getAreaName()) then result = true else result = false }
7174
}
7275

7376
/** A compiler-generated Razor page. */
@@ -119,38 +122,43 @@ private predicate viewCallRefersToPageRelative(ViewCall vc, RazorPage rp) {
119122
}
120123

121124
/** Gets the `i`th template for view discovery. */
122-
private string getViewSearchTemplate(int i) {
123-
i = 0 and result = "/Areas/{2}/Views/{1}/{0}.cshtml"
125+
private string getViewSearchTemplate(int i, boolean isArea) {
126+
i = 0 and result = "/Areas/{2}/Views/{1}/{0}.cshtml" and isArea = true
127+
or
128+
i = 1 and result = "/Areas/{2}/Views/Shared/{0}.cshtml" and isArea = true
124129
or
125-
i = 1 and result = "/Areas/{2}/Views/Shared/{0}.cshtml"
130+
i = 2 and result = "/Views/{1}/{0}.cshtml" and isArea = false
126131
or
127-
i = 2 and result = "/Views/{1}/{0}.cshtml"
132+
i = 3 and result = "/Views/Shared/{0}.cshtml" and isArea = [true, false]
128133
or
129-
i = 3 and result = "/Views/Shared/{0}.cshtml"
134+
i = 4 and result = "/Pages/Shared/{0}.cshtml" and isArea = true
130135
or
131-
i = 4 and result = getAViewSearchTemplateInCode()
136+
i = 5 and result = getAViewSearchTemplateInCode(isArea)
132137
}
133138

134139
/** Gets an additional template used for view discovery defined in code. */
135-
private string getAViewSearchTemplateInCode() {
140+
private string getAViewSearchTemplateInCode(boolean isArea) {
136141
exists(StringLiteral str, MethodCall addCall |
137142
addCall.getTarget().hasName("Add") and
138143
DataFlow::localExprFlow(str, addCall.getArgument(0)) and
139-
addCall.getQualifier() = getAViewLocationList() and
144+
addCall.getQualifier() = getAViewLocationList(isArea) and
140145
result = str.getValue()
141146
)
142147
}
143148

144149
/** Gets a list expression containing view search locations */
145-
private Expr getAViewLocationList() {
146-
result
147-
.(PropertyRead)
148-
.getProperty()
149-
.hasQualifiedName("Microsoft.AspNetCore.Mvc.Razor", "RazorViewEngineOptions",
150-
[
151-
"ViewLocationFormats", "AreaViewLocationFormats",
152-
//"PageViewLocationFormats","AreaPageViewLocationFormats"
153-
])
150+
private Expr getAViewLocationList(boolean isArea) {
151+
exists(string name |
152+
result
153+
.(PropertyRead)
154+
.getProperty()
155+
.hasQualifiedName("Microsoft.AspNetCore.Mvc.Razor", "RazorViewEngineOptions", name)
156+
|
157+
name = "ViewLocationFormats" and isArea = false
158+
or
159+
name = "AreaViewLocationFormats" and isArea = true
160+
// PageViewLocationFormats and AreaPageViewLocationFormats are used for calls within a page rather than a controller
161+
)
154162
}
155163

156164
/** A filepath that should be searched for a View call. */
@@ -160,7 +168,7 @@ private class RelativeViewCallFilepath extends NormalizableFilepath {
160168

161169
RelativeViewCallFilepath() {
162170
exists(string template, string sub2, string sub1, string sub0 |
163-
template = getViewSearchTemplate(idx_)
171+
template = getViewSearchTemplate(idx_, vc_.hasArea())
164172
|
165173
(
166174
if template.matches("%{2}%")

‎csharp/ql/test/query-tests/Security Features/CWE-079/XSSRazorPages/Controllers/TestController.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ public class Test3Controller : Controller {
101101
public void Setup(RazorViewEngineOptions o) {
102102
o.ViewLocationFormats.Add("/Views/Custom/{1}/{0}.cshtml");
103103
o.ViewLocationFormats.Add("~/Views/Custom2/{0}.cshtml");
104+
o.AreaViewLocationFormats.Add("/MyAreas/{2}/{1}/{0}.cshtml");
104105
}
105106

106107
public IActionResult Test15(UserData tainted15) {
@@ -132,7 +133,17 @@ public IActionResult test19(UserData tainted19) {
132133
}
133134

134135
public IActionResult test20(UserData tainted20) {
135-
// SPURIOUS: Expected to find nothing (and NOT /Views/Test4/Test20.cshtml).
136+
// Expected to find nothing (and NOT /Views/Test4/Test20.cshtml).
136137
return View("Test20", tainted20);
137138
}
139+
140+
public IActionResult test21(UserData tainted21) {
141+
// Expected to find file /Pages/Shared/Test21.cshtml
142+
return View("Test21", tainted21);
143+
}
144+
145+
public IActionResult test22(UserData tainted22) {
146+
// Expected to find file /MyAreas/TestArea/Test4/Test22.cshtml
147+
return View("Test22", tainted22);
148+
}
138149
}

‎csharp/ql/test/query-tests/Security Features/CWE-079/XSSRazorPages/Generated/MyAreas_Test4_Test22.cshtml.g.cs

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// A test file that mimics the output of compiling a `.cshtml` file
2+
// <auto-generated/>
3+
#pragma warning disable 1591
4+
[assembly: global::Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemAttribute(typeof(test.Views.MyAreas_Test4_Test22), @"mvc.1.0.view", @"/MyAreas/Test4/Test22.cshtml")]
5+
namespace test.Views
6+
{
7+
#line hidden
8+
using System;
9+
using System.Collections.Generic;
10+
using System.Linq;
11+
using System.Threading.Tasks;
12+
using Microsoft.AspNetCore.Mvc;
13+
using Microsoft.AspNetCore.Mvc.Rendering;
14+
using Microsoft.AspNetCore.Mvc.ViewFeatures;
15+
#nullable restore
16+
using test;
17+
18+
#line default
19+
#line hidden
20+
#nullable disable
21+
[global::Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemMetadataAttribute("Identifier", "/MyAreas/Test4/Test22.cshtml")]
22+
public class MyAreas_Test4_Test22 : global::Microsoft.AspNetCore.Mvc.Razor.RazorPage<UserData>
23+
{
24+
#pragma warning disable 1998
25+
public async override global::System.Threading.Tasks.Task ExecuteAsync()
26+
{
27+
#line 6 "MyAreas/Test4/Test22.cshtml"
28+
if (Model != null)
29+
{
30+
31+
#line default
32+
#line hidden
33+
#nullable disable
34+
WriteLiteral(" <h3>Hello \"");
35+
#nullable restore
36+
#line 8 "MyAreas/Test4/Test22.cshtml"
37+
Write(Html.Raw(Model.Name));
38+
39+
#line default
40+
#line hidden
41+
#nullable disable
42+
WriteLiteral("\"</h3>\n");
43+
#nullable restore
44+
#line 9 "MyAreas/Test4/Test22.cshtml"
45+
}
46+
47+
#line default
48+
#line hidden
49+
#nullable disable
50+
}
51+
#pragma warning restore 1998
52+
#nullable restore
53+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
54+
public global::Microsoft.AspNetCore.Mvc.ViewFeatures.IModelExpressionProvider ModelExpressionProvider { get; private set; } = default!;
55+
#nullable disable
56+
#nullable restore
57+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
58+
public global::Microsoft.AspNetCore.Mvc.IUrlHelper Url { get; private set; } = default!;
59+
#nullable disable
60+
#nullable restore
61+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
62+
public global::Microsoft.AspNetCore.Mvc.IViewComponentHelper Component { get; private set; } = default!;
63+
#nullable disable
64+
#nullable restore
65+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
66+
public global::Microsoft.AspNetCore.Mvc.Rendering.IJsonHelper Json { get; private set; } = default!;
67+
#nullable disable
68+
#nullable restore
69+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
70+
public global::Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper<UserData> Html { get; private set; } = default!;
71+
#nullable disable
72+
}
73+
}
74+
#pragma warning restore 1591

‎csharp/ql/test/query-tests/Security Features/CWE-079/XSSRazorPages/Generated/Pages_Shared_Test21.cshtml.g.cs

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// A test file that mimics the output of compiling a `.cshtml` file
2+
// <auto-generated/>
3+
#pragma warning disable 1591
4+
[assembly: global::Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemAttribute(typeof(test.Views.Pages_Shared_Test21), @"mvc.1.0.view", @"/Pages/Shared/Test21.cshtml")]
5+
namespace test.Views
6+
{
7+
#line hidden
8+
using System;
9+
using System.Collections.Generic;
10+
using System.Linq;
11+
using System.Threading.Tasks;
12+
using Microsoft.AspNetCore.Mvc;
13+
using Microsoft.AspNetCore.Mvc.Rendering;
14+
using Microsoft.AspNetCore.Mvc.ViewFeatures;
15+
#nullable restore
16+
using test;
17+
18+
#line default
19+
#line hidden
20+
#nullable disable
21+
[global::Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemMetadataAttribute("Identifier", "/Pages/Shared/Test21.cshtml")]
22+
public class Pages_Shared_Test21 : global::Microsoft.AspNetCore.Mvc.Razor.RazorPage<UserData>
23+
{
24+
#pragma warning disable 1998
25+
public async override global::System.Threading.Tasks.Task ExecuteAsync()
26+
{
27+
#line 6 "Pages/Shared/Test21.cshtml"
28+
if (Model != null)
29+
{
30+
31+
#line default
32+
#line hidden
33+
#nullable disable
34+
WriteLiteral(" <h3>Hello \"");
35+
#nullable restore
36+
#line 8 "Pages/Shared/Test21.cshtml"
37+
Write(Html.Raw(Model.Name));
38+
39+
#line default
40+
#line hidden
41+
#nullable disable
42+
WriteLiteral("\"</h3>\n");
43+
#nullable restore
44+
#line 9 "Pages/Shared/Test21.cshtml"
45+
}
46+
47+
#line default
48+
#line hidden
49+
#nullable disable
50+
}
51+
#pragma warning restore 1998
52+
#nullable restore
53+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
54+
public global::Microsoft.AspNetCore.Mvc.ViewFeatures.IModelExpressionProvider ModelExpressionProvider { get; private set; } = default!;
55+
#nullable disable
56+
#nullable restore
57+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
58+
public global::Microsoft.AspNetCore.Mvc.IUrlHelper Url { get; private set; } = default!;
59+
#nullable disable
60+
#nullable restore
61+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
62+
public global::Microsoft.AspNetCore.Mvc.IViewComponentHelper Component { get; private set; } = default!;
63+
#nullable disable
64+
#nullable restore
65+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
66+
public global::Microsoft.AspNetCore.Mvc.Rendering.IJsonHelper Json { get; private set; } = default!;
67+
#nullable disable
68+
#nullable restore
69+
[global::Microsoft.AspNetCore.Mvc.Razor.Internal.RazorInjectAttribute]
70+
public global::Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper<UserData> Html { get; private set; } = default!;
71+
#nullable disable
72+
}
73+
}
74+
#pragma warning restore 1591

‎csharp/ql/test/query-tests/Security Features/CWE-079/XSSRazorPages/MyAreas/Test4/Test22.cshtml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
@namespace test
2+
@model UserData
3+
@{
4+
}
5+
6+
@if (Model != null)
7+
{
8+
<h3>Hello "@Html.Raw(Model.Name)"</h3>
9+
}

‎csharp/ql/test/query-tests/Security Features/CWE-079/XSSRazorPages/Pages/Shared/Test21.cshtml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
@namespace test
2+
@model UserData
3+
@{
4+
}
5+
6+
@if (Model != null)
7+
{
8+
<h3>Hello "@Html.Raw(Model.Name)"</h3>
9+
}

0 commit comments

Comments
 (0)