Can you help me secure this

[Miks]

I'm new to all this so was wondering if someone could teach/help me to secure this?

Heres what I have got so far

 

include "globals.php";

print "
<h2>Upload Cover Picture</h2>
You can personalize your profile page by adding a cover picture<BR><BR>
Current url:<BR>
{$ir['cover_pic']}<BR>
<img src='{$ir['cover_pic']}' width='840' height='300' alt='' title='' style='float: center'>
";

if(isset($_POST['cp_update']))
if(!preg_match('~(.?).(jpg|jpeg|gif|png)~i', $_POST['cp_update'])) 
{
print "You are trying to upload an invalid image";
  } 

else{
$db->query("UPDATE users SET cover_pic='{$_POST['cp_update']}' WHERE userid=$userid");
$ir['cover_pic']=stripslashes($_POST['cp_update']);
print "<br><b>Your Cover Picture Has Been Updated!</b>";
}
print "
<form method='post' action='coverupload.php'>
url:<input type='text' name='cp_update' value='http://'>
<input type='submit' value='Submit'>
</form><BR>
You can set a cover picture by posting the url to the image itself in the above box, make sure you include http://.<BR>If you want a free website to upload your image to then we recommend ImgShare.co.uk.

";

$h->endpage();

[Truefalse]

Yes.

 

• stripslashes() provides near zero data sanitisation thus little security.
  
• Your lazy match (.?) in your preg_match() is very worrying (also, you're missing the anchor tags (^ and $)
  

 

Images have EXIF data - which is pretty much meta data about the image, detailing; the image name, MIME types, dimensions and everything else (such as GPS co-ordinates when taken - if the device logs this data). To secure your script, you're going to want to do the following;

 

• Grab the image they have pasted with code
  
• Inspect that image with various functions
  

Various functions to check something is a valid image are;

 

• getimagesize()
  
• exif_read_data()
  

 

Now that you've checked the integrity of the thing the user is uploading, we need to check the file name (to disallow XSS, SQL, and CSRF injections).

You've made a start with the preg_match() - which is decent, however your regular expression will not detect any of the threats listed above.

Here is a ruleset for file naming: http://www.dpbestflow.org/file-management/file-naming

 

A basic regular expression you could use is the following:

This will allow only;

• Alphabetic characters (a-zA-Z)
  
• Numeric characters
  
• Special characters (_, -, %, /)
  

 

^([a-zA-Z0-9_\-%]+\.)(jpe?g|gif|png)$

 

Ofcourse this will only validate the filename, and not the entire URI.

https://regex101.com/r/pD5dB5/2

Edited March 22, 2015 by Truefalse

[Truefalse]

[...]

3. Next I would use mysqli's real_escape_string to prevent a user from trying to insert some unwanted SQL into your database. Your code will then become:

 $cover_pic = trim($_POST['cp_update']);
$cover_pic = $db->escape_string($cover_pic);

 

For me personally that is enough, although people will debate about it.

[...]

Absolutely not. Do not preach "for me that is enough". It is nowhere near substantial.

Sure, it helps against SQL injection - but binding query parameters does that a lot better. If you kept with your version that is "enough for you", you are not securing yourself against stored XSS attacks - which is a huge thing. With XSS, you can do whatever you like;

 

• Redirect users
  
• Modify the DOM
  
• Steal users cookies
  
• ...
  

[Miks]

Ok this is what I have now

 

if(isset($_POST['cp_update']))
if(!preg_match('~(.?).(jpg|jpeg|gif|png)~i', $_POST['cp_update']))
{
print "You are trying to upload an invalid image";
  }

else{
$ir['cover_pic']=stripslashes($_POST['cp_update']);
$cover_pic = trim($_POST['cp_update']);
$db->query("UPDATE users SET cover_pic='$cover_pic' WHERE userid=$userid");
print "<b>Your Cover Picture Has Been Updated!</b>";
}
print "
<form method='post' action=$_SERVER[php_SELF]>
url:<input type='text' name='cp_update' value='http://'>
<input type='submit' value='Submit'>
</form>

 

Using the below stopped the feature from working, not sure why

 

$cover_pic = $db->escape_string($cover_pic);

 

After doing some Google searching I read that it was best to change the form action to

<form method='post' action=$_SERVER[php_SELF]>

[Truefalse]

Ok this is what I have now

 

if(isset($_POST['cp_update']))
if(!preg_match('~(.?).(jpg|jpeg|gif|png)~i', $_POST['cp_update']))
{
print "You are trying to upload an invalid image";
  }

else{
$ir['cover_pic']=stripslashes($_POST['cp_update']);
$cover_pic = trim($_POST['cp_update']);
$db->query("UPDATE users SET cover_pic='$cover_pic' WHERE userid=$userid");
print "<b>Your Cover Picture Has Been Updated!</b>";
}
print "
<form method='post' action=$_SERVER[php_SELF]>
url:<input type='text' name='cp_update' value='http://'>
<input type='submit' value='Submit'>
</form>

 

Using the below stopped the feature from working, not sure why

 

$cover_pic = $db->escape_string($cover_pic);

 

After doing some Google searching I read that it was best to change the form action to

<form method='post' action=$_SERVER[php_SELF]>

Still has security holes. See my first reply.

[Miks]

Ok I'll have another read, we dont have EXIF installed on the server, if its important then I'll put a request in to have it added

I'll read up on preg_match as I'm still learning and dont know exactly what to do from your post however it is helpful but I will need to do a bit of searching to understand it

[Miks]

When I update the preg_match to

if(!preg_match('^([a-z0-9_\-%\/\/.]+\.)(jpe?g|gif|png)$', $_POST['cp_update']))

and insert a image url like: http://covers4timelines.com/uploads/covers/7f99bef877271bf7dd4aee74c0629e32.jpg

It keeps on saying You are trying to upload an invalid image

[KyleMassacre]

I really don't see anything wrong with what [MENTION=50378]Guest[/MENTION] gave you

http://mrmarlow.co.uk/image_check.txt

that provides more than enough to get you by in this

[Miks]

I have copied and pasted Guests code and updated any reference to dp_update to cp_update. When you insert an image url it gives you the success message but doesnt update the DB

I also dont see any reference to it updating the database apart from my commented out section, although I'm sure I'm wrong

 

<?php
include "globals.php";

$image = isset($_POST['cp_update']) && is_string($_POST['cp_update']) && filter_var($_POST['cp_update'], FILTER_VALIDATE_URL) ? $_POST['cp_update'] : FALSE ;
if ($image) {
$ext = substr($image, strripos($image, '.') + 1);
switch($ext) {
	case "png":		$file = imagecreatefrompng($image);		break;
	case "jpg":		$file = imagecreatefromjpeg($image);	break;
	case "gif":		$file = imagecreatefromgif($image);		break;
	default:		$error = 'No valid image entered';		break;
}

if (isset($error)) {
	echo $error;
} else {
	$size = getimagesize($image);
	if (is_array($size)) {
		$mime = array('image/jpg', 'image/jpeg', 'image/gif', 'image/png');
		if (in_array($size['mime'], $mime)) {
			if ($size[0] && $size[1]) {
				//$db->query("UPDATE users SET cover_pic='{$_POST['cp_update']}' WHERE userid=$userid");
				//$ir['cover_pic']=stripslashes($_POST['cp_update']);
				echo '<b>Your Avatar Has Been Updated!</b>';
			}
			else {
				echo '<b>Height or Width could not be detected</b>';
			}
		}
		else {
			echo '<b>No mime type returned.</b>';
		}
	} else {
		echo '<b>Did not pass getimagesize() so not a valid resource.</b>';
	}
}
}
echo '<form action="coverupload.php" method="post">
Image Link : <input type="text" name="cp_update" value="http://"> <button>Test</button>
</form>';
$h->endpage();
?>

[Veramys]

That's because Guest commented out the update stuff. Also the problem with your $db->escape_string is because no function like so exists in mccodes. It is actually $db->escape

[Miks]

Ok so I was right about the comment bit, I thought he commented it out as it was not needed but then couldn't understand how it worked!

And thank you for pointing out the DB escape issue as well, thank you!

[lucky3809]

You ALWAYS want to check the size of the picture, because someone can do some nasty stuff with the size, everything truefalse has stated is good information to read up on. Security is not just copy and pasting, please do not do that, make sure you are fully going over your code and also testing it yourself. Just because you use MRES or stripslashes or anything a long those lines, does not mean it's fully secured, checking the file completely is what makes it secure.

[Miks]

Hey [MENTION=65073]lucky3809[/MENTION] thanks for your advice, at the moment I am learning about security and will continue to post my work as I create new features until I get it right. I appreciate everyone's feedback and ask that you continue to give feedback on my future posts as its helping me learn quickly!

I dont want to copy and paste when I can help it as I do like to learn, I have a "fairly" good understanding and things are starting to click into place and become a little clearer. I'm still confused about a few things but will post them if I cant work them out for myself.

[SRB]

Ok so I was right about the comment bit, I thought he commented it out as it was not needed but then couldn't understand how it worked!

And thank you for pointing out the DB escape issue as well, thank you!

The line was commented out as I don't have MC Codes on my server, so don't have any of the tables etc.

That would have just gave me an error.

That said, if it wasn't needed - I would have just deleted the line of code completely.