Taint should be ignored on int cast about psalm HOT 16 CLOSED

I just stumbled over this after upgrading from version 5.12.0 and bisecting it down to commit 94a98cc, merge 7a7d6a2 and issue #10238.

The original issue is describing an attack vector known as IDOR, which can be seen as cross-cutting concern for multiple taint types in PsalmPHP.

The mentioned commit caused a bunch of false-positive failures on our side. We are testing 3rd party code (plugins) we cannot directly control.

In general I agree that any attacker-controlled parameter can lead to a vulnerability - however the mentioned change was too intrusive. It should be reverted and re-introduced as opt-in feature. Here's is why:

• the change was introduced with PsalmPHP 5.16.0 as bugfix, which changes behavior a lot and should have been considered as a breaking change - actually it's enforce tainted numerics instead of allow tainted numerics

  Fixes
  Allow tainted numerics except for 'html' and 'has_quotes' by @cgocast in #10242

• the meaning of the documented taint types at https://psalm.dev/docs/security_analysis/#taint-types were changed

  sql - used for strings that could contain SQL
  shell - used for strings that could contain shell commands
  ...

• it is not complete (as outlined in previous comments already), here is another example that came to my mind
  • shell_exec(escapeshellcmd((int)$_GET['inject'])); is not considered tainted
  • shell_exec((int)$_GET['inject']); is considered tainted
• fixing the those tainted numerics is time consuming and difficult to impossible when dealing with 3rd party code and libraries, examples
  • Symfony Access-Control (authentication) can be configured in multiple different ways, e.g. via routes or (custom), tags or attributes → simply adding a @psalm-taint-escape to all occurrences is not possible for those scenarios (except writing a complex custom plugin that is parsing route configuration and the roles model)
  • Laravel CSRF tokens and other similar techniques that are for instance signing server-side request params using HMACs are located in different framework layers (templating/view or routing/dispatcher) and cannot be handled by the taint-flow graph
• existing TaintTest case were adjusted to match the new code - those tests were there for good reasons

tl;dr: Instead of applying "follow-up work" on an incomplete concept, I'd like to see those changes being reverted and probably re-implemented as explicit opt-in feature.

from psalm.

Well, I believe the example in the issue (psalm.dev/r/0d4f65473b) was on point.

I agree to @mmcev106 that it is inconsistent because escaped strings are problematic in the same way:

class A {
    public function deleteUser(PDO $pdo) : void {
        $userId = self::getUserId();
        $sth = $pdo->prepare("delete from users where email = ?");
        $sth->bindValue(1, $_GET["email"]);
        $sth->execute();
    }
}

from psalm.

I found these snippets:

<?php // --taint-analysis

$i = (int) $_GET['i'];

// This does not cause a taint:
echo $i;

// This does, but shouldn't:
echo "The integer is: $i";

Psalm output (using commit a75d26a):

ERROR: TaintedHtml - 9:6 - Detected tainted HTML

ERROR: TaintedTextWithQuotes - 9:6 - Detected tainted text with possible quotes

from psalm.

Well, I do agree that both of your examples should behave the same way.

However, I'm not certain which way it should go.

The relevant issue/discussion we had is here: #10238

The example snippet is indeed bad. you could also alter it to show that a LIMIT $i could be dangerous in some cases and there are probably many cases I don't think of.

TaintedHTML (and more widely, any echo/print in the code) is more complex though, it doesn't deal with performance critical systems and if it's designed to be returned to the client, it's mostly safe.
However, you could be using html to generate a quotation PDF and the client managed to push 0 for $i in "Total: $".$i so this is a legit taint I believe
You could also be echoing something that you retrieve through ob_ functions to do something critical, it's hard to tell

Contrary to the "normal" analysis of Psalm, which aims to reduce false positive to an acceptable compromise, we should probably aim for security based analysis to reduce false negative to a minimum because people may rely on it too much and create a false sense of security

from psalm.

@orklah, I'm not sure I understand how an integer value could be considered tainted, even for SQL. Tainted SQL becomes problematic when there is risk of SQL injection, which is not an issue for integers. It is a valid use case for the application to allow any integer to be passed from user input to a SQL query, and up to the application to determine if permissions allow that action on that integer. You can pass any integer with prepared statements as well (which of course does not and should not report a taint). I think the logic in #10238 that lead up to this change might be flawed...

Since htmlspecialchars() resolves TaintedHTML, I believe integers should always be considered untainted in that case because they can't contain any of the chars that could cause injection (those escaped by htmlspecialchars()).

Am I missing something?

from psalm.

Tainted SQL becomes problematic when there is risk of SQL injection

That's the most obvious case yes, but imagine an application with pagination. You have an option that lets you display 10 items per page, 20 items per page or 50 items per page. I'd absolutely want to be warned if that option could be manipulated to display 999999999 items (this may look like a mild side effect, but if some kind of processing is done to display the items, this is an easy entry point for a DoS attack)

The article says it better than me:

DoS User Specified Object Allocation
If users can supply, directly or indirectly, a value that will specify how many of an object to create on the application server, and if the server does not enforce a hard upper limit on that value, it is possible to cause the environment to run out of available memory. The server may begin to allocate the required number of objects specified, but if this is an extremely large number, it can cause serious issues on the server, possibly filling its whole available memory and corrupting its performance.

No amount of prepared statement can protect you against this

up to the application to determine if permissions allow that action on that integer

I agree, and you should flag whatever function doing that in your application as an escape function for this purpose. In my example above, I should probably check that the integer is in an acceptable range.

Am I missing something?

Well, I believe the example in the issue (https://psalm.dev/r/0d4f65473b) was on point. You can have malicious/manipulated integer that can end up in your sql that are not sql injection (in the example given, an attacker client could use a badly thought website in order to delete any user in the database. The fix for that is checking whether said client has authorizations to delete said user. For example, this: https://psalm.dev/r/e6ed2fc730)

I'm aware this could be tedious and maybe we should have a plugin/config that controls wether integer can transmit a taint, but again, security is a critical domain where we must avoid false negatives when we can and my first assumption (that I shared with you) was kinda proven wrong in the discussion we had

from psalm.

I found these snippets:

<?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 a75d26a):

ERROR: TaintedSql - 6:20 - Detected tainted SQL

<?php //--taint-analysis

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

    public static function getUserId() : int {
        return (int) $_GET["user_id"];
    }
   
    /**
     * @psalm-taint-escape sql
     */
    public function isDeletionAllowed(int $userId): bool{
     	//internal check for permission
        return true;
    }
}

Psalm output (using commit a75d26a):

No issues!

from psalm.

You can have maliciously manipulated strings just as easily integers. Only application logic is qualified to validate either. I'm concerned we're straying into new and inappropriate territory by trying to police valid values, while we should perhaps stick to policing injection vulnerabilities as we've done up until this point.

To play devil's advocate, what you’re suggesting might eventually lead to considering all user input to be “tainted” even after escaping, and require allow listing or otherwise notifying psalm that a value is safe. This behavior could be a configuration option.

In any case, the current behavior is a pretty big conceptual change. I'd propose reverting commit #94a98c until we find a solution that is at least logically consistent with itself.

from psalm.

I agree with you @mmcev106 that follow-up work is needed on the topic of tainted integers, specially with the example you gave.

On the one hand, I also agree with @orklah when he says that we should minimize false negatives when run psalm with --taint-analysis option. Therefore, I expect both echo statements to raise a TaintedHtml by default.

On the other hand, I understand your point of view that integers should not be considered tainted for specific cases. Thus, I approve @orklah's suggestion to offer a configuration option to disable tainted integers. It could be in the likes of:

<psalm
  disableTaintedIntegers="[string]"
>

For instance you could set this option to "html, has_quotes" to suppress all issues in the example you gave. You could go even further by setting it to "html, has_quotes, sql" if you believe TaintedSql issues should be raised only for classical SQL injections.

But again, I think it is a better practice to handle issues on a case-by-case basis with a @psalm-taint-escape annotation as showed by @orklah orklah.

from psalm.

The issues here go deeper. If we consider integers to be tainted for SQL sinks, then for consistency this would suggest integers set via bindParam() should be considered tainted as well. At this point, it is logically inconsistent to consider integer bound parameters to be tainted, but not string bound parameters. This significantly changes the definition of "taint" in a way that I'm not sure is what we want, and would require sweeping changes for full consistency.

Please really consider this, as it is very problematic. If the above paragraph is not fully making sense, please ask me to clarify it. In the mean time, my app is stuck on 5.15.0, as upgrading causes dozens of non-sensical false positives. I could escape those of course, but that would not make logical sense in our cases, and could potentially mask actual taints down the road.

from psalm.

You are right. Method PDOStatemen::bindParam() should be fine-tuned such that:

• $stmt->bindParam('param', $mystring, PDO::PARAM_STR) removes the taint on $myString by default
• $stmt->bindParam('param', $myInt, PDO::PARAM_INT) leaves $myInt unchanged by default

Unfortunately, I lack time do this "follow-up work" anytime soon. If it is that problematic for you, you might want to give it a shot. Here is an example of code that removes taints depending on arguments: https://github.com/vimeo/psalm/blob/5.x/src/Psalm/Internal/Provider/AddRemoveTaints/HtmlFunctionTainter.php

from psalm.

@cgocast, why would the taint be removed from the string, but not the int?!? Strings are much riskier when it come to SQL injection. This is logically inconsistent. I still believe removing the taint from the int is the most appropriate solution to resolve this inconsistency (in this and all cases).

from psalm.

Because the sample from OWASP protects your code only against malicious strings.

In my opinion, there are cases where malicious integers can end up in your SQL queries and bindParam() is helpless. That's why I wish to fine-tune it.

Again, for some applications (as yours apparently) it is irrelevant to consider that integers can be tainted. That's why I like orklah's proposal for a configuration option to disable tainted integers.

from psalm.

@cgocast & @orklah, I agree that integers can be malicious. What do you think about the potential for string taints to be even more malicious, and the value of consistent tainting behavior both between types and with the definition of a "taint" historically in psalm? I feel like there is a communication disconnect around the greater logic & implications of this change. I'd be happy to hop on a call using any app of your choice if that would be the easiest way to resolve the confusion.

from psalm.

I found these snippets:

<?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 c488d40):

ERROR: TaintedSql - 6:20 - Detected tainted SQL

from psalm.

Please @ohader, can you give me a few Symfony and/or Laravel code samples that you consider false positives ? This would help me a lot to re-implement tainted numerics.

from psalm.