Logging system safety

Question

I've created a logging system with one-use passwords (sent via SMS API). What I would like to know is if it's "safe". I know it is hard to estimate safety but I am curious if it is safer than regular password. I would appreciate any advice to improve following code.

Scheme is:

Logging:

Loggin at a webpage -> Ask external server for password -> Store the password in session var and when form is sent compare it with the sent one

External script:

When asked with proper apikey -> give random password and send it via SMS API

The code is below:

External script:

    <?php 

   $apiKey='foobar123';

   if ($_GET['api']!=$apiKey) {

       header('HTTP/1.0 404 Not Found');
       die();
   }

    $ile=12; //char number

    $onepass='';
    for($i=0;$i<$ile;$i++)
    {
        $k=rand(1,10);
        $onepass.=$letter[$k];
    }
    if (!isset($_POST['ip'],$_POST['ip'])) {echo 'error'; die();}
    $ip=$_POST['ip'];
    $www=$_POST['www'];
    echo $onepass;

    include('sender.php'); //sending API

    ?>

Logging script (at page foo.com)

    <?php 
    session_start();

    $sessi='1231dsahsda8'; 
    $www="foo.com";
    $ip=$_SERVER['REMOTE_ADDR'];

    if(isset($_GET['destroyer'])){
        if($_GET['destroyer']=='yes'){
            session_destroy(); echo 'Logged out - <a href="?relog">Log in again</a>'; die();
        }
        if($_GET['destroyer']=='no'){
            session_destroy(); echo 'New pass has been sent!'; session_start();
        }
    }

    if (isset($_POST['password'])){
        if($_POST['password']==$_SESSION['pass']) $_SESSION[$sessi]="log";
        else echo'Wrong Password <a href="?destroyer=no">Send again</a>';
    }

    if ($_SESSION[$sessi]!="log") { 
        if(!isset($_SESSION['pass'])){
            $adres='http://www.foobar.com/index.php?api=foobar123';
            $c = curl_init();
            curl_setopt($c, CURLOPT_URL, $adres);
            curl_setopt($c, CURLOPT_POST, 1);
            curl_setopt($c, CURLOPT_POSTFIELDS, "ip=$ip&www=$www"); 
            curl_setopt($c, CURLOPT_RETURNTRANSFER, 1); /
            $dane=curl_exec($c); 
            curl_close($c); 
            $_SESSION['pass']=$dane;
            echo 'Password has been sent';
        }
        echo'<p>Log in</p><form action="?a" method="post"><input type="password" name="password" value=""/><input type="submit" value="Login" /></form>';
    }
    else{//authorised
         echo 'Logged <p><a href="?destroyer=yes">Logout</a></p'; 
    }

    ?>

Why have you decided to split this up into two scripts? It seems that everything taking place in the "api" script could replace the curl stuff in the log in script (not "logging"). — Michael Zedeler
– Michael Zedeler, Commented Nov 18, 2012 at 21:33
in the future i would like to store in one place phone number for each user and have one account for few sites - that's why I would like to store it in another place — Niedam Wam
– Niedam Wam, Commented Nov 20, 2012 at 18:00
Somethnig just doesnt seem right to me about storing a password in a session... — SaggingRufus
– SaggingRufus, Commented Jan 6, 2014 at 12:09

palacsint · Accepted Answer · 2014-03-24 12:04:38Z

This probably makes integration and debugging harder:
```
if ($_GET['api']!=$apiKey) {
   header('HTTP/1.0 404 Not Found');
   die();
}
```
The error message is misleading, if someone use an older or wrong API key they probably will spend some time searching the error in a wrong place since it indicates that the URL is wrong, not the key. 403 Forbidden would be more convenient here.
This should be in a configuration file, it might change more often than the code itself:
```
$apiKey='foobar123';
```
This generates a random number:
```
$k=rand(1,10);
```
Documentation of mt_rand says the following:

mt_rand — Generate a better random value

So I guess using mt_rand would we better. Furthermore, the linked page contains another interesting thing:

[...]

Caution

This function does not generate cryptographically secure values, and should not be used for cryptographic purposes. If you need a cryptographically secure value, consider using openssl_random_pseudo_bytes() instead.

It really depends on the application and the sensitivity of your data which is more appropriate.
This usually referred as one-time password:

one-use passwords
This check could be at the beginning of the script:
```
if (!isset($_POST['ip'],$_POST['ip'])) {echo 'error'; die();}
```
It's unnecessary to create an one-time password if the ip is empty.

Additionally, I guess the following is the same:
```
if (!isset($_POST['ip'])) {echo 'error'; die();}
```
You could eliminate the comment here with better variable naming:
```
$ile=12; //char number
```
Just rename it to $passwordLength.

See also: Clean Code by Robert C. Martin, Bad Comments, p67: Don’t Use a Comment When You Can Use a Function or a Variable
Two typos:
- $adres should be $address
- $dane to $done
I'd rename $sessi to a descriptive name. What does this variable store, what's its purpose? Put that into the name.
I'd change log to something more descriptive here:
```
$_SESSION[$sessi]="log";
```
logged_in would be easier to understand since it describes a state not an action (like log).

Stack Exchange Network

Logging system safety

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Logging system safety

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions