0
\$\begingroup\$

This is a function that reads a setting data from DB.

    public function getSetting(Request $request)
    {
        $user = Auth::user();
        $tableName = $request->name;
        $setting = \DB::table($tableName)->where('id', $user->id)->first();
        return response()->json($setting, '200', ['Content-Type' => 'application/json','Charset' => 'utf-8'], JSON_UNESCAPED_UNICODE);
    }

Do you think it is okay to put the table name from GET parameter.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ It is open way for any logged in user to query any arbitrary table. Don't do that. \$\endgroup\$
    – Tpojka
    Commented Jan 20, 2022 at 23:23

4 Answers 4

2
\$\begingroup\$

This very much depends on what DB::table() is doing with the table name. If it only accepts string that are valid table names and throws exception otherwise, then it would be ok if you actually caught that exception and handle it.

try {
  $table = \DB::table($tableName);
} catch (InvalidTableNameException $e) {
  $this->logInvalidTableNameException($e); // might be good to log this stuff
  return response()->json(['error' => 'invalid_table_name'], 400)
}

$table->where(....
\$\endgroup\$
3
  • 1
    \$\begingroup\$ table(...) doesn't execute the query so no exception would be thrown at that point \$\endgroup\$
    – lagbox
    Commented Jan 21, 2022 at 18:38
  • \$\begingroup\$ @lagbox why would I need to execute any query to tell if a string is a valid db table name. I only need to know what is supported by my database. I don't know laravel, I didn't even realize DB::tablr is laravel code. But it's not important. It's a black box. I said IF it check validity, handle it, if it does not (what a shame), implement your own check... \$\endgroup\$
    – slepic
    Commented Jan 22, 2022 at 8:01
  • \$\begingroup\$ right, i was just informing of how query builder works since this answer says "it depends on what DB::table()` is doing, that is all, just FYI since you didn't know and for the OP since they are specifically using Laravel \$\endgroup\$
    – lagbox
    Commented Jan 22, 2022 at 16:35
2
\$\begingroup\$

User input should be considered unsafe, since it may lead to SQL injection that may leak, destroy or modify information, thus we must protect against misuse. For security reasons the least information you release the better off you are.

Thus

Do not allow SQL Injection in column/table names nor reveal them.

// 1.- Best remap your column/table/db names (keys) to safe undisclosed ones
/**
 * Change unsafe keys for safe keys
 * remapKeys(['id'=>1, 'n'=>'Susan', 'm'=>3],  ['id'=>'user_id', 'n'=>'user_name', 'm'=>'friend'])
 *  [user_id=>1, user_name=>'Susan', 'friend'=>['user_id'=>3]
 *
 * @param array $inputArray ;
 * @param array $remapRules
 * @return array
 * @require php >= 8.0
 *
 */
    function remapKeys(array $inputArray, array $remapRules):array {
        $remapped = [];
        foreach($inputArray as $key => $value)
            if(array_key_exists($key, $remapRules) || is_numeric($key))
                $remapped[$remapRules[$key]] = is_array($value) ?
                    remapKeys($value, $remapRules) :
                    $value;
        return $remapped;
    }
    $in = ['id'=>1, 'n'=>'Susan', 'm'=>['id'=>3]];
    $protected = remapKeys($in, ['id'=>'user_id', 'n'=>'user_name', 'm'=>'friend']);
    // $protected = [user_id=>1, user_name=>'Susan', 'friend'=>['user_id'=>3] ]

// 2.- Second best validate your  column/table/db names, something like
    $in = ['user_id'=>1, 'user_name'=>'Susan', 'friend'=>['user_id'=>3], 'injection'=>'evil'];
    $protected =  array_intersect_key($in, ['user_id'=>'int', 'user_name'=>'string', 'friend'=>'array']);

//3.- If not at least the third best option: Protect your  column/table/db names
    $sql = "SELECT " . fieldIt($_REQUEST['column_name']) . 
        " FROM " . fieldIt($_REQUEST['table_name']) .
        " WHERE " . fieldIt($_REQUEST['filter_column_name']) . "=?";
    
/**
 * Protect with ` an sql name
 * Quotes a: column/table/db name to `column name` respecting . table.column to `table`.`column`
 *
 * @param string|Stringable $fieldName
 * @return string
 */
function fieldIt($fieldName):string {
    $protected = [];
    $n = explode('.',$fieldName);
    foreach($n as $field) {
        $protected[]= '`'.str_replace(['`',"\r","\n","\t","\0","\8","\\"], '', trimSuper($field) ).'`';
    }
    return implode('.', $protected);
}

/**
 * Trim a string, or array of strings, to a one liner with single spaces
 *
 * @param string|Stringable|array $str
 * @return:string|array
 */
function trimSuper($str):string|array {
    if($str === null)
        return '';
    if(is_array($str)) {
        foreach($str as &$d)
            $d = strim($d);
        return $str;
    }
    $s1 = preg_replace('/[\pZ\pC]/muS',' ',$str);
    return trim(preg_replace('/ {2,}/muS',' ',$s1));
}
\$\endgroup\$
0
1
\$\begingroup\$

The Laravel query builder is supposed to protect against SQL injection. But if you provide an invalid table name this code will trigger an exception, that may possibly result in information disclosure ie file paths, SQL queries, debug stacktrace. Did you try it ?

Leaking the table name is information disclosure in itself, that can potentially be leveraged by an attacker if a vulnerability is found on your site.

The code proposed by @slepic is an improvement, but it can also be used for enumeration attacks to determine valid table names on your system. That does not mean it can be hacked, but you are giving away too much information.

\$\endgroup\$
0
1
\$\begingroup\$

This allows SQL injection

According to the docs:

PDO does not support binding column names. Therefore, you should never allow user input to dictate the column names referenced by your queries, including "order by" columns.

Use a switch, match (PHP 8) or even an enum (PHP 8.1) to allow the user to select from a list of table names you want them to be able to access.

Why ID?

where('id', $user->id)

This bit makes no sense unless you are querying the user table. Otherwise it should be called user_id. The id column should always be the primary key of the table.

Use use

My linter recommends you use Illuminate\Support\Facades\DB;, probably because it makes it easier to keep track of what else you're using in your file.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.