patternphpMinor
Safety issues in PHP log-in system
Viewed 0 times
logissuesphpsystemsafety
Problem
This is a user login (some session wrapper I managed to put together after a lot of web searching).
It's for a simple CMS I'm trying to build. It only needs one user and there is no need for multiple user log in at the same time. It works, the problem being I think it has safety issues.
It works like this:
I commented it quite well. If you see any safety issues please point them out as I can't seem to notice them.
The form is a simple user, password and submit form.
The class:
```
getConnection();
$sql = 'SELECT * FROM utilizatori ';
$sql .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';
$result = $mysqli->query($sql) or die (mysqli_error());
if($row = $result->fetch_assoc()){
if(($row['user'] === self::$_user)&&($row['password'] === self::$_password)){
self::set('user',self::$_user);
self::set('key',session_id());
}
}
}
//this method starts the session
public static function start(){
if(self::$_sessionStart == false){
session_start();
self::$_sessionStart = true;
}
}
//this function sets the session values
public static function set($key,$value){
$_SESSION[$key] = $value;
}
public static function get($key){
if(isset($_SESSION[$key])){
return $_SESSION[$key];
}
else{
return false;
}
}
}
//from here is the handeling of the data in the "index" page:
include("class_def/database.inc");
include("class_def/user.inc");
Ses
It's for a simple CMS I'm trying to build. It only needs one user and there is no need for multiple user log in at the same time. It works, the problem being I think it has safety issues.
It works like this:
- The user accesses CMS index page, and inserts the user and password.
- If one of them does not coincide with the data in the database it does not set the session and triggers a "die" or a error message.
- If the info is ok then it sets the session and redirects the user from the index page to an admin.php page as you can see in the code below.
I commented it quite well. If you see any safety issues please point them out as I can't seem to notice them.
The form is a simple user, password and submit form.
The class:
```
getConnection();
$sql = 'SELECT * FROM utilizatori ';
$sql .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';
$result = $mysqli->query($sql) or die (mysqli_error());
if($row = $result->fetch_assoc()){
if(($row['user'] === self::$_user)&&($row['password'] === self::$_password)){
self::set('user',self::$_user);
self::set('key',session_id());
}
}
}
//this method starts the session
public static function start(){
if(self::$_sessionStart == false){
session_start();
self::$_sessionStart = true;
}
}
//this function sets the session values
public static function set($key,$value){
$_SESSION[$key] = $value;
}
public static function get($key){
if(isset($_SESSION[$key])){
return $_SESSION[$key];
}
else{
return false;
}
}
}
//from here is the handeling of the data in the "index" page:
include("class_def/database.inc");
include("class_def/user.inc");
Ses
Solution
MVC
If you're trying to make a CMS, then there's a possibility that this CMS will become bigger than you first though. More pages, more modules, more code. You might want to consider doing an MVP approach to splitting code (rather than MVC).
A good reference framework for this is CodeIgniter. Presenter (CodeIgninter calls it the Controller) receives a request, then operates the data on the model (the model layer + database), then grabs a view (A template, your HTML) and renders it ("echo" the template).
SQL Injection
Then I notice this part:
You should read more about SQL injection. In a gist, it allows users to run arbitrary SQL, even be able to fake logins, see through existing usernames and passwords, as well as drop entire databases.
Sessions
Then there's sessions. For simple situations, that's fine. But for security, there's a lot to consider. If I can shoot malicious code through the forms, then I just might be able to fake my session. Read on this post for things to consider with sessions.
Validation
I see you use
Redirect
This operates on the browser. This means that
If you're trying to make a CMS, then there's a possibility that this CMS will become bigger than you first though. More pages, more modules, more code. You might want to consider doing an MVP approach to splitting code (rather than MVC).
A good reference framework for this is CodeIgniter. Presenter (CodeIgninter calls it the Controller) receives a request, then operates the data on the model (the model layer + database), then grabs a view (A template, your HTML) and renders it ("echo" the template).
SQL Injection
Then I notice this part:
$sql = 'SELECT * FROM utilizatori ';
$sql .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';You should read more about SQL injection. In a gist, it allows users to run arbitrary SQL, even be able to fake logins, see through existing usernames and passwords, as well as drop entire databases.
Sessions
Then there's sessions. For simple situations, that's fine. But for security, there's a lot to consider. If I can shoot malicious code through the forms, then I just might be able to fake my session. Read on this post for things to consider with sessions.
Validation
I see you use
strip_tags() but it only does what it does, strip tags. It does not strip the other stuff, like backticks and quotes which can still be used to break in. A good example is the story of Little Bobby Tables. No tags, still broke through the database.Redirect
window.location = "admin.php"This operates on the browser. This means that
index.php is actually served before redirecting, possibly rendering the page before this runs. I suggest you do a header redirect. It does not serve the page to the browser. It just tells the browser directly to load another page instead.Code Snippets
$sql = 'SELECT * FROM utilizatori ';
$sql .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';window.location = "admin.php"Context
StackExchange Code Review Q#44087, answer score: 6
Revisions (0)
No revisions yet.