Skip to content
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

Jump to bottom

Taint Tracking of Function Passed Through JSX Attributes #15207

Closed
gtsp233 opened this issue Dec 30, 2023 · 3 comments · Fixed by #15221
Closed

Taint Tracking of Function Passed Through JSX Attributes #15207

gtsp233 opened this issue Dec 30, 2023 · 3 comments · Fixed by #15221
Labels
acknowledged GitHub staff acknowledges this issue JS question Further information is requested

Comments

@gtsp233
Copy link

gtsp233 commented Dec 30, 2023 •
edited

Description of the issue

I'm facing an issue where CodeQL is failing to track data flow correctly when a function is passed through a JSX attribute. For example:

import React from "react";
import ReactDOM from "react-dom/client";

function App() {
  <Comp p1={window.location} p2={() => window.location} />;
}
function Comp(props) {
  return (
    <div>
      <p
        dangerouslySetInnerHTML={{
          __html: props.p1,
        }}
      />
      <p
        dangerouslySetInnerHTML={{
          __html: props.p2(),
        }}
      />
    </div>
  );
}
const root = ReactDOM.createRoot(document.getElementById("root"));
root.render(<App />);
export default App;

I ran the official query client-side cross-site scripting on this example, and it turned out that CodeQl failed to track the data flow through props.p2 when it is a function passed via a JSX attribute, in contrast to its successful detection in the case of props.p1.

Is this an inherent limitation, a bug, or a case of incorrect usage?
@gtsp233 gtsp233 added the question Further information is requested label Dec 30, 2023
@jketema
Copy link
Contributor

jketema commented Jan 2, 2024

Hi @gtsp233,

I've asked the relevant engineering team to take a look. Note that this may take a few days, depending on when people are back from their New Year's holiday.

@erik-krogh
Copy link
Contributor

Is this an inherent limitation, a bug, or a case of incorrect usage?

It's a limitation of our current analysis.
The relevant code can be found here:

codeql/javascript/ql/lib/semmle/javascript/frameworks/React.qll

Lines 860 to 870 in 19c5d1f

private class PropsTaintStep extends TaintTracking::SharedTaintStep {
override predicate viewComponentStep(DataFlow::Node pred, DataFlow::Node succ) {
exists(ReactComponent c, string name, DataFlow::PropRead prn |
prn = c.getAPropRead(name) or
prn = c.getAPreviousPropsSource().getAPropertyRead(name)
|
pred = c.getACandidatePropsValue(name) and
succ = prn
)
}
}

On such a React property we're only tracking that the value might be tainted, we're not tracking what the value contains.
This is to prevent the analysis from blowing up in complexity/runtime, and to prevent possible false positives.

But, it might be an easy fix to improve the analysis. I'm looking into that.

@sidshank sidshank added JS acknowledged GitHub staff acknowledges this issue labels Jan 2, 2024
@erik-krogh erik-krogh mentioned this issue Jan 4, 2024
Merged
@erik-krogh
Copy link
Contributor

The missing flow you described should be fixed now that #15221 has been merged.

It will take a few weeks for the fix to be released in a CLI release / code-scanning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
acknowledged GitHub staff acknowledges this issue JS question Further information is requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

JS: promote PropsTaintStep to a PreCallGraphStep erik-krogh/ql
4 participants
@sidshank @erik-krogh @jketema @gtsp233