Skip to content
This repository has been archived by the owner on Jun 7, 2019. It is now read-only.

OfHash Implement (Issue 7 Fix) #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

OfHash Implement (Issue 7 Fix) #26

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50be302
This code tried to eat me alive. I risked my life getting this to you…
resba Feb 25, 2011
8c1e676
stop spamming .gitignore
resba Feb 26, 2011
2c84239
goddamnit gitignore.
resba Feb 26, 2011
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
backups/*
npp.bat
.svn
.svn
.DS_Store
6 changes: 5 additions & 1 deletion admin/add_user.php
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
$ignoresrestrictions=$db->escape_string($_POST['ignoresrestrictions']=="on"?"1":"0");
$ip=$db->escape_string($_POST['ip']);
$group=explode(":",$db->escape_string($_POST['groups']));
$password=sha1($db->escape_string($_POST['pass']));
include_once('hash/OfHash.php');
$hash = new OfHash();
$inputstring=$_POST['pass'];
$hashedpassword = $hash->hash($inputstring);
$password=$db->escape_string($hashedpassword);
if (strlen($_POST['pass']) > 0)
{
$db->insert("users",Array("name"=>$nick,
Expand Down
2 changes: 1 addition & 1 deletion admin/buildconfig.php
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@
{
mysql_query('ALTER TABLE users ADD password varchar(255)');
}
mysql_query('insert into users (name, password) values ("admin", "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3");', $conn);
mysql_query('insert into users (name, password) values ("admin", "$O$Brl4bRZY/Lt9yEk.6htwp7hmElu6rO1gXFLmzoBTyYHLpZWUDy.gkyUtZoqakeESC1Z5JsXlPbPkoPJHP95hSzoN4aiDe2/");', $conn);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, you might as well just include OfHash in here and run it. That way, it's harder to detect if the default password is in use, just by looking at the hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default pass is supposed to be changed, its just inserted for placeholder.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, having a policy of assuming people not being idiots is a poor one at best. Unless you're asking for an admin password in an installation script, you're doing it in a way that leaves the application vulnerable to a stupid administrator.

Remember, you don't just have to protect applications from attackers, but also from stupid users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would involve asking the user to make a password during installation, right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ayup. Note that this is not necessarily a bad thing, actually, since it also means that there is a non-static "admin" account that's created as well preventing an attacker from knowing a "guaranteed" or "highly likely" account name.


$rs = mysql_query('SELECT * FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_NAME = "items" and table_schema = "'.$_POST['mysql_database'].'"', $conn);
if (mysql_num_rows($rs) == 0)
Expand Down
5 changes: 4 additions & 1 deletion admin/edit_user.php
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
$ignoresrestrictions=$_POST['ignoresrestrictions']=="on"?"1":"0";
$ip=$_POST['ip'];
$group=explode(":",$_POST['groups']);
$password=sha1($_POST['pass']);
include_once('hash/OfHash.php');
$hash = new OfHash();
$inputstring=$_POST['pass'];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to check if the strlen of $_POST['pass'] is greater than 0 here BEFORE running the hasher -- no reason to run the hasher if there's no input here, since it's just editing a user (and not login, where it does actually matter).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It takes the POST, hashes it, then I use $password in the edit, since you can edit a password.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but hash it /after/ checking the string length. It is more efficient this way, as you don't hash if the user didn't even provide a password to hash.

$password = $hash->hash($inputstring);
if (strlen($_POST['pass']) > 0)
{
$db->set("users",Array(
Expand Down
26 changes: 24 additions & 2 deletions admin/hash/OfHash.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
* Minimum Requirement: PHP 5.0.0
*/

if(!defined('OF_ROOT')) exit;

/**
* OpenFlame Web Framework - Hashing framework,
Expand Down Expand Up @@ -168,6 +167,29 @@ public function check($password, $stored_hash)
if ($hash[0] == '*')
$hash = crypt($password, $stored_hash);

return $hash == $stored_hash;
return $this->full_compare($hash, $stored_hash);
}

/**
* A time-insensitive string comparison function, to help deter highly accurate timing attacks.
* @param string $a - The first string to compare
* @param string $b - The second string to compare
* @return boolean - Do the strings match?
*
* @license - Public Domain - http://twitter.com/padraicb/status/41055320243437568
* @link http://blog.astrumfutura.com/2010/10/nanosecond-scale-remote-timing-attacks-on-php-applications-time-to-take-them-seriously/
* @author http://twitter.com/padraicb
*/
public function full_compare($a, $b)
{
if(strlen($a) !== strlen($b))
return false;

$result = 0;

for($i = 0, $size = strlen($a); $i < $size; $i++)
$result |= ord($a[$i]) ^ ord($b[$i]);

return $result == 0;
}
}
11 changes: 7 additions & 4 deletions admin/login_process.php
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
<?php
session_start();
require_once('config.php');
require_once('hash/OfHash.php');
$hash = new OfHash();


if($useflatfile){
require("flatfile.class.php");
Expand All @@ -16,10 +19,10 @@
echo "MySQL Configuration Incorrect.";
exit();
}
$result=$db->fetch_by("users",Array("password"=>sha1($_POST['pass']),"name"=>$_POST['user']),"");
if($result['password'] == sha1($_POST['pass'])) {
$_SESSION['user']=$result["name"];
echo "1";
$result=$db->fetch_by("users",Array("password"=>$_POST['pass'],"name"=>$_POST['user']),"");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're still fetching with the hash as a WHERE condition in the select. Not gonna work.

if($hash->check($_POST['pass'], $result['password'])){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure you're also running OfHash if the user doesn't exist, and that you're handling the "user does not exist" cases properly still. I can't tell by the diff context, it's up to you to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say "running OfHash" what do you mean exactly?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either generating a hash initially or checking a password; basically something that will "execute" the hash generation code for OfHash.
In this case, checking a bad password would take up the time necessary to avoid timing attacks.

Useful article if you're not aware of the growing precision OF timing attacks: http://blog.astrumfutura.com/2010/10/nanosecond-scale-remote-timing-attacks-on-php-applications-time-to-take-them-seriously/

$_SESSION['user']=$result["name"];
echo "1";
}else{
echo "0";
}
Expand Down
2 changes: 1 addition & 1 deletion admin/mineadmin.sql
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ CALL addcol();

DROP PROCEDURE addcol;

insert into users (name, groups, password) values ('admin','default', 'a94a8fe5ccb19ba61c4c0873d391e987982fbbd3');
insert into users (name, groups, password) values ('admin','default', '$O$Brl4bRZY/Lt9yEk.6htwp7hmElu6rO1gXFLmzoBTyYHLpZWUDy.gkyUtZoqakeESC1Z5JsXlPbPkoPJHP95hSzoN4aiDe2/');
20 changes: 3 additions & 17 deletions index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,15 @@
<html lang="en">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<title>MinecraftServers.com</title>
<style type="text/css">
#wrapper{display:table;overflow:hidden;margin:0px auto;}
#container{display:table-cell;}
html,body{height:100%;}
body {background: #d3d3d3 url('images/bg.png') no-repeat top center; font-family: sans-serif; font-size: 16px;}
a {text-decoration: none;color:#144564;}
ul,li {}
#footer {color: #b2b2b2;text-shadow:#fff 0px 1px 0, #999 0 -1px 0;}
#footer a {color: #144564;text-decoration: none;}
#wrapper{height:100%;width:800px;}
#content {margin-top: 75px;text-align: left;}
#footer {position: absolute;bottom: 15px;width:800px;text-align: center;}
</style>
<title>MinecraftServers.com</title><link rel="stylesheet" type="text/css" href="style.css">
</head>
<body>
<div id="wrapper">
<div id="container">
<div id="content">
<?php echo Markdown(file_get_contents('admin/markdown.md'));?>
</div>
<?php echo Markdown(file_get_contents('admin/markdown.md'));?></div>
<div id="footer">Powered by <a href="http://minecraftservers.com">MinecraftServers.com</a></div>
</div>
</div>
</body>
</html>
</html>
11 changes: 11 additions & 0 deletions style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#wrapper{display:table;overflow:hidden;margin:0px auto;}
#container{display:table-cell;}
html,body{height:100%;}
body {background: #d3d3d3 url('images/bg.png') no-repeat top center; font-family: sans-serif; font-size: 16px;}
a {text-decoration: none;color:#144564;}
ul,li {}
#footer {color: #b2b2b2;text-shadow:#fff 0px 1px 0, #999 0 -1px 0;}
#footer a {color: #144564;text-decoration: none;}
#wrapper{height:100%;width:800px;}
#content {margin-top: 75px;text-align: left;}
#footer {position: absolute;bottom: 15px;width:800px;text-align: center;}