Backdoor or security vulnerability?

Lines 969 to 983 in eaf1f6c

    
           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)

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

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();

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.

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

clothing as not a bug

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

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.

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.

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.

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

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;
}

@satalways

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.

@JarLob

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

@JarLob

@JarLob , would you mind sharing the fix?

@killua-eu

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.

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.

@JarLob

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

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

Backdoor or security vulnerability? #823

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	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;
	}

Sunbelt Computer Software

PL/B Language Development and Support

Backdoor or security vulnerability? #823

Description

Activity

avbdr commented on Mar 13, 2019

satalways commented on Mar 13, 2019

JarLob commented on Mar 13, 2019

satalways commented on Mar 13, 2019

avbdr commented on Mar 13, 2019

avbdr commented on Mar 13, 2019

JarLob commented on Mar 13, 2019

satalways commented on Mar 13, 2019

avbdr commented on Mar 13, 2019

satalways commented on Mar 13, 2019

satalways commented on Mar 13, 2019

JarLob commented on Mar 13, 2019

VeNoMouS commented on May 21, 2019

killua-eu commented on Aug 23, 2019

JarLob commented on Aug 24, 2019

avbdr commented on Aug 24, 2019

killua-eu commented on Aug 24, 2019

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions