Skip to content

Commit befe578

Browse files
committed
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.
1 parent 459b9ff commit befe578

1 file changed

Lines changed: 16 additions & 22 deletions

File tree

  • csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks

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

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -192,10 +192,12 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink {
192192
* Because a basic block cannot contain a branch, requiring `beginCall`, `writeLiteral`, and
193193
* `endCall` to appear (in that order) in the same basic block, with no other
194194
* `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).
195+
* `endCall`, guarantees that `beginCall`/`endCall` are the immediately enclosing bracket around
196+
* `writeLiteral` on every path that reaches it (that is, the bracket opened by `beginCall` is
197+
* still open, and not yet closed by some other `endCall`, at the point `writeLiteral` executes).
198+
* No other such call can coincide with `writeLiteral` itself, so checking the whole open interval
199+
* between `beginCall` and `endCall` is equivalent to checking it on both sides of `writeLiteral`
200+
* separately.
199201
*
200202
* `beginCall`, `writeLiteral`, and `endCall` are additionally required to have an implicit `this`
201203
* qualifier, which is how the Razor source generator always emits these calls. This ensures all
@@ -204,37 +206,29 @@ class MicrosoftAspNetCoreMvcHtmlHelperRawSink extends AspNetCoreHtmlSink {
204206
*/
205207
private predicate isBracketedForTagHelperAttribute(Call writeLiteral) {
206208
exists(
207-
MicrosoftAspNetCoreMvcRazorPageBase page, MethodCall beginCall, MethodCall endCall, int i,
208-
int j, int k
209+
MicrosoftAspNetCoreMvcRazorPageBase page, MethodCall beginCall, MethodCall endCall,
210+
BasicBlock bb, int i, int j, int k
209211
|
212+
bb = writeLiteral.getBasicBlock() and
210213
writeLiteral = page.getWriteLiteralMethod().getACall() and
211214
beginCall = page.getBeginWriteTagHelperAttributeMethod().getACall() and
212215
endCall = page.getEndWriteTagHelperAttributeMethod().getACall() and
213216
writeLiteral.(QualifiableExpr).hasImplicitThisQualifier() and
214217
beginCall.hasImplicitThisQualifier() and
215218
endCall.hasImplicitThisQualifier() and
216-
writeLiteral.getBasicBlock().getNode(i) = beginCall.getControlFlowNode() and
217-
writeLiteral.getBasicBlock().getNode(j) = writeLiteral.getControlFlowNode() and
218-
writeLiteral.getBasicBlock().getNode(k) = endCall.getControlFlowNode() and
219+
bb.getNode(i) = beginCall.getControlFlowNode() and
220+
bb.getNode(j) = writeLiteral.getControlFlowNode() and
221+
bb.getNode(k) = endCall.getControlFlowNode() and
219222
i < j and
220223
j < k and
221-
not exists(int i2, Call other |
224+
not exists(int l, Call other |
222225
(
223226
other = page.getBeginWriteTagHelperAttributeMethod().getACall() or
224227
other = page.getEndWriteTagHelperAttributeMethod().getACall()
225228
) and
226-
writeLiteral.getBasicBlock().getNode(i2) = other.getControlFlowNode() and
227-
i < i2 and
228-
i2 < j
229-
) and
230-
not exists(int k2, Call other |
231-
(
232-
other = page.getBeginWriteTagHelperAttributeMethod().getACall() or
233-
other = page.getEndWriteTagHelperAttributeMethod().getACall()
234-
) and
235-
writeLiteral.getBasicBlock().getNode(k2) = other.getControlFlowNode() and
236-
j < k2 and
237-
k2 < k
229+
bb.getNode(l) = other.getControlFlowNode() and
230+
i < l and
231+
l < k
238232
)
239233
)
240234
}

0 commit comments

Comments
 (0)