Code vulnerable for SQL injections

[YOUR1]

Hi,

I see a lot of lines like this function:

	public function getBatch ( $id ) {
		$id *= 1 ;
		$ret = array('commands'=>array()) ;
		$db = $this->getDB() ;
		$sql = "SELECT * FROM command WHERE batch_id=$id" ;
		if(!$result = $db->query($sql)) return $this->setErrorMessage ( 'There was an error running the query [' . $db->error . ']'."\n$sql" ) ;
		while ( $o = $result->fetch_object() ) {
			$j = json_decode ( $o->json ) ;
			$j->_meta = $o ;
			unset ( $j->_meta->json ) ;
			$ret['commands'][$o->num] = $j ;
		}
		return $ret ;
	}

If one would simply set $id to 0;DROP TABLE COMMAND;, the command table will be gone. Easy as that.. Please, for the love of god; use SQL prepared statements. See: https://www.w3schools.com/php/php_mysql_prepared_statements.asp

[lucaswerkmeister]

The $id *= 1 at the start of the method effectively casts it to a number or throws an exception, so by the time of the $sql assignment it should be safe. I know the code looks scary, but IIRC I looked through it at some point and didn’t find any remaining places that were actually vulnerable.

[lucaswerkmeister]

(Correction by @GreenReaper: throwing an exception is new in PHP 8, in PHP 7 it raises a warning and evaluates to 0, or maybe it depends on some kind of strictness setting too. But at no point does the string survive into the $sql, I believe.)

[GreenReaper]

Could avoid the exception with type coercion to int if that is desired. As it is, it'd probably still take a float, which may not be desirable to present to SQL that expects an int (I don't think you can pass in INF or NAN; they'd be taken as strings, not constants).

In the general case, yes, it's probably a good idea to use parameterized queries in new code and seek to convert old ones, because it's easy to slip up.

[YOUR1]

The $id *= 1 at the start of the method effectively casts it to a number or throws an exception, so by the time of the $sql assignment it should be safe. I know the code looks scary, but IIRC I looked through it at some point and didn’t find any remaining places that were actually vulnerable.

Maybe in this particular example.. But that's not the point. Not using prepared statements is not something you would like to do..