diff mbox series

core: accept 128 and 192 bit per-image aes-key

Message ID BE0P281MB000219D82F0D0B68B1E1A8CE8C822@BE0P281MB0002.DEUP281.PROD.OUTLOOK.COM
State New
Headers show
Series core: accept 128 and 192 bit per-image aes-key | expand

Commit Message

Raimar Sandner Sept. 23, 2026, 9:15 a.m. UTC 
copyfile() passes the size of the destination buffer to ascii_to_bin
which only converts when the hex string has exactly twice that length.
The buffer is AES_256_KEY_LEN, so a per-image "aes-key" in
sw-description is rejected with "Invalid image aes-key" for every key
that is not 256 bit, although the line below already computes keylen
from the string. The global key file has always accepted all three
lengths, so the two ways of supplying a key disagree.

Derive the length from the string, accept it only if it is one of the
three AES key lengths, and convert exactly that many bytes.

Signed-off-by: Lukas Knöpfle <lukas.knoepfle@sick.de>
Co-developed-by: Raimar Sandner <raimar.sandner@sick.de>
Signed-off-by: Raimar Sandner <raimar.sandner@sick.de>
---
 core/cpio_utils.c | 8 ++++++--
 include/util.h    | 7 +++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

Comments

Stefano Babic Oct. 5, 2026, 8:05 a.m. UTC | #1 
Hi Raimar,

On 9/23/26 11:15, 'Raimar Sandner' via swupdate wrote:
> copyfile() passes the size of the destination buffer to ascii_to_bin
> which only converts when the hex string has exactly twice that length.
> The buffer is AES_256_KEY_LEN, so a per-image "aes-key" in
> sw-description is rejected with "Invalid image aes-key" for every key
> that is not 256 bit, although the line below already computes keylen
> from the string. The global key file has always accepted all three
> lengths, so the two ways of supplying a key disagree.
> 
> Derive the length from the string, accept it only if it is one of the
> three AES key lengths, and convert exactly that many bytes.
> 

This was to implicitly force to use the strongest key length, and 
officially only AES-256 is supported. Probably not clear in doc, but see 
source/swupdate-ipc-interface.rst.

Are there any reason to allow weaker length ?

Anyway, if this becomes supported, docs must be adjusted as well, and 
also be sure that IPC is working,  int swupdate_set_aes(char *key, char 
*ivt).

Best regards,
Stefano

> Signed-off-by: Lukas Knöpfle <lukas.knoepfle@sick.de>
> Co-developed-by: Raimar Sandner <raimar.sandner@sick.de>
> Signed-off-by: Raimar Sandner <raimar.sandner@sick.de>
> ---
>   core/cpio_utils.c | 8 ++++++--
>   include/util.h    | 7 +++++++
>   2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/core/cpio_utils.c b/core/cpio_utils.c
> index 9faa8019..4802c390 100644
> --- a/core/cpio_utils.c
> +++ b/core/cpio_utils.c
> @@ -706,12 +706,16 @@ int copyfile(struct swupdate_copy *args)
>   			ivt = get_aes_ivt();
>   
>   		if (args->imgaes && strlen(args->imgaes)) {
> -				if (!is_hex_str(args->imgaes) || ascii_to_bin(aesbuf, sizeof(aesbuf), args->imgaes)) {
> +				size_t imgkeylen = strlen(args->imgaes) / 2;
> +
> +				if (!is_valid_aes_keylen(imgkeylen) ||
> +				    !is_hex_str(args->imgaes) ||
> +				    ascii_to_bin(aesbuf, imgkeylen, args->imgaes)) {
>   					ERROR("Invalid image aes-key");
>   					return -EINVAL;
>   				}
>   				aes_key = aesbuf;
> -				keylen = strlen(args->imgaes) / 2;
> +				keylen = imgkeylen;
>   		} else {
>   			/*
>   			 * Only fall back to the default decryption key if the requested cipher matches.
> diff --git a/include/util.h b/include/util.h
> index 7007d357..4255a126 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -34,6 +34,13 @@
>   #define AES_192_KEY_LEN	24
>   #define AES_256_KEY_LEN	32
>   
> +static inline bool is_valid_aes_keylen(size_t keylen)
> +{
> +	return keylen == AES_128_KEY_LEN ||
> +	       keylen == AES_192_KEY_LEN ||
> +	       keylen == AES_256_KEY_LEN;
> +}
> +
>   #define HWID_REGEXP_PREFIX	"#RE:"
>   
>   #define BOOTVAR_TRANSACTION "recovery_status"
Raimar Sandner Oct. 5, 2026, 4:03 p.m. UTC | #2 
copyfile() passes the size of the destination buffer to ascii_to_bin
which only converts when the hex string has exactly twice that length.
The buffer is AES_256_KEY_LEN, so a per-image "aes-key" in
sw-description is rejected with "Invalid image aes-key" for every key
that is not 256 bit, although the line below already computes keylen
from the string. The global key file has always accepted all three
lengths, so the two ways of supplying a key disagree.

Derive the length from the string, accept it only if it is one of the
three AES key lengths, and convert exactly that many bytes.

This patch brings the per-image aes-key handling in line with the
documentation (sw-description.rst) and the global key file. Note that
the IPC interface intentionally only accepts 256 bit keys, as documented
in swupdate-ipc-interface.rst. The patch makes sure this limitation is
still correctly enforced.

Signed-off-by: Lukas Knöpfle <lukas.knoepfle@sick.de>
Co-developed-by: Raimar Sandner <raimar.sandner@sick.de>
Signed-off-by: Raimar Sandner <raimar.sandner@sick.de>
---

v2: revised commit message clarifying the IPC vs aes-key behavior, also
fix the overly permissive length check in swupdate_set_aes of
network_ipc-if.c

 core/cpio_utils.c    | 8 ++++++--
 include/util.h       | 7 +++++++
 ipc/network_ipc-if.c | 2 +-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/core/cpio_utils.c b/core/cpio_utils.c
index ff417d7e..e3aed763 100644
--- a/core/cpio_utils.c
+++ b/core/cpio_utils.c
@@ -697,12 +697,16 @@ int copyfile(struct swupdate_copy *args)
 			ivt = get_aes_ivt();

 		if (args->imgaes && strlen(args->imgaes)) {
-				if (!is_hex_str(args->imgaes) || ascii_to_bin(aesbuf, sizeof(aesbuf), args->imgaes)) {
+				size_t imgkeylen = strlen(args->imgaes) / 2;
+
+				if (!is_valid_aes_keylen(imgkeylen) ||
+				    !is_hex_str(args->imgaes) ||
+				    ascii_to_bin(aesbuf, imgkeylen, args->imgaes)) {
 					ERROR("Invalid image aes-key");
 					return -EINVAL;
 				}
 				aes_key = aesbuf;
-				keylen = strlen(args->imgaes) / 2;
+				keylen = imgkeylen;
 		} else {
 			/*
 			 * Only fall back to the default decryption key if the requested cipher matches.
diff --git a/include/util.h b/include/util.h
index 7007d357..4255a126 100644
--- a/include/util.h
+++ b/include/util.h
@@ -34,6 +34,13 @@
 #define AES_192_KEY_LEN	24
 #define AES_256_KEY_LEN	32

+static inline bool is_valid_aes_keylen(size_t keylen)
+{
+	return keylen == AES_128_KEY_LEN ||
+	       keylen == AES_192_KEY_LEN ||
+	       keylen == AES_256_KEY_LEN;
+}
+
 #define HWID_REGEXP_PREFIX	"#RE:"

 #define BOOTVAR_TRANSACTION "recovery_status"
diff --git a/ipc/network_ipc-if.c b/ipc/network_ipc-if.c
index 7c209ad4..1b43e560 100644
--- a/ipc/network_ipc-if.c
+++ b/ipc/network_ipc-if.c
@@ -336,7 +336,7 @@ int swupdate_set_aes(char *key, char *ivt)

 	if (!key || !ivt)
 		return -EINVAL;
-	if (strlen(key) != 64 && strlen(ivt) != 32)
+	if (strlen(key) != 64 || strlen(ivt) != 32)
 		return -EINVAL;

 	memset(&msg, 0, sizeof(msg));
--
2.43.0
Raimar Sandner Oct. 5, 2026, 4:06 p.m. UTC | #3 
Dear Stefano,

> Are there any reason to allow weaker length ?

Not for us. If you don't see any need for the smaller key sizes (e.g., low-end
hardware) then only supporting AES256 is totally fine. As of today, however,
there is a discrepancy between using the global key file (which supports the
smaller keys) and the per-image keys. To avoid misunderstandings, this is
probably worth a comment in encrypted_images.rst. This file claims "you may use
other key lengths as well" unconditionally. Also sw-description.rst documents
16, 24 or 32 characters as valid hex-string lengths for the aes-key parameter
in the manifest.

The issue came up when implementing the AEAD (AES-GCM) support and testing it
with per-image keys. We didn't realize the limitation on per-image keys was
intentional, our motivation was to submit this as independent "bug fix" before
the AEAD implementation patches land.

Applying the revised patch I just sent would bring the implementation in line
with the sw-description documentation, and the IPC limitation to only support
256-bit is now correctly checked in network_ipc-if.c, consistent with the
documentation in swupdate-ipc-interface.rst. Not applying this patch and
keeping the behavior as is also works for us.

Either way, here is a quick update on the status of AEAD encryption for
SWUpdate: the student project is feature-complete with solid test coverage. A
short list of blockers remains, which we're closing before the patches go out.
There is still some polishing left to do on the documentation, and we will
conduct a full internal review before sending the patches.

Unfortunately Lukas now has other priorities in his curriculum, and a change in
my availability means I'll be out of office for the next four months. I'm still
fully committed to finishing the project with an upstream contribution, but the
patches will arrive no earlier than February 2027.

Thanks for your support so far!
Raimar

Mit freundlichen Grüßen / Best regards

i.A. Dr. Raimar Sandner
Expert in Industrial Cybersecurity  |  Autonomous Perception
diff mbox series

Patch

diff --git a/core/cpio_utils.c b/core/cpio_utils.c
index 9faa8019..4802c390 100644
--- a/core/cpio_utils.c
+++ b/core/cpio_utils.c
@@ -706,12 +706,16 @@  int copyfile(struct swupdate_copy *args)
 			ivt = get_aes_ivt();
 
 		if (args->imgaes && strlen(args->imgaes)) {
-				if (!is_hex_str(args->imgaes) || ascii_to_bin(aesbuf, sizeof(aesbuf), args->imgaes)) {
+				size_t imgkeylen = strlen(args->imgaes) / 2;
+
+				if (!is_valid_aes_keylen(imgkeylen) ||
+				    !is_hex_str(args->imgaes) ||
+				    ascii_to_bin(aesbuf, imgkeylen, args->imgaes)) {
 					ERROR("Invalid image aes-key");
 					return -EINVAL;
 				}
 				aes_key = aesbuf;
-				keylen = strlen(args->imgaes) / 2;
+				keylen = imgkeylen;
 		} else {
 			/*
 			 * Only fall back to the default decryption key if the requested cipher matches.
diff --git a/include/util.h b/include/util.h
index 7007d357..4255a126 100644
--- a/include/util.h
+++ b/include/util.h
@@ -34,6 +34,13 @@ 
 #define AES_192_KEY_LEN	24
 #define AES_256_KEY_LEN	32
 
+static inline bool is_valid_aes_keylen(size_t keylen)
+{
+	return keylen == AES_128_KEY_LEN ||
+	       keylen == AES_192_KEY_LEN ||
+	       keylen == AES_256_KEY_LEN;
+}
+
 #define HWID_REGEXP_PREFIX	"#RE:"
 
 #define BOOTVAR_TRANSACTION "recovery_status"