restrict some types of uploads

[Abdallah-Fouad-X]

Describe the bug

in import.php
when i import new database there is no validation while the uploaded extension , for example i uploaded img and i name it like that : <svg/onload=alert(document.cookie)>

and i see the successful upload image and the app try to show me that error but there isn't any shown errors from it

To Reproduce

Steps to reproduce the behavior:

1. Go to 'import.php'
2. Click on 'select database'
3. Scroll down to 'select the named image'
4. See error

Expected behavior

A clear and concise description of what you expected to happen.
show me the error from inside the image , i injected evil function with php

Server configuration

• Operating system: win10
• phpMyAdmin version: 4.9.1

Client configuration

• Browser: firfox
• Operating system: win

Additional context

as you can see in the uploaded img
on of them show the error from the content file the uploaded php file
but when i create image with that name it's uploaded but can't read the evil function from the image

[techievivek]

I am trying to look into this issue and would like to suggest that we do frontend validation for the supported file type.
One way is

<input type="file" name="import_file" id="input_import_file" accept=".csv,.sql,.xml,.txt,.ods,.atx,.ain,.aih,.dbf">

And we can also have backend logic to check file extension and then do the database query.
Here are some possible shapefiles , I have chosen one that is related to database operation(.atx,.ain,.aih,.dbf)
I hope this might help.

[Abdallah-Fouad-X]

The most secure way is make validation on the file type (extension) in the back end , in the fronted it can bypass with (intercept the request via web proxy and change the extension with any thing else to escape from the front validation)

the back end validation to check related database operation

recommendation :

Add new function in the back end like :

if(isset($images)) {

//do this for each file
foreach($images['name'] AS $key => $name) {

//get the extension of the file to make sure its an image
   $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION));
                   //if the file has the right extension and is less than 500 KB, do the upload... 
		if (in_array($ext, ['jpg', 'gif', 'png','bnp']) && $images["size"][$key] < $size_per_upload && $images["size"][$key] > 0) {

    $source = $images['tmp_name'][$key];   
    $target = $path_to_image_directory . $name;
 
    move_uploaded_file($source, $target);
	}
}

}

(change the the image with file type and add the used extensions (CVS , SQL , XML , TXT ))

and if this process succeeded : move to next step to validate your sql check
else that die the connection with the error case

I hope that help

[techievivek]

Yes, i agree to you. I suggested for both the frontend as well backend.

[Abdallah-Fouad-X]

Sure , because it missing for secure validation in back end also , i hope that fix soon

[tlshifat]

Hello . I am doing this fix . Someone please send me list of valid file extension to upload.
Thanks.

[Abdallah-Fouad-X]

You will find the extension in @techievivek first comment

@ibennetch @williamdes
Are this will be in 2.9.3 updates ?!

[tlshifat]

Okay I have done exactly like that comment for my testing. All ok then. @OverRide-BT

[Abdallah-Fouad-X]

Are this fixed!

[tlshifat]

Yes it is fixed in my local . Will send PR .
I want to alert the following message . is it okay? what you think?

"Invalid file format . Valid extensions ('csv','sql','xml','txt','ods','atx','ain','aih','dbf','gz','zip','bz2')
File may be compressed (gzip, bzip2, zip) or uncompressed.
A compressed file's name must end in .[format].[compression]. Example: .sql.zip"

[tlshifat]

@OverRide-BT

[ibennetch]

I don't really see the problem here; as far as I can tell the code isn't being interpreted when it's loaded. It's up to the user to upload the correct file — since we accept many file types, and some users don't store SQL in files with a .sql extension, it seems it would be counter-productive to restrict users.

Are you saying you're able to upload arbitrary non-SQL code or view arbitrary files from the server's file system through this? I'm a little confused about whether the initial report is that an evil user can run commands or that an innocent user can inadvertently upload a file that can't be processed.

[Abdallah-Fouad-X]

Great , i will test it again.
If you can test this on my local machine also to make confirmation about it .
If you can upload the update php file tbl_import , import. Php or any update you make it on the code

[Abdallah-Fouad-X]

@ibennetch about you question :
Are you saying you're able to upload arbitrary non-SQL code or view arbitrary files from the server's file system through this?

yes i have already uploaded img .jpg extension as you can see and the successful message appear and this will lead to inject evil codes in the image ,And i believe that if the uploaded accepted jpg file injected by php evil function , it can accept python upload and running it the back end when the user check it and for worst cases lead to open session over the internal server

also when name the uploaded file in wired name like any javascript code it will ignore the file content and show you the file have problem with the SQL codes ?!
and there is no validation to check the file content when you can bypass the filters with the uploaded file in code name like svg/onload=alert().jpg
then it completely deal with the file name as the SQL commend and start the process by the file name (also that not spouses to be application logic )

[Abdallah-Fouad-X]

Yes it is fixed in my local . Will send PR .
I want to alert the following message . is it okay? what you think?

"Invalid file format . Valid extensions ('csv','sql','xml','txt','ods','atx','ain','aih','dbf','gz','zip','bz2')
File may be compressed (gzip, bzip2, zip) or uncompressed.
A compressed file's name must end in .[format].[compression]. Example: .sql.zip"

Great fix ! but are you checking the file content !!!!?

[ibennetch]

yes i have already uploaded img .jpg extension as you can see and the successful message appear and this will lead to inject evil codes in the image

I see in the images that you posted that the file did upload, but when MySQL tried to interpret the commands, as it's not valid SQL there are syntax errors. So I don't see anywhere where the file contents are interpreted by PHP or the system, it's all run through the MySQL interpreter. So I don't see yet how this could be used to run arbitrary PHP, Python, or evil image files. Are we talking about the same thing here?

[Abdallah-Fouad-X]

yes i have already uploaded img .jpg extension as you can see and the successful message appear and this will lead to inject evil codes in the image

I see in the images that you posted that the file did upload, but when MySQL tried to interpret the commands, as it's not valid SQL there are syntax errors. So I don't see anywhere where the file contents are interpreted by PHP or the system, it's all run through the MySQL interpreter. So I don't see yet how this could be used to run arbitrary PHP, Python, or evil image files. Are we talking about the same thing here?

Acutely the uploaded process complete successfully as you can see in the provided screenshot, And deal with the image name as the SQL quarry
Just quickly method (make the image name like Drop pr Delete quarry) then the upload process will deal with the name as sql quarry

[williamdes]

@OverRide-BT we are having a hard time understanding what is the vulnerability
Please send screenshots as a proof that you can be malicious to the server

[tlshifat]

I dont think there is a vulnerability , its just for preventing user from uploading wrong files. And It should be done because even if someone uploads a wrong file phpmyadmin tries to execute. Which leads to database hang.

[Abdallah-Fouad-X]

I dont think there is a vulnerability , its just for preventing user from uploading wrong files. And It should be done because even if someone uploads a wrong file phpmyadmin tries to execute. Which leads to database hang.

Good way to explain .... ,But there is way to manage to execute an sql commend to drop or delete database over the file content as i said

so i created table called e then i uploaded an txt file contain this line :
DROP TABLE e;

the the sql quarry executed and drop the table
this must not to be in the business logic

as you can see this is the evil method i have talk about it in my last comment
so i don't know are you have an idea about this should be happen or not or that is normal in the application logic or not
@ibennetch @williamdes

[tlshifat]

@OverRide-BT Thanks for your great and logical explanation. I have done the fixation long ago but still its waiting for merging. Lets see they approve it or not.

[Abdallah-Fouad-X]

@OverRide-BT Thanks for your great and logical explanation. I have done the fixation long ago but still its waiting for merging. Lets see they approve it or not.

Thank you, and the most critical thing in this case when the user upload any SQL evil functions (Drop or Delete) the application did not alert the user with the Drop process , so that make it even worse in the application behavior

[emanuelb]

to summarize the security improvements from this thread that are relevant in import upload (there are no vulnerabilities)

• limit file extensions allowed in import.php (can be restricted by default, with config option to allow more file-extensions for people who use non-standard extensions)
• file-content can also be checked for example by using the SQL parser (file won't help, it return ASCII text for tested .sql files) which is problematic as the SQL parser also may report valid sql files as invalid (as it's not support everything?) so maybe add config for that one as well to disable?
• SQL queries imported can be dangerous/not-expected? (such as not generated by the export option) which I think is by design (to allow other user-supplied files upload), but can be improved by text (to explain that the SQL imported are not filtered at all, it's not just import, but text-content to execute as SQL) or by using the SQL parser to detect possible 'unwanted' operations and ask the user to allow them? maybe have mode that will run each query after approving by user (interactive mode)?

[Abdallah-Fouad-X]

@emanuelb , Yes indeed , but you can make blacklist for evil function like drop or delete

in case upload file contain those words it will be delete or drop the table such this function is bad for the application logic , if you drop or delete database by uploading file

[emanuelb]

@Abdallah-Fouad-X

Yes indeed , but you can make blacklist for evil function like drop or delete

Blacklists are bad and it's easy to bypass them, there are other commands to achieve the results like deleting data such as: ALTER or TRUNCATE and DELETE can be replaced by calls to REPLACE which target the related data, and there even more ways for bypasses (like using CALL for stored procedures that call DELETE or DROP) or using code in comments /*!50000 DROP DATABASE*/ and many others....

The sql-parser used by PMA:
https://github.com/phpmyadmin/sql-parser

can be used to enforce it, but still require more testing as bypasses are probably possible, see for example the fixed issue: #12531

And even then I am not sure there is a reason to add this enforcement as the import.php work like "give me SQL file and it will be executed without validation at all"
So maybe to add 'warning message' that file uploaded to import.php can contain anything (any SQL command, REVOKE/DROP/DELETE, and it's better to review it before executing, which can be added in PMA as well in 2 steps operation)

also DROP and DELETE are not evil (depend on usage), the export.php file can generate DROP command as well when selecting options: (see in: https://demo.phpmyadmin.net/master-config/index.php?route=/server/export after selecting Custom - display all possible options)
Add DROP DATABASE IF EXISTS statement
Add DROP TABLE / VIEW / PROCEDURE / FUNCTION / EVENT / TRIGGER statement

[Abdallah-Fouad-X]

@emanuelb yes blacklist not the solution , but if you want you can alert the user by doping the tables