Page 1 of 2
Some of my early PHP
Posted: Sat Aug 26, 2006 7:31 pm
by Ollie Saunders
Just logged in the FTP on a really old PHP site I made and for a laugh I thought I'd see how bad the PHP was.
The first 15 lines or so were surprisingly good and then I saw this:
Code: Select all
extract($_POST);
foreach ($_POST as $extractedpost => $tostrip) { // strip_tags and trim on all of $_POST
$$extractedpost = addslashes(trim(strip_tags($tostrip,'<p><ul><li>')));
}
Eeek!
In fact extract appear 6 times! Once on $_POST (as above) a couple of times on $row, once on $_FILES and once on $_GET.
At the time I wrote this I knew absolutely nothing about security and it shows.
Then of course I thought, why, as an inexperienced PHP developer, should one suspect addslashes() or extract() as being dangerous. They are there aren't they? Why not use them! I am now of the mind that extract() should be removed or at least trigger a notice and addslashes() should have a big fat notice in the manual saying where you can and cannot use it. addslashes() is as bad as magic quotes really.
Interestingly the main reason I used those appauling functions is because of "PHP and MySQL for Dummies" which uses them frequently in all the examples. That book is truely terrible.
Posted: Sat Aug 26, 2006 7:51 pm
by RobertGonzalez
That last time I did that I left the room in embarassment. I have since cleaned a lot of client sites I did early on, but hooo baby, it was some bad stuff.
Posted: Sat Aug 26, 2006 8:15 pm
by Ollie Saunders
Everah wrote:That last time I did that I left the room in embarassment. I have since cleaned a lot of client sites I did early on, but hooo baby, it was some bad stuff.
Haha! Well I'm confident of my abilities now so I feel no embarassment.
Posted: Sat Aug 26, 2006 9:15 pm
by jabbaonthedais
I have learned a lot in the past year... I recently had to go back and edit some of my first php scripts and it was a funny sight.

Posted: Sun Aug 27, 2006 12:37 am
by matt1019
@ole/Everah:
What is the potential security risk posed by extract()? and in what ways can it do you harm?
-Matt
Posted: Sun Aug 27, 2006 12:54 am
by timvw
The potential risk that i immediately see (so if you think about it more than 5 minutes you'll probably find more

) is the following:
Code: Select all
<?php
$permissionLevel = 0;
// get posted username and password
extract($_POST);
if (new Credential($username, $password).IsValid()) {
$permissionLevel = 500;
}
?>
Since the user can submit an extra 'permissionLevel' with value 1000.. After the extract the permissionLevel will be 1000, no matter what...
Posted: Sun Aug 27, 2006 1:04 am
by matt1019
timvw wrote:The potential risk that i immediately see (so if you think about it more than 5 minutes you'll probably find more

) is the following:
Code: Select all
<?php
$permissionLevel = 0;
// get posted username and password
extract($_POST);
if (new Credential($username, $password).IsValid()) {
$permissionLevel = 500;
}
?>
Since the user can submit an extra 'permissionLevel' with value 1000.. After the extract the permissionLevel will be 1000, no matter what...
?? huh? (or could be because I am very very sleeepy!)
I am lost! I am soo bad at security spottings & when to use the functions ONLY WHERE THEY ARE NEEDED.
I need to hang around php-security section more often.
Explain further.... the Concept.
-Matt
Posted: Sun Aug 27, 2006 1:34 am
by timvw
the code below is misleading:
Code: Select all
// get posted username and password
extract($_POST);
If the user had posted more than only username and password (eg: permissionLevel = 1000) then the extract would result in the following:
Code: Select all
$username = xxx;
$password = yyy;
$permissionLevel = 1000; // <---- at best it's a feature :p
Posted: Sun Aug 27, 2006 1:48 am
by William
That is actually really funny, because I am working on a project right now, and I was going through the code and found extract($_POST);. Well I was pretty sure I knew exactly what it was, but just in case I went to the PHP website, and I thought to my self... What a huge security risk. Well my friend was right here with me and ask me what kind of security risk, and like what was said above, you can easilly send post arguments to the file and create or even change old variables.
Posted: Sun Aug 27, 2006 3:20 am
by RobertGonzalez
A lot of code from the early stages of a new developers projects seems nasty. Unless that developer comes from a programming background. But in many cases people happen across PHP and begin to use it because it is fairly easy to get a hold of, conceptually. The problem is that most people use PHP to achieve a very specific purpose, something that they cannot do with javascript, HTML or other technologies. And they can get it to work the way the want it to work not understanding that there is probably a better, more efficient, more secure way of doing it.
I know I am guilty of that. My early code was all about getting done what I wanted done. As long as the syntax did not error, I was fat and happy. But as a developer begins to take development more seriously we begin to look for ways to make our work better. Then we can go back and look on what we have done and see that it was absolutely bitter-beer-face nasty.
Posted: Sun Aug 27, 2006 4:33 am
by onion2k
For the last month or so I've been revisiting a site I wrote about 4 and a half years ago. The code is suprisingly good .. the HTML is a table nightmare .. but the really bad thing is the SQL. In several places I've rewritten an SQL statement to make the code about 100 times quicker.

Posted: Sun Aug 27, 2006 4:54 am
by matthijs
I won't say my first scripts were bullet-proof (will they ever?) but I'm happy I've always been very, very careful and critical, even for the most basic scripts. Still a lot to learn!
But it is amazing how you read posts on forums which go like "Hi, I started with php this week so sorry if I make funny mistake but I made a portal site with forum and cms-thing, but my chat-script does not work. Can you help please? Below is my code."
And that code is put on a live server out there ....
makes me think. Isn't it very boring to be a scriptkiddie/hacker these days? Were is the challenge?
Posted: Sun Aug 27, 2006 5:15 am
by Ollie Saunders
Isn't it very boring to be a scriptkiddie/hacker these days? Were is the challenge?
Its very boring and sad to be a script kiddie anyway. But yeah you are right there isn't enough security awareness.
There are two ways of looking at that however.
- Most people know nothing about security but I am an exception. This makes me better than them. yay!

- Most people know nothing about security and that will damage faith in the industry that I work in.

As long as you promote yourself as being security aware there isn't too much of a problem.
but the really bad thing is the SQL. In several places I've rewritten an SQL statement to make the code about 100 times quicker.
Yeah that's just experience. In loads of places I wrote my own functions and then discovered PHP came with its own better versions. That was annoying.
Posted: Sun Aug 27, 2006 5:40 am
by timvw
The first time i proudly released a portal application i got an anonymous (meanwhile i know who informed about it

) comment on my blog that there was a serious security hole.. So i'll share another vulnerability that occurs quite often
http://en.wikipedia.org/wiki/Cross_site_scripting
Since php defaults to sending text/html as content-type i think it should also apply htmlentitities to all output, unless for parts where it was explicitely told otherwise.
Posted: Sun Aug 27, 2006 9:31 am
by Weirdan
meanwhile i know who informed about it
IIRC, that was me
