Help me sanitize the demon

PHP
1

Hi everyone. I have received an amazing amount help in creating a project site site for my family/friends to use, with the goal of learning as much as I can about PHP, and while I’m still a newbie, I’ve gained a lot of knowledge thanks to this amazing community.

I have one last (for now! ;)) big request… help me make sure I am sanitizing all of my code correctly… as I’m SURE I have some mistakes…

I’m going to be posting a LOT (aka bumping with new snippets) in this thread (any part of my page that allows user input), but I will try to trim the code down as much as I can to just work out the sanitizing stuff.

Here we go with the first piece…

1


// I have this code for checking if a GET is a legitimate user in the db.
// The page that this is used on will display the selected members homepage to anyone who requests it.
<?php
if (isset($_GET['user'])) {
  $user = mysqli_real_escape_string($link, $_GET['user']);
  $check = mysqli_query($link, "SELECT username FROM members WHERE username = '$user'");
	$result = mysqli_num_rows($check);
		if ($result != 1) {
			header('Location: /');
			exit();
		}
		else {
	$get_user = mysqli_real_escape_string($link, $_GET['user']); // hm, isn't this redundant now that I look at it?
		}
	}
?>
2

2


// in my user navigation I am doing this (it was one of the first php things I did so I'm pretty sure it is dumb):
<li><?php echo '<a href="movies.php?user=' . htmlspecialchars($username) . '">' ?><strong>My Movies</strong></a></li>
<li><?php echo '<a href="addmovie.php?user=' . htmlspecialchars($username) . '">' ?>Add Movie</a></li>
<li><?php echo '<a href="editmovie.php?user=' . htmlspecialchars($username) . '">' ?>Edit Movies</a></li>

// Though there is probably no point since they are set links and I should just add the following at the top of any pages that involve a GET?:
<?php
if (isset($_GET['user'])) {
	$_GET['user'] = htmlspecialchars($_GET['user']);
}
?>
3

maybe 1 should be changed to this, hm:


<?php
if (isset($_GET['user'])) {
  $user = mysqli_real_escape_string($link, $_GET['user']);
  $check = mysqli_query($link, "SELECT username FROM members WHERE username = '$user'");
	$result = mysqli_num_rows($check);
		if ($result != 1) {
			header('Location: /');
			exit();
		}
		else {
	$get_user = htmlspecialchars($_GET['user']);
		}
	}
?>
4

Try taking a look at the filter extension. also, whilst I don’t use them in the example, a look at [URL=“http://uk2.php.net/manual/en/pdo.prepare.php”]prepared statements might be beneficial too. :slight_smile:


<?php
function getSuppliedUser($key = 'name', $input = INPUT_GET){
  $user = null;
  if(filter_has_var($input, $key)){
    $user = filter_input($input, $key, FILTER_SANITIZE_STRING);
  }
  return $user;
}

function isValidUser($user){
  $result = mysqli_query($link, sprintf(
    "SELECT username FROM members WHERE username = '%s' LIMIT 1;",
    mysqli_real_escape_string($link, $user)
  ));
  return 1 === mysqli_num_rows($result);
}

function redirect($path = 'http://example.org/'){
  header(sprintf('Location: %s', $path));
  exit;
}

$user = getSuppliedUser();

if(null === $user){
  redirect();
}

if(false === isValidUser($user)){
  redirect();
}

/*
  Consider this user valid
*/
5

Please excuse my ignorance (I haven’t dealt with that type of code a lot so it takes a little deciphering for me… hehe), but is that ‘nearly’ the same as what I am doing except for the following comment:


<?php
function getSuppliedUser($key = 'name', $input = INPUT_GET){
  $user = null;
  if(filter_has_var($input, $key)){
    $user = filter_input($input, $key, FILTER_SANITIZE_STRING); // use this filter instead of mysqli_real_escape_string($link, $_GET['user']); ?
?>

side note, couldn’t I change my last line to:

$get_user = $user;

and it would still be sanitized from the first time it was dealt with in the code?

6

hope that didn’t come off as rude or anything, just trying to understand. :slight_smile:

7

Hey! Rude? Not at all. :wink:

…but is that ‘nearly’ the same as what I am doing

Not quite. You’re not filtering, or sanitising the incoming string; you’re simply making it safe to be used in a SQL query.

The code I posted is similar, because the flow is the same - to mimic what you originally had.

In my example, the string is sanitised then only made safe to use in an SQL query when we need it to be; and even then, we don’t touch the original value assigned to $user.

Barring, the use of the filter extension, and moving a few operations to dedicated functions it should operate the same way. :slight_smile:

8

Ok thanks Anthony, you rock as usual. Appreciate the further info. I think I get it somewhat. Will play with implementing it in my code style and see what happens. Thanks!

Will make a post in a bit with a more generalized set of questions re the other sanitization questions that I have, instead of posting a ton of big code blocks, to make it easier/quicker to get help. :slight_smile:

Will post that soon…

9

I guess a lot of my questions are pretty simple. Here’s a couple…

Question 1:
I have forms on my site that allow users to add/edit things in the db, and I’m just wondering, am I supposed to use some sort of hidden form field on any form to prevent ‘xss’? Because that’s the only thing I’m not doing, I think anyway. I’m comfortable with how I am sanitizing the data though.

Question 2:
I built my site with md5, but I am reading that I should use a better system such as sha1 (or something else?).

If so, is changing my site to a different password system as easy as the following, or do I need to make other changes (increase char length for the password entry in the db etc)?

swap all of my

md5('salt' . 'password' . 'salt');

for

sha1('salt' . 'password' . 'salt');