Could there be any security issues with this code ?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
guischarf
Forum Newbie
Posts: 1
Joined: Thu Sep 04, 2008 3:23 pm

Could there be any security issues with this code ?

Post by guischarf »

Hello,

Could this --using dynamic variables to fill up a form-- have security issues ? This is an update form. A user logins to his account and gets back his data in "<form>", where he can update it.

I am checking all the data before it goes back to the BD.

Code: Select all

<?php
$qry = "SELECT name, lastname [, ..., ...] FROM table WHERE id = 'someid' ";
$res = mysql_query($qry);
 
while ($row = mysql_fetch_assoc($res)):
    $records[] = $row
endwhile;
 
foreach ($records as $record):
   foreach($record as $key => $value)
      $thisvar = $key;
      $$thisvar = $value;
   endforeach;
endforeach;
?>
 
<form>
   <input name="name" value="<?php echo $name ?>">
   <input name="lastname" value="<?php echo $lastname?>">
   [...]
</form>
Tnx.
User avatar
andyhoneycutt
Forum Contributor
Posts: 468
Joined: Wed Aug 27, 2008 10:02 am
Location: Idaho Falls

Re: Could there be any security issues with this code ?

Post by andyhoneycutt »

I would say that potentially any piece of code brings with it some possible security risk. As long as you are type-checking the values that the user submits and properly escaping your data input, you should be about as safe as anyone else out there. sprintf is a great function for forcing values to adhere to their types, and any time you plan on inserting data that comes from any user at any point on the path to database entry it is always good practice to escape the data before inputting it.

That said, I see no glaring errors in the piece of code you provide as long as the "someid" variable you are using to run your select statement is properly escaped and cleaned.

-Andy
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Could there be any security issues with this code ?

Post by Mordred »

andyhoneycutt wrote: and any time you plan on inserting data that comes from any user at any point on the path to database entry it is always good practice to escape the data before inputting it.
No. No conditionals. Escape always all data.

http://www.logris.org/security/escaping ... ty-measure

guischarf, there are possible XSS issues, and related problems with register_globals=on. Also, how secure is a single piece of code may change with the context - who uses this, in what way, etc. Especially seeing that this is not your actual code, but an abridged version of it, we can't really say anything of the piece itself. Maybe this is safe, but the code updating the user isn't. We can't tell.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Could there be any security issues with this code ?

Post by Christopher »

I agree, both $name and $lastname are potential sources of attacks it the code above.
(#10850)
allicient
Forum Newbie
Posts: 9
Joined: Fri Sep 19, 2008 7:11 pm

Re: Could there be any security issues with this code ?

Post by allicient »

Mordred's comment about context is especially relevant here: if 'someid' is under control of the client, and is not explicitly bound to an active session then you've just allowed anyone and their dog to enumerate your entire database table (not a great move). But I'm guessing 'someid' is protected and tied to the user's valid session, right?

You've probably not implemented things like what you've shown (with the php tags in the form), but an additional suggestion: always seperate your code from your data... i.e. mixing lots of <?php blah blah ?> into the middle of an HTML page is going to make it practically unreadably and prone to errors. My advice (and I know there's a lot of people who disagree on this) is to have the HTTP GET or POST request to whatever.php and then have the php code output HTML through echo or whatever (you can load big chunks of HTML from a file). Keeps it simple. I've found Smarty (templating system) to be very useful in the past for splitting code and data (plus you can do some really nifty stuff with it), although I've not had the time to assess the security of the Smarty libraries to date.
Post Reply