From c697f9b25a5a8f0b35575031ea22651c090916cb Mon Sep 17 00:00:00 2001
From: davidearl <david@frankieandshadow.com>
Date: Thu, 8 Dec 2022 18:17:40 +0000
Subject: [PATCH] fixes #40 fixes #45 prevent login replays

The strings passed to prepareForLogin and authenticate are now modified, so the state can be saved back to the user database. Once authenticated, it can't be used again. It doesn't actually work as a timeout, but a login attempt invalidates previous login attempts, and a valid authentication also invalidates re-use of that login.
---
 README.md             | 12 +++++++++++-
 WebAuthn/WebAuthn.php | 39 +++++++++++++++++++++++++++++++--------
 example/index.php     | 22 ++++++++++++++++------
 3 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/README.md b/README.md
index 3384955..b4efdb5 100644
--- a/README.md
+++ b/README.md
@@ -42,6 +42,15 @@ enough to put into a convenient JSON form to transport to the server),
 and I thought I would share it. Several others have since helped with support for broader
 application with fingerprints and Windows Hello.
 
+## Changes 8 Dec 2022
+
+To prevent a possible replay of a login, the user's webauthn string is
+now passed by reference to prepareForLogin and authenticate, both of
+which modify it. The string should now be saved with the user in your
+user after these calls, as well as when registering. This saves and
+then cross-checks and clears the challenge data sent to the key, so
+that it cannot be re-used.
+
 ## Changes from branch 0.1.0
 
 The original code was updated in August 2019 by a number of
@@ -63,7 +72,8 @@ This requires
 * [phpseclib](https://github.com/phpseclib/phpseclib), ditto
 * A recent openssl included in PHP ([openssl_verify](http://php.net/manual/en/function.openssl-verify.php)
 in particular)
-* PHP 5.6 or later (preferably PHP 7)
+* PHP 5.6 or later (preferably PHP 7.4 or 8.1; not tested with PHP 8.2, which has some significant changes
+  to property declarations)
 
 ## Example
 
diff --git a/WebAuthn/WebAuthn.php b/WebAuthn/WebAuthn.php
index b743dfd..66af242 100644
--- a/WebAuthn/WebAuthn.php
+++ b/WebAuthn/WebAuthn.php
@@ -234,22 +234,31 @@ public function register($info, $userwebauthn)
 
     /**
     * generates a new key string for the physical key, fingerprint
-    * reader or whatever to respond to on login
-    * @param string $userwebauthn the existing webauthn field for the user from your database
+    * reader or whatever to respond to on login.
+    * You should store the revised userwebauthn back to your database after calling this function
+    * (to avoid replay attacks)
+    * @param string &$userwebauthn the existing webauthn field for the user from your database
     * @return string to pass to javascript webauthnAuthenticate
     */
-    public function prepareForLogin($userwebauthn)
+    public function prepareForLogin(&$userwebauthn)
     {
         $allow = (object)array();
         $allow->type = 'public-key';
         $allow->transports = array('usb','nfc','ble','internal');
         $allow->id = null;
         $allows = array();
+
+        $challengebytes = self::randomBytes(16);
+        $challengeb64 = rtrim(strtr(base64_encode($challengebytes), '+/', '-_'), '=');
+        
         if (! empty($userwebauthn)) {
-            foreach (json_decode($userwebauthn) as $key) {
+            $webauthn = json_decode($userwebauthn);
+            foreach ($webauthn as $idx => $key) {
                 $allow->id = $key->id;
                 $allows[] = clone $allow;
+                $webauthn[$idx]->challenge = $challengeb64;
             }
+            $userwebauthn = json_encode($webauthn);
         } else {
             /* including empty user, so they can't tell whether the user exists or not (need same result each
             time for each user) */
@@ -262,7 +271,7 @@ public function prepareForLogin($userwebauthn)
 
         /* generate key request */
         $publickey = (object)array();
-        $publickey->challenge = self::stringToArray(self::randomBytes(16));
+        $publickey->challenge = self::stringToArray($challengebytes);
         $publickey->timeout = 60000;
         $publickey->allowCredentials = $allows;
         $publickey->userVerification = 'discouraged';
@@ -275,15 +284,17 @@ public function prepareForLogin($userwebauthn)
 
     /**
     * validates a response for login or 2fa
-    * requires info from the hardware via javascript given below
+    * requires info from the hardware via javascript given below.
+    * You should store the revised userwebauthn back to your database after calling this function
+    * (to avoid replay attacks)
     * @param string $info supplied to the PHP script via a POST, constructed by the Javascript given below, ultimately
     *        provided by the key
-    * @param string $userwebauthn the exisiting webauthn field for the user from your
+    * @param string &$userwebauthn the exisiting webauthn field for the user from your
     *        database (it's actaully a JSON string, but that's entirely internal to
     *        this code)
     * @return boolean true for valid authentication or false for failed validation
     */
-    public function authenticate($info, $userwebauthn)
+    public function authenticate($info, &$userwebauthn)
     {
         if (! is_string($info)) {
             $this->oops('info must be a string', 1);
@@ -315,6 +326,18 @@ public function authenticate($info, $userwebauthn)
             $this->oops("challenge mismatch");
         }
 
+        /* Does the challenge Correspond to the one we stored for the user? If no challenge is stored, that
+           implies $userwebauthn was not saved back to the user database after prepareForLogin. It
+           would be better to produce an error here, but for the purposes of backward compatibility, it'll 
+           allow it, but with a replay vulnerability */
+        //log(print_r($key->challenge,1).' '.print_r($info->response->clientData->challenge,1));
+        if (isset($key->challenge) && $key->challenge != $info->response->clientData->challenge) {
+            $this->oops("you cannot use the same login more than once");
+        }
+        /* clear the challenge (but retain it as a property) from each of the keys, so it cannot be re-used */
+        foreach($webauthn as $idx => $candkey) { $webauthn[$idx]->challenge = ''; }
+        $userwebauthn = json_encode($webauthn);
+        
         /* cross check origin */
         $origin = parse_url($info->response->clientData->origin);
         if ($this->appid != $origin['host']) {
diff --git a/example/index.php b/example/index.php
index 864cdae..380bff6 100644
--- a/example/index.php
+++ b/example/index.php
@@ -40,6 +40,10 @@ function getuser($username){
   return $user;
 }
 
+function saveuser($user){
+  file_put_contents(userpath($user->name), json_encode($user));
+}
+
 /* A post is an ajax request, otherwise display the page */
 if (! empty($_POST)) {
 
@@ -52,7 +56,7 @@ function getuser($username){
     case isset($_POST['registerusername']):
       /* initiate the registration */
       $username = $_POST['registerusername'];
-	  $crossplatform = ! empty($_POST['crossplatform']) && $_POST['crossplatform'] == 'Yes';
+      $crossplatform = ! empty($_POST['crossplatform']) && $_POST['crossplatform'] == 'Yes';
       $userid = md5(time() . '-'. rand(1,1000000000));
 
       if (file_exists(userpath($username))) {
@@ -64,11 +68,12 @@ function getuser($username){
          but you'd probably do that from a user profile page rather than initial
          registration. The procedure is the same, just don't cancel existing
          keys like this.*/
-      file_put_contents(userpath($username), json_encode(['name'=> $username,
-                                                          'id'=> $userid,
-                                                          'webauthnkeys' => $webauthn->cancel()]));
+      $user = (object)['name'=> $username,
+                       'id'=> $userid,
+                       'webauthnkeys' => $webauthn->cancel()];
+      saveuser($user);
       $_SESSION['username'] = $username;
-	  $j = ['challenge' => $webauthn->prepareChallengeForRegistration($username, $userid, $crossplatform)];
+      $j = ['challenge' => $webauthn->prepareChallengeForRegistration($username, $userid, $crossplatform)];
       break;
 
     case isset($_POST['register']):
@@ -81,7 +86,7 @@ function getuser($username){
 
       /* Save the result to enable a challenge to be raised agains this
          newly created key in order to log in */
-      file_put_contents(userpath($user->name), json_encode($user));
+      saveuser($user);
       $j = 'ok';
 
       break;
@@ -97,6 +102,9 @@ function getuser($username){
          people to interrogate your user database for existence */
 
       $j['challenge'] = $webauthn->prepareForLogin($user->webauthnkeys);
+
+      /* Save user again, which sets server state to include the challenge expected */
+      saveuser($user);
       break;
 
     case isset($_POST['login']):
@@ -109,6 +117,8 @@ function getuser($username){
         echo 'failed to authenticate with that key';
         exit;
       }
+      /* Save user again, which sets server state to include the challenge expected */
+      saveuser($user);
       $j = 'ok';
 
       break;