SEO

Hello,

I have a site with 7 pages, on each of which users answer 4 questions.
The answers are submitted using AJAX and the php code below into a
MySQL database.

There is a possibility that the site may get mentioned on the radio
which could result in a high number of users accessing the site in a
short period of time.

Are there are any precautions I need to take re the adding of the data
to the database? Is the code below adequate?

Thanks

Geoff

@require(dirname(__FILE__) . '/../../../config/config.php');

$result1 = $_GET['answer1'];
$result2 = $_GET['answer2'];
$result3 = $_GET['answer3'];
$result4 = $_GET['answer4'];

mysql_connect($conf['sql']['host'], $conf['sql']['user'],
$conf['sql']['pass']) or die(mysql_error());
mysql_select_db($conf['sql']['db']) or die(mysql_error());

mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
VALUES ('$result1','$result2','$result3','$result4')");

Geoff Cox wrote:
> Hello,
>
> I have a site with 7 pages, on each of which users answer 4 questions.
> The answers are submitted using AJAX and the php code below into a
> MySQL database.
>
> There is a possibility that the site may get mentioned on the radio
> which could result in a high number of users accessing the site in a
> short period of time.
>
> Are there are any precautions I need to take re the adding of the data
> to the database? Is the code below adequate?
>
> Thanks
>
> Geoff
>
> @require(dirname(__FILE__) . '/../../../config/config.php');
>
> $result1 = $_GET['answer1'];
> $result2 = $_GET['answer2'];
> $result3 = $_GET['answer3'];
> $result4 = $_GET['answer4'];
>
> mysql_connect($conf['sql']['host'], $conf['sql']['user'],
> $conf['sql']['pass']) or die(mysql_error());
> mysql_select_db($conf['sql']['db']) or die(mysql_error());
>
> mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
> VALUES ('$result1','$result2','$result3','$result4')");
>

Impossible to tell from what you have. But it looks like you have done
absolutely no validation of the input data. The result can be a
complete destruction of your database - or worse. Google for "SQL
injection.

--
==================
Remove the "x" from my email address
Jerry Stuckle
JDS Computer Training Corp.
jstucklex@attglobal.net
==================

On Sun, 04 May 2008 21:31:03 -0400, Jerry Stuckle
<jstucklex@attglobal.net> wrote:

>> @require(dirname(__FILE__) . '/../../../config/config.php');
>>
>> $result1 = $_GET['answer1'];
>> $result2 = $_GET['answer2'];
>> $result3 = $_GET['answer3'];
>> $result4 = $_GET['answer4'];
>>
>> mysql_connect($conf['sql']['host'], $conf['sql']['user'],
>> $conf['sql']['pass']) or die(mysql_error());
>> mysql_select_db($conf['sql']['db']) or die(mysql_error());
>>
>> mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
>> VALUES ('$result1','$result2','$result3','$result4')");
>>
>
>Impossible to tell from what you have. But it looks like you have done
>absolutely no validation of the input data. The result can be a
>complete destruction of your database - or worse. Google for "SQL
>injection.

Jerry,

I have perhaps over simplified above - in fact only in one case is the
user asked to type in data - in the other cases it's a matter of
clicking on one of two images to give a response.

The box one does check for a number between 0 and 8

var number_check = number_given;
if ( (number_check >7) || (number_check < 1) ) {
alert("Tthe number must be in the range 1 to 7!");
} else
if (isNaN(number_check)) {
alert("Please enter a valid number");
} else {
sendGroup1Lab1(number_check);
}

is any further validation needed?

Cheers

Geoff

On Sun, 04 May 2008 21:31:03 -0400, Jerry Stuckle
<jstucklex@attglobal.net> wrote:

>Impossible to tell from what you have. But it looks like you have done
>absolutely no validation of the input data. The result can be a
>complete destruction of your database - or worse. Google for "SQL
>injection.

Jerry,

where the user is typing a number into a box

var number_check = number_given;
if ( (number_check >7) || (number_check < 1) ) {
alert("The number must be in the range 1 to 7!");
} else
if (isNaN(number_check)) {
alert("Please enter a valid number");
} else {
sendGroup1Lab1(number_check);
}


I have changed (***) the php to

$result1 = $_GET['answer1'];
$result2 = $_GET['answer2'];
$result3 = $_GET['answer3'];
$result4 = $_GET['answer4'];

mysql_connect($conf['sql']['host'], $conf['sql']['user'],
$conf['sql']['pass']) or die(mysql_error());
mysql_select_db($conf['sql']['db']) or die(mysql_error());

$result4 = mysql_real_escape_string($_GET['favorite']); ***

mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
VALUES ('$result1','$result2','$result3','$result4')");

I see that mysql_real_escape_string can only be used after connecting
to the database?

Is the above safer?

Cheers

Geoff

The wise Geoff Cox enlightened me with:
>
> I have perhaps over simplified above - in fact only in one case is the
> user asked to type in data - in the other cases it's a matter of
> clicking on one of two images to give a response.

And what if the user crafts his own http response? You don't check the
data he is giving you, so you might be in trouble. sprintf's and/or
mysql_escape_string is your friend.

Mark
--
Terantula - Industrial Strength Open Source - http://www.terantula.com/
Projects and administration - +31 6 5140 5160

On Mon, 05 May 2008 01:38:23 +0200, Geoff Cox <gcox@freeuk.notcom> wrote:
> mysql_connect($conf['sql']['host'], $conf['sql']['user'],
> $conf['sql']['pass']) or die(mysql_error());
> mysql_select_db($conf['sql']['db']) or die(mysql_error());

At least provide a more elegant solution then to 'die' with the database
message. Something like: include('sorrythesiteistobusyreturnlater.html');
exit; would be better.

> mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
> VALUES ('$result1','$result2','$result3','$result4')");

Alterations based on a GET? Unless for logging purposes, I'd change that
to a POST.
--
Rik Wasmus

On Mon, 5 May 2008 09:14:40 +0200, Mark Huizer
<xaa+news_comp.databases.mysql@dohd.org> wrote:

>The wise Geoff Cox enlightened me with:
>>
>> I have perhaps over simplified above - in fact only in one case is the
>> user asked to type in data - in the other cases it's a matter of
>> clicking on one of two images to give a response.
>
>And what if the user crafts his own http response? You don't check the
>data he is giving you, so you might be in trouble. sprintf's and/or
>mysql_escape_string is your friend.
>
>Mark


Mark,

I have now added mysql_escape_string to all the php files!

Cheers

Geoff

On Mon, 05 May 2008 09:43:14 +0200, "Rik Wasmus"
<luiheidsgoeroe@hotmail.com> wrote:

>On Mon, 05 May 2008 01:38:23 +0200, Geoff Cox <gcox@freeuk.notcom> wrote:
>> mysql_connect($conf['sql']['host'], $conf['sql']['user'],
>> $conf['sql']['pass']) or die(mysql_error());
>> mysql_select_db($conf['sql']['db']) or die(mysql_error());
>
>At least provide a more elegant solution then to 'die' with the database
>message. Something like: include('sorrythesiteistobusyreturnlater.html');
>exit; would be better.
>
>> mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
>> VALUES ('$result1','$result2','$result3','$result4')");
>
>Alterations based on a GET? Unless for logging purposes, I'd change that
>to a POST.

Thanks Rik for both suggestions.

Cheers

geoff

On Mon, 05 May 2008 07:16:35 +0100, Geoff Cox wrote:

> [...]
> where the user is typing a number into a box
>
> var number_check = number_given;
> if ( (number_check >7) || (number_check < 1) ) { alert("The number must
> be in the range 1 to 7!"); } else
> if (isNaN(number_check)) {
> alert("Please enter a valid number"); } else {
> sendGroup1Lab1(number_check);
> }

You must _never_ rely on client-side validation! As far as the
server-side script is concerned you should at least assume the user
switched JavaScript off (not to mention intended malformed values).

> I have changed (***) the php to
>
> $result1 = $_GET['answer1'];
> $result2 = $_GET['answer2'];
> $result3 = $_GET['answer3'];
> $result4 = $_GET['answer4'];

If those values are supposed to be numbers you should at least
ensure that:

$result1 = $_GET['answer1'] * 1;
$result2 = $_GET['answer2'] * 1;
$result3 = $_GET['answer3'] * 1;
$result4 = $_GET['answer4'] * 1;

In case not all digits are allowed (as your JavaScript fragment
suggests) a RegEx based test seems appropriate:

$hits = array();
$_GET['answer1'] = (isset($_GET['answer1'])
&& preg_match('|([1-7]+)|', $_GET['answer1'], $hits))
? $hits[1] * 1
: 0; // map invalid values to zero
$_GET['answer2'] = (isset($_GET['answer2'])
&& preg_match('|([1-7]+)|', $_GET['answer2'], $hits))
? $hits[1] * 1
: 0; // map invalid values to zero
[...]

(There's no need for those "$result" variables: Why keep the same value
in memory multiple times?) Do _not_ assume all the expected CGI
arguments are there actually but always check that.

> [...]
> $result4 = mysql_real_escape_string($_GET['favorite']); ***

What's "favorite" supposed to be? A string? A number (real or integer)?
How do you validate that?

> mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
> VALUES ('$result1','$result2','$result3','$result4')");

The single quote are needed for string values, while those "$resultX"
variables are integers. Hence you could omit the single quotes.

> I see that mysql_real_escape_string can only be used after connecting
> to the database?

Yes. However, id you'd validate the user provided values there's not
much need for such a call anyway. And escaping possibly malicious
values might avoid some SQL injection problem but it does _not_ avoid
your tables being filled with useless data.

> Is the above safer?

Further improvement could be gained by not using the tables directly
but calling an stored procedure which could implement additional
validation. But that's another topic and probably oversized for
your application.

However, you should switch from HTTP/GET to HTTP/POST. While that's
not a safety net as such at least it makes it a _little_ harder to
fake the CGI arguments.

--
Matthias
/"\
\ / ASCII RIBBON CAMPAIGN - AGAINST HTML MAIL
X - AGAINST M$ ATTACHMENTS
/ \

On Mon, 05 May 2008 10:25:06 +0200, Matthias Watermann <lists@mwat.de>
wrote:

>On Mon, 05 May 2008 07:16:35 +0100, Geoff Cox wrote:
>
>> [...]
>> where the user is typing a number into a box
>>
>> var number_check = number_given;
>> if ( (number_check >7) || (number_check < 1) ) { alert("The number must
>> be in the range 1 to 7!"); } else
>> if (isNaN(number_check)) {
>> alert("Please enter a valid number"); } else {
>> sendGroup1Lab1(number_check);
>> }
>
>You must _never_ rely on client-side validation! As far as the
>server-side script is concerned you should at least assume the user
>switched JavaScript off (not to mention intended malformed values).
>
>> I have changed (***) the php to
>>
>> $result1 = $_GET['answer1'];
>> $result2 = $_GET['answer2'];
>> $result3 = $_GET['answer3'];
>> $result4 = $_GET['answer4'];
>
>If those values are supposed to be numbers you should at least
>ensure that:
>
>$result1 = $_GET['answer1'] * 1;
>$result2 = $_GET['answer2'] * 1;
>$result3 = $_GET['answer3'] * 1;
>$result4 = $_GET['answer4'] * 1;
>
>In case not all digits are allowed (as your JavaScript fragment
>suggests) a RegEx based test seems appropriate:
>
>$hits = array();
>$_GET['answer1'] = (isset($_GET['answer1'])
> && preg_match('|([1-7]+)|', $_GET['answer1'], $hits))
> ? $hits[1] * 1
> : 0; // map invalid values to zero
>$_GET['answer2'] = (isset($_GET['answer2'])
> && preg_match('|([1-7]+)|', $_GET['answer2'], $hits))
> ? $hits[1] * 1
> : 0; // map invalid values to zero
>[...]
>
>(There's no need for those "$result" variables: Why keep the same value
>in memory multiple times?) Do _not_ assume all the expected CGI
>arguments are there actually but always check that.
>
>> [...]
>> $result4 = mysql_real_escape_string($_GET['favorite']); ***
>
>What's "favorite" supposed to be? A string? A number (real or integer)?
>How do you validate that?
>
>> mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
>> VALUES ('$result1','$result2','$result3','$result4')");
>
>The single quote are needed for string values, while those "$resultX"
>variables are integers. Hence you could omit the single quotes.
>
>> I see that mysql_real_escape_string can only be used after connecting
>> to the database?
>
>Yes. However, id you'd validate the user provided values there's not
>much need for such a call anyway. And escaping possibly malicious
>values might avoid some SQL injection problem but it does _not_ avoid
>your tables being filled with useless data.
>
>> Is the above safer?
>
>Further improvement could be gained by not using the tables directly
>but calling an stored procedure which could implement additional
>validation. But that's another topic and probably oversized for
>your application.
>
>However, you should switch from HTTP/GET to HTTP/POST. While that's
>not a safety net as such at least it makes it a _little_ harder to
>fake the CGI arguments.

Many thanks Matthias - quite a bit to get my head round but will start
trying now!

Cheers

Geoff