Taint should be ignored on int cast #10521
Comments
|
I found these snippets: https://psalm.dev/r/c775928c77<?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"; |
|
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. 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 |
|
@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 Am I missing something? |
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:
No amount of prepared statement can protect you against this
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.
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 |
|
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"];
}
}https://psalm.dev/r/e6ed2fc730<?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;
}
} |
|
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. |
|
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 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: For instance you could set this option to But again, I think it is a better practice to handle issues on a case-by-case basis with a |
|
The issues here go deeper. If we consider integers to be tainted for SQL sinks, then for consistency this would suggest integers set via 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. |
|
You are right. Method PDOStatemen::bindParam() should be fine-tuned such that:
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 |
|
@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). |
|
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 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. |
|
@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. |
|
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:
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. |
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();
}
} |
|
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"];
}
} |
|
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. |
@cgocast, I believe commit #94a98c may need some follow-up work. It's causing dozens of false positives for us, which I have simplified to the following example:
https://psalm.dev/r/c775928c77
I don't believe appending a hard coded string to an untainted integer value should cause it to become tainted. I think the same incorrect logic might also apply to
taintedInputAfterIntCast, as prepending"aaaa"is something the developer has explicitly chosen to do (not a taint). Am I missing something?The text was updated successfully, but these errors were encountered: