FTP input vulnerability

[php_nub_qq]

Hi there. I was wondering if there was something like an "FTP Injection" just like SQL injection because I was looking at my code today and I noticed this

if(file_exists($_REQUEST['filename']))

Should I be concerned about using an unescaped string in this case?

[lemmin]

The user-input string isn't dangerous by itself. It's only dangerous when you implement it in some sort of command execution, like an SQL statement. If you are running a command-line FTP using shell, then it is definitely dangerous. Just by the code you have posted, there is no way to know whether or not there is a vulnerability involved in your implementation.

[Psycho]

Hmm . . .

 

file_exists() only returns True/False based upon whether the file exists or not. It doesn't execute any action against the file. But, it is probably a bad idea to allow a user to pass a file name/path to your script anyway as it gives them a way to find out information about your directory structure and file format. If these are files on the server then you should have the user select from a list of options where the file path/name are not exposed.

 

Also, if you are checking if a file exists then I would assume you will be performing some action on it. In that case using $_REQUEST['filename'] would be a big risk.

[php_nub_qq]

I'm using $_REQUEST because this current file is an ajax base file which runs every time an ajax call is executed, meaning it could be a POST or GET request, and it then calls the file in question "filename". The filename is not directly editable by the user but even though it is hardcoded in an external javascript file I still believe that there are people out there who can manage to change it which would of course trigger the 404 error file but I thought I should ask because in SQL as you most certainly know an injection might go far beneath what the actual code is meant for. As a conclusion I assume that in my case there is no danger what so ever and there is no need of further protection, yes?

Edited April 30, 2013 by php_nub_qq

[Psycho]

I'm using $_REQUEST because this current file is an ajax base file which runs every time an ajax call is executed, meaning it could be a POST or GET request, and it then calls the file in question "filename". The filename is not directly editable by the user but even though it is hardcoded in an external javascript file I still believe that there are people out there who can manage to change it which would of course trigger the 404 error file but I thought I should ask because in SQL as you most certainly know an injection might go far beneath what the actual code is meant for. As a conclusion I assume that in my case there is no danger what so ever and there is no need of further protection, yes?

 

Right, the problem is not specific to $_REQUEST - it is with any data sent from the user. You should not, in my opinion, have the file information accessible on the client. You don't need to have a hard-coded JavaScript with the file information. Just use a generic list of options on the client side and then translate those values to their actual value on the server.

 

E.g. the list of "files" on the client might look like this in the JS code

var files = new Array();
files[0] = 'fileA';
files[1] = 'fileB';
files[2] = 'file1C';
files[3] = 'fileD';

And when the user makes a selection, send the ID, not the value. Then on the server page take the ID and translate it into the actual file needed

$files = array(
    0 => 'path/filename.php',
    1 => 'anotherpath/anotherfile.php',
    2 => 'pathnumber2/differentfile.php',
    3 => 'path/image.jpg'
);
 
$fileID = intval($_REQUEST['fileID']);
 
if(!in_array($fileID, $files))
{
    //User didn't select a valid file
}
else
{
    //User selected valid file
}

Edited April 30, 2013 by Psycho

[php_nub_qq]

That is interesting, I have never thought of that ( how silly of me ). But you misunderstood, there is no selection of files, just ordinary ajax calls which all call the same file with an extra parameter "script" which refers to the script to be executed. I made this because there are some basic files I need in all scripts that are to be called dynamically and instead of having to include the same thing in all scripts I rather created one file and call scripts to it. I'm having my doubts if you can understand me because of my English but in case you did, I could put all scripts in an array and then call them by numbers but in that case the "hacker" or lets just call him "non-good wisher" can simply change the numbers and mess stuff up. Of course I have security measures in all scripts, but it still gives me that bad feeling that the person in question can modify stuff?

Edited April 30, 2013 by php_nub_qq

[DavidAM]

If a "difficult" user changes the ID that is being submitted, the worst thing that can happen is he gets no data back.

 

If, on the other hand, he changes the FILENAME to /etc/passwd, he can learn a whole lot about your system users.

 

Keep in mind: just because you use AJAX to get the file, and your Javascript processes the returned contents, does NOT mean that someone cannot get the contents of the file. If AJAX can request the file, I can request it directly in the browser, or with wget or curl or telnet. None of those is going to have your JS "protecting" the contents from prying eyes.

[oaass]

An attacker will love a function like this. It makes it very easy to map your application. It's like error-based SQL injection. The attacker only needs to do a few manual attempts to compare the responses between true and false. This information can then be used to write a tool that brute forces your application structure.

[php_nub_qq]

File numbers it is then thanks!

[php_nub_qq]

A little update.

In the process of converting my scripts to numbers I realized that it is completely unnecessary because even if the script name gets changed there is no way it can pass the conditions, see:

	if(in_array($_REQUEST['script'], $scripts) && file_exists('../Framework/scripts/'.$_REQUEST['script'].'.php')){

		\\ Code Code Code

		require('../Framework/scripts/'.$_REQUEST['script'].'.php');

		\\ Code Code Code

	}else{
	        throw new \Exception("File not found", 404);
	}

Am I right?

Edited May 1, 2013 by php_nub_qq

[Psycho]

A little update.

In the process of converting my scripts to numbers I realized that it is completely unnecessary because even if the script name gets changed there is no way it can pass the conditions, see:

	if(in_array($_REQUEST['script'], $scripts) && file_exists('../Framework/scripts/'.$_REQUEST['script'].'.php')){

		\\ Code Code Code

		require('../Framework/scripts/'.$_REQUEST['script'].'.php');

		\\ Code Code Code

	}else{
	        throw new \Exception("File not found", 404);
	}

Am I right?

 

No. You should not provide any information on the client-side regarding your system files. Not knowing what is in $scripts I can't say for sure, but the in_array() part of that condition would "probably" prevent a user from submitting a file that you are not expecting. But, the file_exists() would NOT! The reason is that the user could pass a file name in the format '../../inc/somefile.php' would would cnage the location that file_exists() is looking for the file.

 

If you already have an array of the valid files why not use the index (i.e. ID) when you are passing the value from the client to the server.

 

Bottom line is that using the actual file name to pass between the client and the server is a bad idea

[Psycho]

I'm having my doubts if you can understand me because of my English but in case you did, I could put all scripts in an array and then call them by numbers but in that case the "hacker" or lets just call him "non-good wisher" can simply change the numbers and mess stuff up. Of course I have security measures in all scripts, but it still gives me that bad feeling that the person in question can modify stuff?

 

I do understand you - I don't think you understand me. Right, even if you use numbers an attacker could change the numbers that are passed. But, you are guaranteed that if the number is one that does not exist in your list it can not be used to access other files or be used to interrogate your files. So, if the number is not in the list you simply ignore it or perform whatever error handling you want to. Now, if the file IS in the list and you need to somehow prevent the user from using some files and not others, using an ID does not necessarily provide a method to do that over using the file names. You would still need to implement that validation the same way. But, using an ID (correctly) will 100% guarantee that an attacker cannot pass various file names to use against your own scripts. So, rather than trying to add additional error handling to support file names, do it the right way and use an ID or some other non-descriptive value to identify which file to use.

[php_nub_qq]

I do understand you - I don't think you understand me. Right, even if you use numbers an attacker could change the numbers that are passed. But, you are guaranteed that if the number is one that does not exist in your list it can not be used to access other files or be used to interrogate your files. So, if the number is not in the list you simply ignore it or perform whatever error handling you want to. Now, if the file IS in the list and you need to somehow prevent the user from using some files and not others, using an ID does not necessarily provide a method to do that over using the file names. You would still need to implement that validation the same way. But, using an ID (correctly) will 100% guarantee that an attacker cannot pass various file names to use against your own scripts. So, rather than trying to add additional error handling to support file names, do it the right way and use an ID or some other non-descriptive value to identify which file to use.

 

I understand, I just don't see what is the difference between using a numerical ID or a string ID, either one of them will return errors if they don't exist in the array of allowed files. I'm not trying to argue I completely agree that IDs are a better choice, I just can't see whats that big of a difference :/