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

JS: Add view-component-input threat model #18466

Merged
merged 10 commits into from
Jan 24, 2025

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jan 10, 2025

We sometimes get requests from users to treat React props, Vue props, or Angular inputs as taint sources. Such inputs are not true taint sources however, and tend to generate many false positives in practice if used as taint sources. These inputs are akin to function parameters in that they just hold whatever values are passed in at the component's instantiation site.

As a middle ground, this PR adds a new threat model kind called view-component-input. When enabled, React props, Vue props, and Angular2 inputs are seen as taint sources.

Also adds a new meta-query to report changes to threat model sources other than remote.

Evaluations:

@github-actions github-actions bot added the JS label Jan 10, 2025
@asgerf asgerf force-pushed the js/view-component-inputs branch 3 times, most recently from ca410fe to f0fe3c1 Compare January 21, 2025 09:19
@asgerf asgerf force-pushed the js/view-component-inputs branch from f0fe3c1 to c329336 Compare January 22, 2025 08:51
@asgerf asgerf force-pushed the js/view-component-inputs branch from c329336 to 4161f45 Compare January 22, 2025 09:46
@asgerf asgerf marked this pull request as ready for review January 22, 2025 11:43
@Copilot Copilot bot review requested due to automatic review settings January 22, 2025 11:43
@asgerf asgerf requested a review from a team as a code owner January 22, 2025 11:43

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 16 out of 34 changed files in this pull request and generated no comments.

Files not reviewed (18)
  • docs/codeql/reusables/threat-model-description.rst: Language not supported
  • javascript/ql/lib/semmle/javascript/Concepts.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/ViewComponentInput.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/frameworks/Angular2.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/frameworks/React.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/frameworks/Vue.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/CommandInjectionCustomizations.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/LogInjectionQuery.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/RegExpInjectionCustomizations.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/RemoteFlowSources.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/ResourceExhaustionCustomizations.qll: Language not supported
  • javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll: Language not supported
  • javascript/ql/src/meta/alerts/TaintSources.ql: Language not supported
  • javascript/ql/src/meta/alerts/ThreatModelSources.ql: Language not supported
  • javascript/ql/src/meta/internal/TaintMetrics.qll: Language not supported
  • javascript/ql/test/library-tests/frameworks/Angular2/test.expected: Language not supported

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

The whole safe-type thing seems somewhat like whack-a-mole on FPs.
But it's probably fine, and I don't have better ideas.

Comment on lines +9 to +17
@Input() sink1: string;
@Input() sink2: string;
@Input() sink3: string;
@Input() sink4: string;
@Input() sink5: string;
@Input() sink6: string;
@Input() sink7: string;
@Input() sink8: string;
@Input() sink9: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of these are named "sink", but they're sources, right?

Copy link
Contributor Author

@asgerf asgerf Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're like React props, they're just passed in from the instantiation site. When I initially wrote this test long ago, I had omitted the @Input() decorator on these fields, which you're supposed to put on such input fields in Angular (not sure if they're strictly required though).

If the threat model is enabled they become sources (in addition to having incoming flow from instantiation sites). The unit test now reports which nodes are associated with this threat model, but the threat model is not actually enabled, so the data flow test doesn't treat them as taint sources.

@asgerf
Copy link
Contributor Author

asgerf commented Jan 24, 2025

The whole safe-type thing seems somewhat like whack-a-mole on FPs.

I think it makes sense if you think about it in terms of division of responsibility between user and library. If something has type number then it's reasonable to say that the caller should ensure that only numbers are passed in. If something has type string the library is responsible for handling that safely.

@asgerf asgerf merged commit 1b7977b into github:main Jan 24, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants