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'; 
 }
 ?>

• Michael Zedeler: 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").
• Niedam Wam: 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
• SaggingRufus: Somethnig just doesnt seem right to me about storing a password in a session...

Answer 1

1. 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.

2. This should be in a configuration file, it might change more often than the code itself:

   $apiKey='foobar123';
   

3. 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.

4. This usually referred as one-time password:

   one-use passwords

5. 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();}
   

6. 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

7. Two typos:

   • $adres should be $address
   • $dane to $done

8. I'd rename $sessi to a descriptive name. What does this variable store, what's its purpose? Put that into the name.

9. 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).