C#: Blazor: Add non-local jump node for parameter passing

[tamasvajk]

Continuation of #18930.

Establishes the pattern that when a variable is passed to a subcomponent, the reads of the set property are considered tainted.

For example, if in the following example Names was tainted,

@foreach (string name in Names)
{
    <DisplayName Name="name"/>
}

Then a read in the DisplayName component would be considered tainted

@* Bad: rendering unsanitized strings *@
<p>@((MarkupString)Name)</p>

@code {
   [Parameter]
   string Name { get; set; }
}

[michaelnebel]

Thank you for following up on this @tamasvajk !
Would it be possible to add a brief explanation in the PR description on, which jump step we are modelling?

[michaelnebel]

Thank you for looking into it; One clarifying follow-up question and one comment.

[michaelnebel]

So, the jump step ensures that we have data flow from name into the property VulnerableBlazorApp.Components.Name.TheName?

[tamasvajk]

Yes, there's flow from name to the property reads of VulnerableBlazorApp.Components.Name.TheName.

[michaelnebel]

The NonLocalJumpNode is a bi-directional imported abstract class, but this is not that evident. I think it makes sense to explicitly make a private import module above the NonLocalJumpNode declaration that imports all file modules that extends the class.

[tamasvajk]

Do you mean the below?

--- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
+++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll
@@ -147,6 +147,10 @@ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) }
 pragma[inline]
 predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) }
 
+private import semmle.code.csharp.frameworks.microsoft.aspnetcore.Components
+private import semmle.code.csharp.frameworks.Razor
+private import semmle.code.csharp.frameworks.NHibernate
+
 /**
  * A data flow node that jumps between callables. This can be extended in
  * framework code to add additional data flow steps.

[michaelnebel]

Yes, and just in a private module.

/**
 * A module importing the modules that provide non local jump node declarations,
 * ensuring that they are visible to the taint tracking / data flow library.
 */
private module JumpNodes {
  private import semmle.code.csharp.frameworks.microsoft.aspnetcore.Components
  private import semmle.code.csharp.frameworks.Razor
  private import semmle.code.csharp.frameworks.NHibernate
}

[michaelnebel]

Excellent!

[hvitved]

I have started a DCA run. Also, it would be nicer to use inline test expectations, as suggested by QL4QL.

[tamasvajk]

it would be nicer to use inline test expectations, as suggested by QL4QL.

Should inline test expectations work when the source and sink locations are mapped to .razor files?

[hvitved]

Should inline test expectations work when the source and sink locations are mapped to .razor files?

Sorry, you are right, it doesn't work for .razor pages.