HELP WITH CODE

[Nazwa29]

Hello I am new to this forum so sorry for any mastikes.

I have my own Cloud service and I am using "OwnCloud" View it here

I am trying to create a registration page and I can accross a code however many people say that there is a lot of ways that you can hack in for example SQL injection etc.

 

 

Here is the orginal post with the code that I found So Please Help

 

 

Demo: http://jungliano.com/JDrive . The codes below worked for me thanks to someone who posted at stackoverflow and i modified them. I have added register.php at the root folder of the installation and the code contains.
<?php
if($_GET["regname"] && $_GET["regpass1"] && $_GET["regpass2"] )
{
if($_GET["regpass1"]==$_GET["regpass2"])
{
$servername="localhost";
$username="dbusername";
$password="dbpassword";
$pass = sha1($_GET['regpass1']);
$conn= mysql_connect($servername,$username,$password)or die(mysql_error());
mysql_select_db("dbname",$conn);
$sql="insert into oc_users
(uid,displayname,password)values('$_GET[regname]','$_GET[regemail]','$pass')";
$result=mysql_query($sql,$conn) or die(mysql_error());
}
else print "passwords doesnt match";
}
else print"invaild data";
header("Location:http://mydomain.com/");
exit;
?>



Replace dbname, username and password to match yours.
Then goto core/templates/login.php and add modified as below.




<div style="margin-top:5px; width: 22em;
padding: 0;
float:right;
padding-right: 400px;"
><div style="margin-bottom:0px;margin-right:30px;padding-left: 40px;"><h1><b>Already Registered? Login</b></h1></div>
<!--[if IE 8]><style>input[type=checkbox]{padding:0;}</style><![endif]-->
<form method="post">
<fieldset>
<?php if (!empty($_['redirect_url'])) {
print_unescaped('<input type="hidden" name="redirect_url" value="' . OC_Util::sanitizeHTML($_['redirect_url']) . '" />');
} ?>
<ul>
<?php if (isset($_['invalidcookie']) && ($_['invalidcookie'])): ?>
<li class="errors">
<?php p($l->t('Automatic logon rejected!')); ?><br>
<small><?php p($l->t('If you did not change your password recently, your account may be compromised!')); ?></small>
<br>
<small><?php p($l->t('Please change your password to secure your account again.')); ?></small>
</li>
<?php endif; ?>
<?php if (isset($_['invalidpassword']) && ($_['invalidpassword'])): ?>
<a href="<?php print_unescaped(OC_Helper::linkToRoute('core_lostpassword_index')) ?>">
<li class="errors">
<?php p($l->t('Lost your password?')); ?>
</li>
</a>
<?php endif; ?>
</ul>
<p class="infield grouptop">
<input type="text" name="user" id="user" placeholder=""
value="<?php p($_['username']); ?>"<?php p($_['user_autofocus'] ? ' autofocus' : ''); ?>
autocomplete="on" required/>
<label for="user" class="infield"><?php p($l->t('Username')); ?></label>
<img class="svg" src="<?php print_unescaped(image_path('', 'actions/user.svg')); ?>" alt=""/>
</p>

<p class="infield groupbottom">
<input type="password" name="password" id="password" value="" data-typetoggle="#show" placeholder=""
required<?php p($_['user_autofocus'] ? '' : ' autofocus'); ?> />
<label for="password" class="infield"><?php p($l->t('Password')); ?></label>
<img class="svg" id="password-icon" src="<?php print_unescaped(image_path('', 'actions/password.svg')); ?>" alt=""/>
<input type="checkbox" id="show" name="show" />
<label for="show"></label>
</p>
<input type="checkbox" name="remember_login" value="1" id="remember_login"/><label
for="remember_login"><?php p($l->t('remember')); ?></label>
<input type="hidden" name="timezone-offset" id="timezone-offset"/>
<input type="submit" id="submit" class="login primary" value="<?php p($l->t('Log in')); ?>"/>
</fieldset>
</form>
<?php if (!empty($_['alt_login'])) { ?>
<form id="alternative-logins">
<fieldset>
<legend><?php p($l->t('Alternative Logins')) ?></legend>
<ul>
<?php foreach($_['alt_login'] as $login): ?>
<li><a class="button" href="<?php print_unescaped($login['href']); ?>" ><?php p($login['name']); ?></a></li>
<?php endforeach; ?>
</ul>
</fieldset>
</form></div>
<?php } ?>

<?php
OCP\Util::addscript('core', 'visitortimezone');

?>
<div style="margin-top:-190px; width: 22em;
float:left;
;">
<div style="margin-bottom:0px;margin-top: -15px;margin-left:50px;"><h1><b>Not Registered? Do It Now</b></h1></div>
<FORM ACTION="register.php" METHOD=get>
<fieldset>
<p class="infield grouptop">
<input type="text" name="regname" id="user" placeholder="Username" value="<?php p($_['username']); ?>"<?php p($_['user_autofocus'] ? ' autofocus' : ''); ?>
autocomplete="on" required/>
<label for="user" ></label>
<img class="svg" src="<?php print_unescaped(image_path('', 'actions/user.svg')); ?>" alt=""/>
</p>

<p class="infield groupbottom">
<input type="password" name="regpass1" id="password" value="" data-typetoggle="#show" placeholder="Password"
required<?php p($_['user_autofocus'] ? '' : ' autofocus'); ?> />
<label for="password" class="infield"></label>
<img class="svg" id="password-icon" src="<?php print_unescaped(image_path('', 'actions/password.svg')); ?>" alt=""/>
<input type="checkbox" id="show" name="show" />
<label for="show"></label>
</p>
<p class="infield groupbottom">
<input type="password" name="regpass2" id="password" value="" data-typetoggle="#show" placeholder="Password"
required<?php p($_['user_autofocus'] ? '' : ' autofocus'); ?> />
<label for="password" class="infield"></label>
<img class="svg" id="password-icon" src="<?php print_unescaped(image_path('', 'actions/password.svg')); ?>" alt=""/>
<input type="checkbox" id="show" name="show" />
<label for="show"></label>
</p>
<input type="hidden" name="timezone-offset" id="timezone-offset"/>
<input type="submit" id="submit" class="login primary" value="Register"/>
</fieldset></form> </div>
</div>

 

Thank You.

[vinny42]

Indeed, it's open to injection.

 

What kind of help are you looking for? because right now you are just saying "people say my code is not safe, please help".

[Ch0cu3r]

The following code will be prone to SQL inject attacks and is not safe to use.

<?php
if($_GET["regname"] && $_GET["regpass1"] && $_GET["regpass2"] )
{
if($_GET["regpass1"]==$_GET["regpass2"])
{
$servername="localhost";
$username="dbusername";
$password="dbpassword";
$pass = sha1($_GET['regpass1']);
$conn= mysql_connect($servername,$username,$password)or die(mysql_error());
mysql_select_db("dbname",$conn);
$sql="insert into oc_users
(uid,displayname,password)values('$_GET[regname]','$_GET[regemail]','$pass')";
$result=mysql_query($sql,$conn) or die(mysql_error());
}
else print "passwords doesnt match";
}
else print"invaild data";
header("Location:http://mydomain.com/");
exit;
?>

You should not have your form submit user credentials via GET. You should be submitting the form with POST method. With GET everything entered in the form will be visible in the browsers url when the form has been submitted. This is very insecure method of sending/receiving of sensitive information.

 

First change your $_GET variables to $_POST.

 

Second, You should never use raw user input (variables such as $_GET, $_POST, $_COOKIE) within SQL queries. Before using any user input you should always validate it, (for example making sure the email address is valid) and sanitize it, (making data safe to use).

 

You can use mysql_real_escape_string to sanitize user input to be used in queries. This is not bullet proof but it will help to protect you from SQL Injection attacks. So change

$sql="insert into oc_users
(uid,displayname,password)values('$_GET[regname]','$_GET[regemail]','$pass')";
$result=mysql_query($sql,$conn) or die(mysql_error());

to

$regname  = mysql_real_escape_string($_POST[regname]);
$regemail = mysql_real_escape_string($_POST[regemail]);

$sql="insert into oc_users
(uid,displayname,password)values('$regname','$regemail','$pass')";
$result=mysql_query($sql,$conn) or die(mysql_error());

Also note that the mysql_* function library is now deprecated and could be removed in future versions of PHP. I would recommend you to use the new mysql improved function library (mysqli extension) instead. 

Edited October 13, 2013 by Ch0cu3r

[Irate]

Example why escaping - for example a password field - is important.

Say you have two input fields, one for username, one for password.

You query the input as following, assuming you are using $_POST and the fields are named "user" and "pass", respectively:

$query = "SELECT * FROM `users` WHERE 'user' = '".$_POST['user']."' AND 'pass' = '".$_POST['pass']."'";

Now, since you don't escape the values, the user might put in "admin" as username and "' OR =''" as password, leaving the above $query with following result:

$query = "SELECT * FROM `users` WHERE 'user' = 'admin' AND 'pass' = '' OR = ''", so the user doesn't need to supply any password to log into any account he wants to. Same works with inserting a semi-colon and then creating a new database user with all priviledges granted etc.

[Nazwa29]

could any one Fix the code for me I dont actually know php at all

[Ch0cu3r]

could any one Fix the code for me I dont actually know php at all

Read my post above? I  pointed out what needs changing.

[Nazwa29]

Read my post above? I  pointed out what needs changing.

 I have changed the top

From

 

<?php

if($_GET["regname"] && $_GET["regpass1"] && $_GET["regpass2"] )

{

if($_GET["regpass1"]==$_GET["regpass2"])

{

$servername="localhost";

$username="dbusername";

$password="dbpassword";

$pass = sha1($_GET['regpass1']);

$conn= mysql_connect($servername,$username,$password)or die(mysql_error());

mysql_select_db("dbname",$conn);

$sql="insert into oc_users

(uid,displayname,password)values('$_GET[regname]','$_GET[regemail]','$pass')";

$result=mysql_query($sql,$conn) or die(mysql_error());

}

else print "passwords doesnt match";

}

else print"invaild data";

header("Location:http://mydomain.com/");

exit;

?>

To

<?php

if($_POST["regname"] && $_POST["regpass1"] && $_POST["regpass2"] )

{

if($_POST["regpass1"]==$_POST["regpass2"])

{

$servername="localhost";

$username="dbusername";

$password="dbpassword";

$pass = sha1($_POST['regpass1']);

$conn= mysql_connect($servername,$username,$password)or die(mysql_error());

mysql_select_db("dbname",$conn);

$regname  = mysql_real_escape_string($_POST[regname]);

$regemail = mysql_real_escape_string($_POST[regemail]);

$sql="insert into oc_users

(uid,displayname,password)values('$regname','$regemail','$pass')";

$result=mysql_query($sql,$conn) or die(mysql_error());

}

else print "passwords doesnt match";

}

else print"invaild data";

header("Location:http://cloud.space-cloud.eu/ ");

exit;

?>

 

 

in the html code there is a line

<FORM ACTION="register.php" METHOD=get>

 

Should I change get to post?

Edited October 13, 2013 by Nazwa29

[Nazwa29]

Is it more secure?

[Irate]

More secure, yes.

However, there are still some things wrong with this...

 

First thing, you're using the mysql module which is deprecated as of PHP 5.2 (I think, it might have been an earlier version number) and it will be removed in the next major PHP release.

Second, you're checking for $_POST['regname'] which will give you an error if the variable is not yet declared - such as if the input field is simply left empty.

You should preceed the line with if( isset($_POST['regname'] && !empty($_POST['regname'] )

Same applies for basically every POST and GET variable you want to query.

[Nazwa29]

More secure, yes.

However, there are still some things wrong with this...

 

First thing, you're using the mysql module which is deprecated as of PHP 5.2 (I think, it might have been an earlier version number) and it will be removed in the next major PHP release.

Second, you're checking for $_POST['regname'] which will give you an error if the variable is not yet declared - such as if the input field is simply left empty.

You should preceed the line with if( isset($_POST['regname'] && !empty($_POST['regname'] )

Same applies for basically every POST and GET variable you want to query.

is it right?

 

<?php

if( isset($_POST['regname'] && !empty($_POST['regname'] )

if($_POST["regname"] && $_POST["regpass1"] && $_POST["regpass2"] )

{

if($_POST["regpass1"]==$_POST["regpass2"])

{

$servername="localhost";

$username="dbusername";

$password="dbpassword";

$pass = sha1($_POST['regpass1']);

$conn= mysql_connect($servername,$username,$password)or die(mysql_error());

mysql_select_db("dbname",$conn);

$regname  = mysql_real_escape_string($_POST[regname]);

$regemail = mysql_real_escape_string($_POST[regemail]);

$sql="insert into oc_users

(uid,displayname,password)values('$regname','$regemail','$pass')";

$result=mysql_query($sql,$conn) or die(mysql_error());

}

else print "passwords doesnt match";

}

else print"invaild data";

header("Location:http://cloud.space-cloud.eu");

exit;

?>

or wrong place?

also in the html

<FORM ACTION="register.php" METHOD=get>

 

Should I change get to post?

[vinny42]

More secure, yes.

[/quote]

 

 

Let's get that out of the way right now; POST is in no way more secure than GET.

The fact that POST parameters don't show up in the URL means very little,  anybody who can press F12 in chrome can edit all of the parameters directly in the page. POST does not protect your data in any way whatsoever.

 

That said; it is usually better to use POST for forms because there is a limit to how long a URL can be and with GET you can reach that limit quite soon. POST can handle much more data and has fewer issues with special characters.

[Ch0cu3r]

Irate meant you need to validate each form field before using it, You code cleaned up and upgraded to use mysqli

<?php

$servername = "localhost";
$username   = "dbusername";
$password   = "dbpassword";
$database   = "dbasename";
$conn = mysqli_connect($servername, $username, $password, $database) or die(mysqli_error());   // connect to database using mysqli

// validate username
if( isset($_POST['regname']) && empty($_POST['regname']) )                                     // make sure username exits
{
	$errors[] = 'Registration name required';                                                  // set error
}

// valid email
if( (isset($_POST['regemail']) && empty($_POST['regemail'])) || !isset($_POST['regemail']))    // make sure email exists
{
	$errors[] = 'Registration email required';                                                 // set error
} 
elseif( isset($_POST['regemail']) && !filter_var($_POST['regemail'], FILTER_VALIDATE_EMAIL) )  // make sure email is a valid address
{
	$errors[] = 'Registration email invalid';                                                  // set error
}

if( isset($_POST['regpass1']) && empty($_POST['regpass1']) )                                   // make sure password exists
{
	$errors[] = 'Password required';                                                           // set error
}
elseif ( isset($_POST['regpass1']) && isset($_POST['regpass2']) && $_POST['regpass1'] != $_POST['regpass1'] ) // make sure passwords match
{
	$errors[] = 'Passwords do not match!';                                                     // set error
}

if(isset($errors) && empty($errors))                                                           // make sure there are no validation errors
{
	$regname  = mysqli_real_escape_string($_POST['regname']);                                  // sanitize username
	$regemail = mysqli_real_escape_string($_POST['regemail']);                                 // sanitize email
	$password = sha1($_POST['regpass1']);                                                      // encrypt password

	$sql = "INSERT INTO oc_users (uid, displayname, password) VALUES ('$regname', '$regemail', '$pass')"; // prepare query
	if($result = mysqli_query($conn, $sql) or die(mysqli_error()))                             // execute query
	{
		echo 'Success user has been registered!';                                              // display message/redirect back to your site 
	}
}
else                                                                                           // $errors contains errors
{
	echo 'Sorry cannot complete registration!'. 
	     '<ul><li>'.implode('</li><li>', $errors).'</li></ul>'.                                // display them
	     '<a href="yoursite.com/registration_form.html">Go Back</a>';                          // provide link back to form, or display the form again
}

?>

[Irate]

 

More secure, yes.

[/quote]

 

 

Let's get that out of the way right now; POST is in no way more secure than GET.

The fact that POST parameters don't show up in the URL means very little, anybody who can press F12 in chrome can edit all of the parameters directly in the page. POST does not protect your data in any way whatsoever.

 

That said; it is usually better to use POST for forms because there is a limit to how long a URL can be and with GET you can reach that limit quite soon. POST can handle much more data and has fewer issues with special characters.

 

I was referring to the escape functions he has implemented.

[vinny42]

Perhaps I quoted the wrong sentince, I thought I was quoting a reply to this:

 

 

You should be submitting the form with POST method. With GET everything entered in the form will be visible in the browsers url when the form has been submitted. This is very insecure method of sending/receiving of sensitive information.