PHP Bug In View Article 1

[d3sol4t3]

Hi

I've got a little bug in my site which is damn hard to fix.

http://www.illworm.com/adm/main.php

^^ Go there first

As you can see it's a news website, click any headline you like (say the 'Bug Needs Fixing' headline).

In your browser it looks like this:

http://www.illworm.com/adm/view_article.php?h=Bug Needs Fixin!

Replace anything after the '=' to whatever you want, say

http://www.illworm.com/adm/view_article.php?h=fdsf

sdf sd f

and hit enter in your browser.

As you can see it prints anything after the '=' onto the screen :\ .

How can i fix this? Source code posted below:

http://www.illworm.com/main1.txt

<== source code to main.php

http://www.illworm.com/view_article1.txt

<=== source code to view_article.php.


Thank You.

 

[Vragabond]

$view=str_replace("HEADLINE",$h,$view);

This line in view_article.php replaces the words HEADLINE in an html file with anything that is printed out as an h= at the top. If you do not want that, just comment out the line (//).

 

[sleipnir214]

It looks like you're using the headline as the index for the article. Don't do that. Instead, use numerical indeces in your database and all links.

Also, your code should only ever display data from the database, never directly from the user's input. If a numerical index submitted by a browser doesn't appear in the database, your code should respond with a graceful error message to the user.



It also looks like your code is requires that register_globals be set to "on". Does your code set $h by referencing $_GET['h'] somewhere, or is $h automatically populated? If the former, you have the possibility of a security hole in your code and you should immediately read the PHP online manual section titled "Using Register Globals":

http://www.php.net/manual/en/security.globals.php

Want the best answers? Ask the best questions!

TANSTAAFL!!

 

[d3sol4t3]

Vragabond: Commenting out the line makes no difference :\. It still prints out whatever is entered in the browser.

Anybody else that can help me?

 

[vacunita]

t also looks like your code is requires that register_globals be set to "on". Does your code set $h by referencing $_GET['h'] somewhere, or is $h automatically populated? If the former, you have the possibility of a security hole in your code and you should immediately read the PHP online manual section titled "Using Register Globals":

http://www.php.net/manual/en/security.globals.php

Do that First, there is absolutley no reason that [green]$h[/green] should just be there. It's a security hazard.

Second there is just so much wrong with your code:

Sending text in a URL is not a good practice unless absolutley necessary. Send the article ID not the header it will make comparing easiest.

H is the header that is being sent from the listing page. Since it is being sent with the URL changing the value fo H will clearly change what is sent.

This is where everythng getr funky:
Your code only checks that the the value of H is in the results from the query, (lousy way to do it, again the article ID should be the way to find the correct article.) But doesn't do anything if it is not in the results it just keeps on going.

Then it updates a record with the number of views, but sicne Update doesn't return an error if it finds nothng to update it just moves on.

Then You display whatever was sent with H. along with the article if any it matched. If nothing matched of course then it just displays whatever was sent in the H. which is exactly what it is doing.

You need to do error checking.

If done via the ID, you could query the ID instead of every single Article in your DB. so you would only get a single record back as opposed to hundreds of them.

Second if the ID does not exist in the DB you get an error. So you know its not there you then display a warning. Not whatever bogus data H contains.









----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[d3sol4t3]

Yikes!

Unfortunately i didn't write the code, since im no PHP programmer, but the person who did has funnily dissapeared.

vacunita or anybody for that reason, willing to re-write the code?

 

[vacunita]

Assuming I can get the article ID from the previous page, and heading Sleipnir's advice on Register_globals I would do somethng like:

/*Get ID that was sent from previous page*/
$articleID=$_GET['id'];

$qry="Select *from article where id=". $articleID;
$res=$mysql_query($qry) or die(mysql_error());  //This will catch any error with the 									Database.

/*Check if any results came back*/
if(!$res){
//If no results, then display Warning.
echo "Invalid Article ID";
}
else{
/*if there are results, there should only be one record, the article in question:
so you get the row.*/
$article=mysql_fetch_array($res);

/*Then you do the view counter udpate*/

$sql="update article set visit=visit+1 where id='$articleID'";
mysql_query($sql);
/*$reset="update article set visit=0";
mysql_query($reset);*/
/*get the article contents and header*/
$text=$article['art_content'];
$h=$article['headline'];
/* Do the replacing in the Html file*/
$view=FFileRead("view_article.html");
$view=str_replace("CONTENT",$text,$view);
$view=str_replace("HEADLINE",$h,$view);
/*display*/
print	 $view;

}

----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[vacunita]

Sorry forgot to mention, that I wrote this straight into the Tek-tips textbox, so there might be some errors.

And you need to send the article ID instead of the Header from the previous page. which is easily accomplished.

----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[d3sol4t3]

Thx m8, but i get an error when putting that into View_article.php :

Fatal error: Call to undefined function: () in /home/ufzvnkbd/public_html/illworm/adm/view_article.php on line 5

<?php
include "./inc/connect.php";
$articleID=$_GET['id'];
$qry="Select * from article where id=". $articleID;
$res=$mysql_query($qry) or die(mysql_error());
if(!$res){
echo "Invalid Article ID";
}


^^ First 8 lines

Any idea whats wrong?

 

[KingCrab]

$mysql_query($qry) is incorrect.. remove $

 

[vacunita]

Sorry remove the [red]$[/red] from

$res=[red]$[/red]mysql_query($qry) or die(mysql_error());

so it looke like:

$res=mysql_query($qry) or die(mysql_error());

----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[d3sol4t3]

Yup done, thx u2

From one error to another

http://www.illworm.com/adm/view_article.php?h=21

You have an error in your SQL syntax. Check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1

This is what script looks like for view_article.php ATM:

http://www.illworm.com/view_article1.txt

 

[vacunita]

That's because you're still sending H, and mny code as I explained is expecting ID, any way, change

$articleID=$_GET['id'];
to
$articleID=$_GET['h'];

----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[d3sol4t3]

Thx alot m8, you've helped me solve my problem an totally re-writ the code i was using =)

Now theres just 1 last 'error' (everything works fine unless you modify the whats after the '=' again).

For e.g.

http://www.illworm.com/adm/view_article.php?h=17dvdd

results in:

Unknown column '17dvdd' in 'where clause'

Anyways i appreciate what you've done for me so i've thanked you for all your valuable posts

 

[d3sol4t3]

Oh ya and another thing, i was playing around and managed to find a SQL injection hole in the view_article.php script. How do i fix this ?

POC:

http://www.illworm.com/adm/view_article.php?h=17 union+select something from somewhere where id=1

Thanx

 

[lgarner]

Put single quotes around $h, so the query is:

$query="select * from article where id='.
mysql_real_escape_string($articleID)."'";

The mysql_real_escape_string() function escapes any special characters for MySQL to help avoid SQL-injection attacks.

 

[vacunita]

lgarner is right, but as I said I typed straight into the textbox, so there where bound to be some errors.

Follow lgarner's advice for both queries.






----------------------------------
Ignorance is not necessarily Bliss, case in point:
Unknown has caused an Unknown Error on Unknown and must be shutdown to prevent damage to Unknown.

 

[d3sol4t3]

Right ok i've done what you said now what do i have to do to this line because i get an error in it:

$sql="update article set visit=visit+1 where id='$articleID'";

Parse error: parse error, unexpected T_CONSTANT_ENCAPSED_STRING in /home/ufzvnkbd/public_html/illworm/adm/view_article.php on line 13


Current code im using:

<?php
include "./inc/connect.php";
include "./inc/functions.php";
$articleID=$_GET['h'];
$query="select * from article where id='.
mysql_real_escape_string($articleID)."'";
$res=mysql_query($qry) or die(mysql_error());
if(!$res){
echo "Invalid Article ID";
}
else{
$article=mysql_fetch_array($res);
$sql="update article set visit=visit+1 where id='$articleID'";
mysql_query($sql);
/*$reset="update article set visit=0";
mysql_query($reset);*/

$text=$article['art_content'];
$h=$article['headline'];

$view=FFileRead("view_article.html");
$view=str_replace("CONTENT",$text,$view);
$view=str_replace("HEADLINE",$h,$view);
/*display*/
print $view;

}



?>

 

[jpadie]

this line is wrong

$query="select * from article where id='.
mysql_real_escape_string($articleID)."'";

you have mismatched quotes. change to the text below and the line 13 error will disappear

<?
$query="
		select 
			* 
		from 
			article 
		where 
			id='". mysql_real_escape_string($articleID)."'
		";
?>

tip: i find if you space queries out with logical line breaks and indents it lessens the chances of these errors going unspotted.

 

[d3sol4t3]

ya line 13 error dissapears, i get another one instead

'Query was empty'


Current PHP code being used for view_article.php:

<?php
include "./inc/connect.php";
include "./inc/functions.php";
$articleID=$_GET['h'];


$query="
select
*
from
article
where
id='". mysql_real_escape_string($articleID)."'
";


$res=mysql_query($qry) or die(mysql_error());
if(!$res){
echo "Invalid Article ID";
}
else{
$article=mysql_fetch_array($res);
$sql="update article set visit=visit+1 where id='$articleID'";
mysql_query($sql);
/*$reset="update article set visit=0";
mysql_query($reset);*/

$text=$article['art_content'];
$h=$article['headline'];

$view=FFileRead("view_article.html");
$view=str_replace("CONTENT",$text,$view);
$view=str_replace("HEADLINE",$h,$view);
/*display*/
print $view;

}



?>

http://www.illworm.com/adm/main.php

<=== click a headline and you will see