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.

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

3 Answers 3

Reset to default
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(....
Improve this answer
\$\endgroup\$
3
  • 1
    \$\begingroup\$ table(...) doesn't execute the query so no exception would be thrown at that point \$\endgroup\$
    – lagbox
    Jan 21 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
    Jan 22 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
    Jan 22 at 16:35
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.

Improve this answer
\$\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.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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