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

Backdoor or security vulnerability? #823

Closed
JarLob opened this issue Mar 13, 2019 · 17 comments
Closed

Backdoor or security vulnerability? #823

JarLob opened this issue Mar 13, 2019 · 17 comments

Comments

@JarLob
Copy link

@JarLob JarLob commented Mar 13, 2019

public function where($whereProp, $whereValue = 'DBNULL', $operator = '=', $cond = 'AND')
{
// forkaround for an old operation api
if (is_array($whereValue) && ($key = key($whereValue)) != "0") {
$operator = $key;
$whereValue = $whereValue[$key];
}
if (count($this->_where) == 0) {
$cond = '';
}
$this->_where[] = array($cond, $whereProp, $operator, $whereValue);
return $this;
}
is vulnerable to SQL injection in functon Where() because of the special "forkaround".

If $whereValue happens to be an array, non parameterized $operator value is extracted from it.
However typical usage of the class looks like:

$db->where('ID', $_POST['id']);
$name = $db->getValue('USERS', 'name');

The $whereValue is usually untrusted and if there are no additional checks like is_numeric($_POST['id']) an attacker may inject his statements. For example: id[= ? or 1=1 --]=0 (Url encoded version for HTTP POST: id%5B%3D%20%3F%20or%201%3D1%20--%5D=0)

@avbdr

This comment has been minimized.

Copy link
Collaborator

@avbdr avbdr commented Mar 13, 2019

thanks for your message.

$db->where('ID', $_POST['id']);

this is looks like a missing cast to int. I'm not sure are you calling this a 'regular usage' while data sanitizing is a primary junior mistake.
unfortunately, mysqlidb is a helper, but not a magic. it have no idea of data types

@satalways

This comment has been minimized.

Copy link

@satalways satalways commented Mar 13, 2019

thanks for your message.

$db->where('ID', $_POST['id']);

this is looks like a missing cast to int. I'm not sure are you calling this a 'regular usage' while data sanitizing is a primary junior mistake.
unfortunately, mysqlidb is a helper, but not a magic. it have no idea of data types

Type casting doesn't matter in MySQLiDB (Class).
$db->where('ID', $_POST['id']);
and
$db->where('ID', (int) $_POST['id']);
both generate same query.. You can check using
echo $db->getLastQuery();

@JarLob

This comment has been minimized.

Copy link
Author

@JarLob JarLob commented Mar 13, 2019

s looks like a missing cast to int. I'm not sure are you calling this a 'regular usage' while data sanitizing is a primary junior mistake.
unfortunately, mysqlidb is a helper, but not a magic. it have no idea of data types

I strongly disagree, user input validation and sanitization is an additional measure, but not a replacement for parameterized queries. MySQLiDB class doesn't concatenate query parameters, but uses ? placeholders after all. However in this case if an array is passed through its index value gets injected into query statement.

@satalways

This comment has been minimized.

Copy link

@satalways satalways commented Mar 13, 2019

That's why I've written an extended class to use MySQLiDB

@avbdr

This comment has been minimized.

Copy link
Collaborator

@avbdr avbdr commented Mar 13, 2019

clothing as not a bug

@avbdr avbdr closed this Mar 13, 2019
@avbdr

This comment has been minimized.

Copy link
Collaborator

@avbdr avbdr commented Mar 13, 2019

so the problem that mysqlidb doesn't support parametrized queries, and not a security vulnerability.
library never been advertising parametrized queries. I would love to see that implemented, but I'm completely short on time to do that.
if you can send a patch for that, I will be happy to review.

thanks for your time

@JarLob

This comment has been minimized.

Copy link
Author

@JarLob JarLob commented Mar 13, 2019

NP, the pleasure is mine. </EndOfSarcasm>

library never been advertising parametrized queries

It is written in the description of the project "Wrapper for a PHP MySQL class, which utilizes MySQLi and prepared statements."

It might be naming misunderstanding, you do use bind parameters also known as "prepared statements" or "parameterized statement" and it is what I meant https://en.wikipedia.org/wiki/Prepared_statement

It is a security vulnerability and your resistance to admit it may make some people believe it was introduced on purpose after all because the patch is just to remove the four lines of special treatment of arrays.

@satalways

This comment has been minimized.

Copy link

@satalways satalways commented Mar 13, 2019

Sorry for little delay. How can I share my extended class.. I've also other functions like drop_table, drop_column, is_mysql_table() etc.

@avbdr

This comment has been minimized.

Copy link
Collaborator

@avbdr avbdr commented Mar 13, 2019

feel free to breakdown new features as 1 patch per feature, include test cases and documentation and feel free to send pull requests.
as well, your naming conventions are not the same as library is using.

@satalways

This comment has been minimized.

Copy link

@satalways satalways commented Mar 13, 2019

I am sorry, But I don't know how to do it..

@satalways

This comment has been minimized.

Copy link

@satalways satalways commented Mar 13, 2019

Anyway, here is my method in extended class.

protected function replacePlaceHolders( $str, $vals ) {
$i = 1;
$newStr = "";

	if ( empty( $vals ) ) {
		return $str;
	}

	while ( $pos = strpos( $str, "?" ) ) {
		$val = $vals[ $i ++ ];
		if ( is_object( $val ) ) {
			$val = '[object]';
		}
		if ( $val === null ) {
			$val = 'NULL';
		}
		
		switch ( gettype( $val ) ) {
			case 'integer':
			case 'float':
			case 'double':
				$newStr .= substr( $str, 0, $pos ) . $val;
				break;
			default:
				$newStr .= substr( $str, 0, $pos ) . "'" . $val . "'";

		}

		$str = substr( $str, $pos + 1 );
	}
	$newStr .= $str;

	return $newStr;
}
@JarLob

This comment has been minimized.

Copy link
Author

@JarLob JarLob commented Mar 13, 2019

No offence @satalways, but your suggestions have nothing to do with the injection. replacePlaceHolders is used only for tracing. Calling DB with prepared statements (that are also called parameterized statements in the official php documentation) is the safe technique.

The library does call bind_param and does advertise using it. The correct fix is to break some legacy compatibility and prevent untrusted user input getting into $operator. The function has a separate argument for passing operator and only if a programmer allows user input passed to the $operator, same as to $whereProp or $cond, it is his own fault.

@VeNoMouS

This comment has been minimized.

Copy link

@VeNoMouS VeNoMouS commented May 21, 2019

I think @JarLob 's point is valid... mysqli is meant to about binding to limit security problems like sql injection attacks etc...

@killua-eu

This comment has been minimized.

Copy link

@killua-eu killua-eu commented Aug 23, 2019

@JarLob , would you mind sharing the fix?

@JarLob

This comment has been minimized.

Copy link
Author

@JarLob JarLob commented Aug 24, 2019

Oh, @killua-eu, the fix is trivial, you delete:

         // forkaround for an old operation api 
         if (is_array($whereValue) && ($key = key($whereValue)) != "0") { 
             $operator = $key; 
             $whereValue = $whereValue[$key]; 
         } 

from the where function and stop extracting the $operator from user input or check the extracted operator against a fixed list of allowed operators if the feature is absolutely needed.

@avbdr

This comment has been minimized.

Copy link
Collaborator

@avbdr avbdr commented Aug 24, 2019

I appologies, I have payed needed attention to the report since initially it was not full enough and i was not wise enough to think.
Issue is resolved in current master and new release been made.
I will add a security warning in the readme with a credit to you.
Thank you for your help.

@killua-eu

This comment has been minimized.

Copy link

@killua-eu killua-eu commented Aug 24, 2019

@JarLob , thanks lots for helping out! @avbdr cheers for merging this in and taking care for the class 👍

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

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.