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

Enforce metavariable equality between sources/sinks/sanitizers, and show metavariables in taint mode messages #4073

Merged
merged 27 commits into from Oct 27, 2021

Conversation

mmcqd
Copy link
Member

@mmcqd mmcqd commented Oct 14, 2021

We can now write rules that use the same metavariable across sources/sinks/sanitizers, and refer to these metavariables in match messages. Currently if a sink could be matched with multiple possible metavariable assignments it will be matched twice by semgrep-core, but semgrep will filter out all but one of these matches.

PR checklist:

  • Documentation is up-to-date
  • Changelog is up-to-date
  • Change has no security implications (otherwise, ping security team)

@linear
Copy link

linear bot commented Oct 14, 2021

PA-313 Metavariable equality between sources, sanitizers and sink specifications

Right now each spec is independent, if you match $MVAR in a source and in a sink these are considered two different variables. We should require that all $MVAR matches are consistent.

@mmcqd mmcqd changed the title Enforce metavariable equality between sources/sinks/sanitizers, and allow metavariables in taint mode messages Enforce metavariable equality between sources/sinks/sanitizers, and show metavariables in taint mode messages Oct 14, 2021
@mmcqd mmcqd requested a review from IagoAbal October 21, 2021 01:52
Copy link
Collaborator

@aryx aryx left a comment

Choose a reason for hiding this comment

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

I don't want the core to depends on analyzing. There has to be a way to avoid this dependency.
In fact in core/dune we should remove pfff-lang_GENERIC-analyze, it's not currently needed; not sure why it was there in the first place.

@aryx
Copy link
Collaborator

aryx commented Oct 21, 2021

Maybe you can move Dataflow_tainting.ml in engine/, and add a note that it used to be in analyzing/, but with
metavariable checking the environment now depends on Pattern_match.t, which means it depends on semgrep-core/core.
Then say it can't be in analyzing/ because
we want analyzing/ to be independent of semgrep-core/core because it's used in other projects (e.g., codemap from aryx)

Copy link
Collaborator

@aryx aryx left a comment

Choose a reason for hiding this comment

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

Asking for more comments mostly.

semgrep-core/src/core/Pattern_match.ml Outdated Show resolved Hide resolved
semgrep-core/src/engine/Dataflow_tainting.ml Show resolved Hide resolved
semgrep-core/src/engine/Dataflow_tainting.ml Show resolved Hide resolved
semgrep-core/src/engine/Dataflow_tainting.mli Show resolved Hide resolved
@IagoAbal
Copy link
Collaborator

I don't want the core to depends on analyzing. There has to be a way to avoid this dependency.

Sorry that I missed the initial context. @mmcqd was the core that depended on analyzing? Or analyzing that depended on core? I would only expect Dataflow_tainting to depend on Pattern_match.t.

@mmcqd mmcqd requested review from IagoAbal and aryx October 26, 2021 19:28
@mmcqd mmcqd merged commit 209c574 into develop Oct 27, 2021
@mmcqd mmcqd deleted the matthew/pa-313-metavariable-equality-between-sources branch October 27, 2021 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants