Downlink replay protection rework

src/saead/downlink.c

@@ -20,6 +20,8 @@ POUCH_LOG_REGISTER(saead_downlink, CONFIG_POUCH_COMMON_LOG_LEVEL);
 static struct session downlink;
 static struct
 {
+    // Whether we have received a valid sequence number from the server
+    bool has_seqnum;
     /**
      * Highest sequence number we've seen the server create a session with.
      * If replay protection isn't enabled, we can still use this to ensure that the server hasn't
@@ -28,6 +30,7 @@ static struct
      * replay attacks.
      */
     uint64_t seqnum;
+
     /**
      * Highest pouch ID we've seen the server send in the current session. Is only updated once
      * at least one block of the pouch is decrypted.
@@ -36,26 +39,34 @@ static struct
 } server;
 
 /** Check that this session is a valid follow up to the previous downlink session */
-static bool is_valid_downlink(const struct session_id *id, psa_algorithm_t algorithm)
+static bool is_valid_downlink(const struct session_id *id,
+                              uint8_t max_block_size_log,
+                              psa_algorithm_t algorithm)
 {
-    if (!pouch_atomic_test_bit(&downlink.flags, SESSION_VALID))
+    if (id->initiator == POUCH_ROLE_DEVICE)
     {
-        // No previous session to invalidate the incoming session
-        return true;
-    }
+        if (id->type != SESSION_ID_TYPE_SEQUENTIAL)
+        {
+            // The server can only use our session if it's a sequential session ID
+            POUCH_LOG_ERR("Session reuse failed: ID not sequential");
+            return false;
+        }
 
-    if (session_id_is_equal(&downlink.id, id) && downlink.algorithm != algorithm)
-    {
-        // Session ID is unchanged, parameters must be identical
-        POUCH_LOG_ERR("Algorithm doesn't match");
-        return false;
+        if (!saead_uplink_session_matches(id, max_block_size_log, algorithm))
+        {
+            // The server claims to use our uplink's session, but it doesn't match
+            POUCH_LOG_ERR("Session reuse failed: No match");
+            return false;
+        }
+
+        return true;
     }
 
-    if (id->initiator == POUCH_ROLE_SERVER)
+    // server initiated, sequential ID:
+    if (id->type == SESSION_ID_TYPE_SEQUENTIAL)
     {
-        // This was initiated by the server. If it's sequential, we can validate the sequence
-        // number.
-        if (id->type == SESSION_ID_TYPE_SEQUENTIAL && id->value.sequential.seqnum <= server.seqnum)
+        // seqnum must be increasing:
+        if (server.has_seqnum && id->value.sequential.seqnum <= server.seqnum)
         {
             POUCH_LOG_ERR("Old seqnum: %" PRIu64 " (was %" PRIu64 ")",
                           id->value.sequential.seqnum,
@@ -74,28 +85,28 @@ int saead_downlink_session_start(const struct session_id *id,
 {
     psa_key_id_t session_key;
 
-    if (!is_valid_downlink(id, algorithm))
+    int match = session_match(&downlink, id, max_block_size_log, algorithm);
+    if (match < 0)
     {
-        POUCH_LOG_ERR("Invalid downlink");
+        POUCH_LOG_ERR("Session parameter changed");
         return -EBADMSG;
     }
 
-    if (id->initiator == POUCH_ROLE_DEVICE)
+    if (match && pouch_atomic_test_bit(&downlink.flags, SESSION_ACTIVE))
     {
-        if (id->type != SESSION_ID_TYPE_SEQUENTIAL)
-        {
-            // The server can only use our session if it's a sequential session ID
-            POUCH_LOG_ERR("Session reuse failed: ID not sequential");
-            return -EBADMSG;
-        }
+        // This is the current session. We already have a session key, and shouldn't
+        // recalculate or reset anything.
+        return 0;
+    }
 
-        if (!saead_uplink_session_matches(id, max_block_size_log, algorithm))
-        {
-            // The server claims to use our uplink's session, but it doesn't match
-            POUCH_LOG_ERR("Session reuse failed: No match");
-            return -EBADMSG;
-        }
+    if (!is_valid_downlink(id, max_block_size_log, algorithm))
+    {
+        POUCH_LOG_ERR("Invalid downlink");
+        return -EBADMSG;
+    }
 
+    if (id->initiator == POUCH_ROLE_DEVICE)
+    {
         // We can make a copy of the uplink session's key instead of deriving it again:
         session_key = saead_uplink_session_key_copy(DOWNLINK_KEY_USAGE);
     }
@@ -121,6 +132,7 @@ int saead_downlink_session_start(const struct session_id *id,
 
     downlink.flags = POUCH_ATOMIC_INIT(0);
     downlink.pouch.id = 0;
+    downlink.max_block_size_log = max_block_size_log;
     downlink.algorithm = algorithm;
     downlink.key = session_key;
     downlink.id = *id;
@@ -170,10 +182,12 @@ int saead_downlink_block_decrypt(const struct pouch_buf *block, struct pouch_buf
     pouch_atomic_set_bit(&downlink.flags, SESSION_HAS_POUCH);
     // We can also update our replay protection:
     server.pouch_id = downlink.pouch.id;
+
     if (downlink.id.initiator == POUCH_ROLE_SERVER
         && downlink.id.type == SESSION_ID_TYPE_SEQUENTIAL)
     {
         server.seqnum = downlink.id.value.sequential.seqnum;
+        server.has_seqnum = true;
     }
 
     return 0;

src/saead/session.c

@@ -335,3 +335,30 @@ int session_decrypt_block(struct session *session,
 
     return 0;
 }
+
+int session_match(const struct session *session,
+                  const struct session_id *id,
+                  uint8_t max_block_size_log,
+                  psa_algorithm_t algorithm)
+{
+    if (!pouch_atomic_test_bit(&session->flags, SESSION_VALID))
+    {
+        // session ID and parameters are unset.
+        return 0;
+    }
+
+    if (!session_id_is_equal(id, &session->id))
+    {
+        // not the same session
+        return 0;
+    }
+
+    if (max_block_size_log != session->max_block_size_log || session->algorithm != algorithm)
+    {
+        // parameters have changed.
+        return -EINVAL;
+    }
+
+    // everything matches
+    return 1;
+}

src/saead/session.h

@@ -57,6 +57,7 @@ struct session
     pouch_atomic_t flags;
     psa_algorithm_t algorithm;
     psa_key_id_t key;
+    uint8_t max_block_size_log;
     struct
     {
         pouch_id_t id;
@@ -85,6 +86,18 @@ psa_key_id_t session_key_generate(const struct session_id *id,
 /** End the given session, deleting the session key. */
 void session_end(struct session *session);
 
+/**
+ * Check if the given parameters matches the session.
+ *
+ * @return 0 if the session ID does not match the session.
+ * @return 1 if everything matches
+ * @return -EINVAL if the ID matches, but not the parameters.
+ */
+int session_match(const struct session *session,
+                  const struct session_id *id,
+                  uint8_t max_block_size_log,
+                  psa_algorithm_t algorithm);
+
 int session_pouch_start(struct session *session, pouch_id_t pouch_id);
 
 /** Allocate a block buffer

src/saead/uplink.c

@@ -58,6 +58,7 @@ int saead_uplink_session_start(psa_algorithm_t algorithm, psa_key_id_t private_k
 
     uplink.algorithm = algorithm;
     uplink.pouch.id = 0;
+    uplink.max_block_size_log = MAX_BLOCK_PAYLOAD_SIZE_LOG;
     pouch_atomic_set_bit(&uplink.flags, SESSION_VALID);
 
     return 0;
@@ -135,9 +136,7 @@ bool saead_uplink_session_matches(const struct session_id *id,
                                   uint8_t max_block_size_log,
                                   psa_algorithm_t algorithm)
 {
-    return pouch_atomic_test_bit(&uplink.flags, SESSION_VALID)
-        && session_id_is_equal(id, &uplink.id) && max_block_size_log == MAX_BLOCK_PAYLOAD_SIZE_LOG
-        && uplink.algorithm == algorithm;
+    return session_match(&uplink, id, max_block_size_log, algorithm) == 1;
 }
 
 psa_key_id_t saead_uplink_session_key_copy(psa_key_usage_t usage)

tests/pouch/downlink_session/CMakeLists.txt

@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.20.0)
+
+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+project(downlink_session_test)
+
+# This suite exercises src/saead/downlink.c and src/saead/session.c with
+# real PSA crypto (ECDH + AEAD), so we can't use CONFIG_POUCH_ENCRYPTION_MOCK.
+# Rather than pulling in the whole pouch library (which would need a real
+# device/server X.509 certificate chain via src/cert.c just to reach the
+# session code), we compile only the sources under test plus test doubles
+# for their few dependencies outside the saead module.
+target_sources(app PRIVATE
+  src/downlink_session.c
+  ${ZEPHYR_POUCH_MODULE_DIR}/src/saead/downlink.c
+  ${ZEPHYR_POUCH_MODULE_DIR}/src/saead/session.c
+  ${ZEPHYR_POUCH_MODULE_DIR}/src/buf.c
+)
+
+target_include_directories(app PRIVATE
+  ${ZEPHYR_POUCH_MODULE_DIR}/include
+  ${ZEPHYR_POUCH_MODULE_DIR}/port/include
+  ${ZEPHYR_POUCH_MODULE_DIR}/src
+  ${ZEPHYR_BINARY_DIR}/include/generated/include
+)
+
+# Provide the macros that the bypassed POUCH Kconfig tree would normally
+# generate (see src/Kconfig and the module log config).
+target_compile_definitions(app PRIVATE
+  CONFIG_POUCH_COMMON_LOG_LEVEL=3
+  CONFIG_POUCH_AUTH_TAG_LEN=16
+  CONFIG_POUCH_BLOCK_SIZE=512
+)
+
+# src/saead/uplink.h (pulled in transitively by src/saead/downlink.c) needs
+# the zcbor-generated cddl/header_types.h for struct saead_info. We don't
+# need the generated codec itself (header_decode.c/header_encode.c) since
+# we never touch the pouch header layer here, just the type definitions.
+set(gen_dir ${ZEPHYR_BINARY_DIR}/include/generated)
+file(MAKE_DIRECTORY ${gen_dir})
+
+add_custom_command(
+  OUTPUT
+    ${gen_dir}/header_decode.c
+    ${gen_dir}/header_encode.c
+    ${gen_dir}/include/cddl/header_types.h
+  COMMAND
+    zcbor code -c ${ZEPHYR_POUCH_MODULE_DIR}/src/header.cddl -t pouch_header -sde
+      --include-prefix cddl/ --oc header.c --oh include/cddl/header.h
+  DEPENDS ${ZEPHYR_POUCH_MODULE_DIR}/src/header.cddl
+  WORKING_DIRECTORY ${gen_dir})
+
+add_custom_target(downlink_session_generate_headers
+  DEPENDS ${gen_dir}/include/cddl/header_types.h)
+add_dependencies(app downlink_session_generate_headers)

tests/pouch/downlink_session/prj.conf

@@ -0,0 +1,29 @@
+CONFIG_ZTEST=y
+CONFIG_LOG=y
+
+# src/saead/uplink.h needs the zcbor-generated cddl/header_types.h, which
+# in turn needs zcbor's own runtime headers.
+CONFIG_ZCBOR=y
+CONFIG_ZCBOR_CANONICAL=y
+
+# Real PSA crypto (ECDH key agreement + AEAD), matching what
+# POUCH_ENCRYPTION_SAEAD would select if we pulled in the full POUCH
+# Kconfig tree.
+CONFIG_MBEDTLS=y
+CONFIG_MBEDTLS_PSA_CRYPTO_C=y
+CONFIG_PSA_CRYPTO=y
+CONFIG_MBEDTLS_BASE64_C=y
+
+CONFIG_PSA_WANT_ALG_ECDH=y
+CONFIG_PSA_WANT_ALG_HKDF=y
+CONFIG_PSA_WANT_ALG_SHA_256=y
+CONFIG_PSA_WANT_ECC_SECP_R1_256=y
+CONFIG_PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE=y
+CONFIG_PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_EXPORT=y
+CONFIG_PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY=y
+CONFIG_PSA_WANT_ALG_CHACHA20_POLY1305=y
+CONFIG_PSA_WANT_KEY_TYPE_CHACHA20=y
+
+# Needed by the test fixture to generate the device/server EC keypairs
+# (production devices are provisioned with their key some other way).
+CONFIG_PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE=y