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

Missing TaintedSql for tainted integers #10238

Closed
cgocast opened this issue Sep 29, 2023 · 3 comments · Fixed by #10242
Closed

Missing TaintedSql for tainted integers #10238

cgocast opened this issue Sep 29, 2023 · 3 comments · Fixed by #10242

Comments

@cgocast
Copy link
Contributor

cgocast commented Sep 29, 2023

This code snippet https://psalm.dev/r/0d4f65473b is a slight modification of the snippet given in the TaintedSql documentation https://psalm.dev/docs/running_psalm/issues/TaintedSql/ The cast to string has been changed to a cast to int.

An attacker could delete thousands of users from the table by doing multiple GET requests with a range of "user_id". Therefore I would expect that Psalm raises a TaintedSql, even if this code snippet is not vulnerable to classical SQL injection.

I'll write a PR to fix this issue.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/0d4f65473b
<?php //--taint-analysis

class A {
    public function deleteUser(PDO $pdo) : void {
        $userId = self::getUserId();
        $pdo->exec("delete from users where user_id = " . $userId);
    }

    public static function getUserId() : int {
        return (int) $_GET["user_id"];
    }
}
Psalm output (using commit 4746f83):

No issues!

@orklah
Copy link
Collaborator

orklah commented Sep 29, 2023

You may want to look at #6993

I assumed back then that some types could not transmit taints, the rule may be more complex than that then

@cgocast
Copy link
Contributor Author

cgocast commented Oct 2, 2023

Thank you @orklah for the interesting link.

In my opinion, which type transmit taints or not depends on the issue. For instance, numerics shoud transmit taints for TaintedSql, but they should not for TaintedHtml.

Of course there will be some specific SQL queries where integers are safe. But I prefer those cases to be handled with a @psalm-taint-escape sql annotation

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

Successfully merging a pull request may close this issue.

2 participants