Can this code be more secure/tighter?

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
bulgin
Forum Commoner
Posts: 29
Joined: Wed Feb 11, 2009 8:47 pm

Can this code be more secure/tighter?

Post by bulgin »

Okay, this is what works for me. I 'd appreciate anyone looking at it to tell me perhaps how it could be made tighter/more secure. somehow I feel the last part UPDATE could be made better. And what about the variables set for the php email send? could that be improved?

thanks.

Code: Select all

<?php
include 'config.php';
include 'opendb.php';
 
 
$mailer = mysql_query("SELECT customer_information.email, customer_inf
ormation.email_subject, customer_information.email_body FROM customer_informatio
n where sent_or_not_sent = '199'") or die (mysql_error());
 
while($user = @mysql_fetch_array($mailer)){
$to=$user[email];
$subject=$user[email_subject];
$body=$user[email_body];
mail($to,$subject,$body) ; 
 
mysql_query("UPDATE customer_information set sent_or_not_sent = 1 where sent_or_
not_sent = '199'") or die (mysql_error());
 
}
 
?>
 
 
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Can this code be more secure/tighter?

Post by Eran »

There are multiple problems here:

Code: Select all

$mailer = mysql_query("SELECT customer_information.email, customer_information.email_subject, customer_information.email_body FROM customer_information where sent_or_not_sent = '199'") or die (mysql_error());
 
while($user = @mysql_fetch_array($mailer)){
Don't use die() in production scripts, it's bad practice (I don't know they're so widely used in tutorials, if that's where you got the inspiration). Especially - don't print to the screen the mysql_error - you will be potentially exposing sensitive information. Instead, redirect to an error page and log the error on the server. If this script is never access directly than just log the error to the server.

Instead of killing the script with die(), use a conditional:

Code: Select all

if($mailer !== false) { //Check that the result is not false
   while($user = @mysql_fetch_array($mailer)){
     ....
   } 
} else {
 // redirect to the error page
}
 
Next, don't use the error suppression operator. If an error happens, you want to know about it. Remove the @ from the while loop:

Code: Select all

while($user = @mysql_fetch_array($mailer)){
After that, make sure to use quotes when accessing associate array indexes:

Code: Select all

$to=$user['email'];
$subject=$user['email_subject'];
$body=$user['email_body'];
PHP recognizes it without the quotes but issues an E_NOTICE error. If you aren't seeing those notices, you should set your error reporting to a higher level, they are important.
Also, if you are only using the associative indexes, use mysql_fetch_assoc() instead (mysql_fetch_array also fetches the numberical indexes).

The last thing is that make sure that those value that come from the database don't contain code that could be injected to your scripts. For example, multiple email address separated by commas might be stored in the email column. This should probably be handled in the script that inserts the data to the database.
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Can this code be more secure/tighter?

Post by josh »

Yes it can be more secure. Header injection. Always escape inputs
bulgin
Forum Commoner
Posts: 29
Joined: Wed Feb 11, 2009 8:47 pm

Re: Can this code be more secure/tighter?

Post by bulgin »

Okay guys I'm a bit of a newbie here so please be kind with me. I understand how to redirect and will implement -- thank you for that advice, pytrin. Now the error suppression operator... I'm a bit lost on that. Okay, looked that up. You mean the '@' symbol, right? Could you elaborate what I must do? I can easily insert quotes and thanks for pointing it out.

Header injection and escaping inputs -- please elaborate. I'm learning but could use some help along the way.

Also I was browsing the books at Barnes and Nobles today and read a little thing about Mysqli -- My SQL Improved. Should I be using that?

Thanks!
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Can this code be more secure/tighter?

Post by Eran »

Yes, @ is the error suppression operator. It's generally bad to use it, as I mentioned - you want to know when errors occur so you could deal with them. If this is on a production server, errors should be logged to a file which you can later go over and see what went wrong.

Josh was referring to escaping input - user input is not be trusted ever. Since your data comes from the database, I can't tell exactly what its source is. If the data is inserted to the database from outside input (for example, a form) then it should be filtered / validated before being entered to the database. This data could be used later to compromise your mail function for example, by injecting headers or multiple addresses.

Mysqli is an object-oriented interface to mostly the same mysql functions. It has some minor advantages but overall if you probably don't need it if you haven't encountered a situation you can't solve using the regular mysql functions.
bulgin
Forum Commoner
Posts: 29
Joined: Wed Feb 11, 2009 8:47 pm

Re: Can this code be more secure/tighter?

Post by bulgin »

thanks for your help.

The file is run solely via a cron job with no user input on forms or such, but it does do a lookup into a database where the users have input information previously by submitting a form -- that's how the data gets into the database. They input their email address, message_subject, message_body.

The email field is auto validated to be an email field and only accepts valid email addresses. The other fields are text where I can control the text length, i.e., limit the input to say, 200 characters, or so, so need I be concerned?
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Can this code be more secure/tighter?

Post by Eran »

That's not enough. The subject needs to be filtered as well to prevent possible injection attacks. Check out the following article - http://www.php-security.org/MOPB/MOPB-34-2007.html

This is why a lot of people use a mail package such as swift-mailer or Zend_Mail which take care of such concerns.
bulgin
Forum Commoner
Posts: 29
Joined: Wed Feb 11, 2009 8:47 pm

Re: Can this code be more secure/tighter?

Post by bulgin »

Interesting reading.

I am using PHP 5.2.6-2

According to that article I would be unaffected then.

Is Zendmail() a drop in replace ment for mail()?

I'm using postfix and not sendmail so would it make a difference. The article seems to imply that zendmail works with sendmail.
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Can this code be more secure/tighter?

Post by Eran »

Zend_Mail is a component of the Zend_Framework not a PHP function. Google it and you will find all the information you need.
bulgin
Forum Commoner
Posts: 29
Joined: Wed Feb 11, 2009 8:47 pm

Re: Can this code be more secure/tighter?

Post by bulgin »

Okay thanks for all your help.

I'm learning!

8)
Post Reply