From 1fe03a6906c8233cedc00fe9f763d2185d74cc87 Mon Sep 17 00:00:00 2001
From: Christoph Hagen <github@christophhagen.de>
Date: Fri, 8 Dec 2023 00:24:15 +0100
Subject: [PATCH] Renew challenge on expiry

---
 include/controller.h | 11 +++++++++++
 include/message.h    |  7 +++++--
 src/controller.cpp   | 23 ++++++++++++++++-------
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/include/controller.h b/include/controller.h
index 8b30979..aec521b 100644
--- a/include/controller.h
+++ b/include/controller.h
@@ -113,6 +113,17 @@ private:
      */
     void processMessage(SignedMessage* message);
 
+    /**
+     * @brief Checks that the message is valid and prepares a challenge.
+     * 
+     * This function is also called when a challenge response arrives too late.
+     * 
+     * @param message The message to respond to
+     * 
+     * Note: Prepares the response in the outgoing message buffer.
+     */
+    void checkAndPrepareChallenge(Message* message);
+
     /**
      * @brief Prepare a server challenge for a local or socket message.
      * 
diff --git a/include/message.h b/include/message.h
index fa8c944..f0d6685 100644
--- a/include/message.h
+++ b/include/message.h
@@ -59,8 +59,11 @@ enum class MessageResult: uint8_t {
     /// @brief A message is already being processed
     TooManyRequests = 8,
 
-    InvalidUrlParameter = 10, 
-    InvalidResponseAuthentication = 11,
+    /// @brief The received message result was invalid
+    InvalidMessageResult = 9,
+
+    /// @brief An invalid Url parameter was set sending a message to the device over a local connection
+    InvalidUrlParameter = 10,
 
 };
 
diff --git a/src/controller.cpp b/src/controller.cpp
index f82c182..c27f12e 100644
--- a/src/controller.cpp
+++ b/src/controller.cpp
@@ -116,7 +116,7 @@ void SesameController::sendPreparedResponseToServer() {
 void SesameController::processMessage(SignedMessage* message) {
     // Result must be empty
     if (message->message.result != MessageResult::MessageAccepted) {
-        prepareResponseBuffer(MessageResult::ClientChallengeInvalid);
+        prepareResponseBuffer(MessageResult::InvalidMessageResult);
         return;
     }
     if (!isAuthenticMessage(message, keyConfig.remoteKey)) {
@@ -125,7 +125,7 @@ void SesameController::processMessage(SignedMessage* message) {
     }
     switch (message->message.messageType) {
     case MessageType::initial:
-        prepareChallenge(&message->message);
+        checkAndPrepareChallenge(&message->message);
         return;
     case MessageType::request:
         completeUnlockRequest(&message->message);
@@ -136,12 +136,16 @@ void SesameController::processMessage(SignedMessage* message) {
     }
 }
 
-void SesameController::prepareChallenge(Message* message) {
+void SesameController::checkAndPrepareChallenge(Message* message) {
     // Server challenge must be empty
     if (message->serverChallenge != 0) {
         prepareResponseBuffer(MessageResult::ClientChallengeInvalid);
         return;
     }
+    prepareChallenge(message);
+}
+
+void SesameController::prepareChallenge(Message* message) {
     if (hasCurrentChallenge()) {
         Serial.println("[INFO] Overwriting old challenge");
     }
@@ -155,10 +159,6 @@ void SesameController::prepareChallenge(Message* message) {
 }
 
 void SesameController::completeUnlockRequest(Message* message) {
-    if (!hasCurrentChallenge()) {
-        prepareResponseBuffer(MessageResult::ClientChallengeInvalid, message);
-        return;
-    }
     // Client and server challenge must match
     if (message->clientChallenge != currentClientChallenge) {
         prepareResponseBuffer(MessageResult::ClientChallengeInvalid, message);
@@ -168,6 +168,15 @@ void SesameController::completeUnlockRequest(Message* message) {
         prepareResponseBuffer(MessageResult::ServerChallengeMismatch, message);
         return;
     }
+    if (!hasCurrentChallenge()) {
+        // Directly send new challenge on expiry, since rest of message is valid
+        // This allows the remote to directly try again without requesting a new challenge.
+        // Security note: The client nonce is reused in this case, but an attacker would still 
+        // not be able to create a valid unlock request due to the new server nonce.
+        prepareChallenge(message);
+        return;
+    }
+
     clearCurrentChallenge();
     
     // Move servo