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

PHP - Taint - Match not detected when in Ternary Operator Assigned to Variable #3778

Closed
erwanlr opened this issue Aug 30, 2021 · 4 comments · Fixed by #3819
Closed

PHP - Taint - Match not detected when in Ternary Operator Assigned to Variable #3778

erwanlr opened this issue Aug 30, 2021 · 4 comments · Fixed by #3819
Assignees

Comments

@erwanlr
Copy link

erwanlr commented Aug 30, 2021

Describe the bug
When trying to reduce duplicate matches of #3742, by adding functions such as isset(...) as sanitizer, I noticed that some code was not detected as tainted

For example: sink((isset($source) ? $source : 'b')); is detected, but the below is not

$tainted = (isset($source) ? $source : 'c');
// ruleid:taint-not-detected
sink($tainted);

To Reproduce
https://semgrep.dev/s/z9zZ

Expected behavior
The match should be detected

What is the priority of the bug to you?

P1: important to fix or quite annoying

Environment
Semgrep.dev & CLI 0.63.0 (installed on MacOSX via Homebrew)

@IagoAbal
Copy link
Collaborator

My initial thought would be that

// ok:taint-not-detected
sanitizer(sink($source));

should be reported, but this depends on how you interpret the sanitizer. I would expect it to be used as sink(sanitizer($source));.

@erwanlr
Copy link
Author

erwanlr commented Aug 30, 2021

Ha, I did not even pay attention to that one as it is not really an issue for me here ;D. In my everyday cases, having sink(sanitizer($source)); or sanitizer(sink($source)); is the same thing and safe, but I definitely understand that in some cases, such behaviour may not be desirable.

@IagoAbal
Copy link
Collaborator

We probably need to support different kinds of sanitizers.

Regarding this issue, it seems related to a bug in the Generic-to-IL translation. Some colleagues wanted to get familiar with the taint-mode code so I'll give them some time to look into this issue. If they don't find time then I'll fix this myself next week.

@erwanlr
Copy link
Author

erwanlr commented Aug 31, 2021

I would love to have different kind of sanitisers, as in the example above sanitizer(sink($source)); is not detected and that's a wanted behaviour in the case I was dealing with, but when the sink/source is a multi lines pattern, then semgrep reports it even though the sink goes through a sanitiser (however that's the correct behaviour from what you mentioned)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants