Skip to content

Commit

Permalink
Fix sql injection in user exists request
Browse files Browse the repository at this point in the history 
Signed-off-by: William Desportes <williamdes@wdes.fr>
  • Loading branch information
@williamdes
williamdes committed Jan 1, 2020
1 parent 9f82b71 commit c86acbf
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion libraries/classes/Server/Privileges.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3067,7 +3067,7 @@ public static function getExtraDataForAjaxBehavior(

if (isset($_GET['validate_username'])) {
$sql_query = "SELECT * FROM `mysql`.`user` WHERE `User` = '"
. $_GET['username'] . "';";
. $GLOBALS['dbi']->escapeString($_GET['username']) . "';";
$res = $GLOBALS['dbi']->query($sql_query);
$row = $GLOBALS['dbi']->fetchRow($res);
if (empty($row)) {
Expand Down

8 comments on commit c86acbf

@IanRobertson-wpe
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason this isn't using a parameterized query?

@williamdes
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IanRobertson-wpe Yes, prepared statement where implemented in #15492

@desmond4elf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Luck to the besta Team ♥
is 5.0.1 is beta or stable cuz i`t was wrote -> Version information: 5.0.1, ((latest stable version: 4.9.4))

@ibennetch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phpreza Because of #15860 the "current version" was incorrectly displayed with phpMyAdmin 5.0.0 and 5.0.1. That should be fixed with the next release, which will be 5.0.2.

@zeront4e
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamdes Is there a specific reason, that the serverity of this vulnerability is "serious"? Is it because of the "phpMyAdmin configuration storage", since this injection concerns the mysql control connection?

@ibennetch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeront4e I'm not sure I follow the purpose of your question; are you asking because you think it should be downgraded to "moderate" or something similar? Because the attacker needs access to the server, it could be argued that this is not severe, but we felt that the possibility of exploiting this in a shared hosting environment justified the serious rating.

Is it because of the "phpMyAdmin configuration storage", since this injection concerns the mysql control connection?

I don't believe this has to do with the control user or phpMyAdmin configuration storage.
It could be that we're talking about two different things, but

@zeront4e
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibennetch Thank you for the answer! I'm just investigating the vulnerability as part of my computer science studies. We tried to exploit the vulnerability in practice (in an isolated environment). To achieve that it is necessary to gain additional privileges in a multi-user environment.

We just noticed that phpMyAdmin executes queries with the user credentials, provided on the login page, when using HTTP- or cookie-based authentication. Because of that it seemed hard to "exploit" the vulnerability, particularly without modifying phpMyAdmin.

The security policy of phpMyAdmin states that SQL injections are serious if they concern "the mysql control connection", which could lead to additional privileges. So we weren't sure if we missed some important information to gain higher privileges, especially by using this SQL injection vulnerability.

That's the reason for my previous question. :)

@ibennetch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see; that makes sense. There is no further escalation involved and it doesn't exploit the control user, we just (somewhat arbitrarily) felt that was an appropriate severity for such a report. Good luck with your studies.

Please sign in to comment.