[PATCH 0/3] Support any-salt for padding pss verification

[PATCH 0/3] Support any-salt for padding pss verification

[SESA644425]

This patchset does add support for any salt length in function
padding_pss_verify which currently supports only max salt-length
option. The fix is preceded by two changes to enhance memory
consumption and const-correctness in the area of the patch.

- Hermann Gioja -

SESA644425 (3):
  lib: rsa: Fix const-correctness of rsassa_pss functions
  lib: rsa: Leverage existing data buffer instead of systematic copy
  lib: rsa: Update function padding_pss_verify (any-salt)

 include/image.h      |  2 +-
 include/u-boot/rsa.h |  4 +--
 lib/rsa/rsa-verify.c | 64 +++++++++++++++++++++++---------------------
 3 files changed, 36 insertions(+), 34 deletions(-)


base-commit: 6d3c46ed0e230d999c3f20f7fd4f3a88c03b14ca
-- 
2.25.1

[PATCH 1/3] lib: rsa: Fix const-correctness of rsassa_pss functions

[SESA644425]

Prior to introduction of modifications in rsassa_pss functions
related to padding verification, doing a pass to update
const-correctness in targeted functions to comply with
coding-rules and avoid const-cast

Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
---
Despite checkpath.pl recommendation, it is not possible to use u8 instead
of uint8_t. Proceeding with u8 breaks build with error: unknown type name u8
 include/image.h      |  2 +-
 include/u-boot/rsa.h |  4 ++--
 lib/rsa/rsa-verify.c | 14 +++++++-------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/image.h b/include/image.h
index 97e5f2eb24..bea93118f8 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1291,7 +1291,7 @@ ll_entry_declare(struct crypto_algo, __name, cryptos)
 struct padding_algo {
 	const char *name;
 	int (*verify)(struct image_sign_info *info,
-		      uint8_t *pad, int pad_len,
+		      const uint8_t *pad, int pad_len,
 		      const uint8_t *hash, int hash_len);
 };
 
diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h
index b9634e38d9..085363eb1e 100644
--- a/include/u-boot/rsa.h
+++ b/include/u-boot/rsa.h
@@ -101,11 +101,11 @@ int rsa_verify_with_pkey(struct image_sign_info *info,
 			 const void *hash, uint8_t *sig, uint sig_len);
 
 int padding_pkcs_15_verify(struct image_sign_info *info,
-			   uint8_t *msg, int msg_len,
+			   const uint8_t *msg, int msg_len,
 			   const uint8_t *hash, int hash_len);
 
 int padding_pss_verify(struct image_sign_info *info,
-		       uint8_t *msg, int msg_len,
+		       const uint8_t *msg, int msg_len,
 		       const uint8_t *hash, int hash_len);
 
 #define RSA_DEFAULT_PADDING_NAME		"pkcs-1.5"
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index 112664059c..c2c7248f90 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -73,7 +73,7 @@ static int rsa_verify_padding(const uint8_t *msg, const int pad_len,
 }
 
 int padding_pkcs_15_verify(struct image_sign_info *info,
-			   uint8_t *msg, int msg_len,
+			   const uint8_t *msg, int msg_len,
 			   const uint8_t *hash, int hash_len)
 {
 	struct checksum_algo *checksum = info->checksum;
@@ -125,7 +125,7 @@ static void u32_i2osp(uint32_t val, uint8_t *buf)
  * Return: 0 if the octet string was correctly generated, others on error
  */
 static int mask_generation_function1(struct checksum_algo *checksum,
-				     uint8_t *seed, int seed_len,
+				     const uint8_t *seed, int seed_len,
 				     uint8_t *output, int output_len)
 {
 	struct image_region region[2];
@@ -176,9 +176,9 @@ out:
 }
 
 static int compute_hash_prime(struct checksum_algo *checksum,
-			      uint8_t *pad, int pad_len,
-			      uint8_t *hash, int hash_len,
-			      uint8_t *salt, int salt_len,
+			      const uint8_t *pad, int pad_len,
+			      const uint8_t *hash, int hash_len,
+			      const uint8_t *salt, int salt_len,
 			      uint8_t *hprime)
 {
 	struct image_region region[3];
@@ -215,7 +215,7 @@ out:
  * @hash_len:	Length of the hash
  */
 int padding_pss_verify(struct image_sign_info *info,
-		       uint8_t *msg, int msg_len,
+		       const uint8_t *msg, int msg_len,
 		       const uint8_t *hash, int hash_len)
 {
 	uint8_t *masked_db = NULL;
@@ -287,7 +287,7 @@ int padding_pss_verify(struct image_sign_info *info,
 
 	/* step 12 & 13 */
 	compute_hash_prime(checksum, pad_zero, 8,
-			   (uint8_t *)hash, hash_len,
+			   hash, hash_len,
 			   salt, salt_len, hprime);
 
 	/* step 14 */
-- 
2.25.1

[PATCH 2/3] lib: rsa: Leverage existing data buffer instead of systematic copy

[SESA644425]

Prior to introduction of modifications in rsassa_pss functions
related to padding verification, doing a pass to reduce memory
consumption of function by replacing memory copies of parts of
const buffer by pointers to the original buffer (masked_db and
h are subparts of msg buffer which is declared const, salt is a
subpart of db which is a working buffer, unmodified after being
filled). New pointers scope is limited to the function where
they are declared (not returned to caller by any mean), zeroing
risk of memory fault related to the change.

Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
---
Despite checkpath.pl recommendation, it is not possible to use u8 instead
of uint8_t. Proceeding with u8 breaks build with error: unknown type name u8
 lib/rsa/rsa-verify.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index c2c7248f90..de71234cfb 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -208,6 +208,10 @@ out:
  * saltlen:-1 "set the salt length to the digest length" is currently
  * not supported.
  *
+ * msg is a concatenation of : masked_db + h + 0xbc
+ * Once unmasked, db is a concatenation of : [0x00]* + 0x01 + salt
+ * Length of 0-padding at begin of db depends on salt length.
+ *
  * @info:	Specifies key and FIT information
  * @msg:	byte array of message, len equal to msg_len
  * @msg_len:	Message length
@@ -218,27 +222,25 @@ int padding_pss_verify(struct image_sign_info *info,
 		       const uint8_t *msg, int msg_len,
 		       const uint8_t *hash, int hash_len)
 {
-	uint8_t *masked_db = NULL;
-	int masked_db_len = msg_len - hash_len - 1;
-	uint8_t *h = NULL, *hprime = NULL;
-	int h_len = hash_len;
+	const uint8_t *masked_db = NULL;
 	uint8_t *db_mask = NULL;
-	int db_mask_len = masked_db_len;
-	uint8_t *db = NULL, *salt = NULL;
-	int db_len = masked_db_len, salt_len = msg_len - hash_len - 2;
+	uint8_t *db = NULL;
+	int db_len = msg_len - hash_len - 1;
+	const uint8_t *h = NULL;
+	uint8_t *hprime = NULL;
+	int h_len = hash_len;
+	uint8_t	*salt = NULL;
+	int salt_len = msg_len - hash_len - 2;
 	uint8_t pad_zero[8] = { 0 };
 	int ret, i, leftmost_bits = 1;
 	uint8_t leftmost_mask;
 	struct checksum_algo *checksum = info->checksum;
 
 	/* first, allocate everything */
-	masked_db = malloc(masked_db_len);
-	h = malloc(h_len);
-	db_mask = malloc(db_mask_len);
+	db_mask = malloc(db_len);
 	db = malloc(db_len);
-	salt = malloc(salt_len);
 	hprime = malloc(hash_len);
-	if (!masked_db || !h || !db_mask || !db || !salt || !hprime) {
+	if (!db_mask || !db || !hprime) {
 		printf("%s: can't allocate some buffer\n", __func__);
 		ret = -ENOMEM;
 		goto out;
@@ -252,8 +254,8 @@ int padding_pss_verify(struct image_sign_info *info,
 	}
 
 	/* step 5 */
-	memcpy(masked_db, msg, masked_db_len);
-	memcpy(h, msg + masked_db_len, h_len);
+	masked_db = &msg[0];
+	h = &msg[db_len];
 
 	/* step 6 */
 	leftmost_mask = (0xff >> (8 - leftmost_bits)) << (8 - leftmost_bits);
@@ -265,7 +267,7 @@ int padding_pss_verify(struct image_sign_info *info,
 	}
 
 	/* step 7 */
-	mask_generation_function1(checksum, h, h_len, db_mask, db_mask_len);
+	mask_generation_function1(checksum, h, h_len, db_mask, db_len);
 
 	/* step 8 */
 	for (i = 0; i < db_len; i++)
@@ -283,7 +285,7 @@ int padding_pss_verify(struct image_sign_info *info,
 	}
 
 	/* step 11 */
-	memcpy(salt, &db[1], salt_len);
+	salt = &db[1];
 
 	/* step 12 & 13 */
 	compute_hash_prime(checksum, pad_zero, 8,
@@ -295,11 +297,8 @@ int padding_pss_verify(struct image_sign_info *info,
 
 out:
 	free(hprime);
-	free(salt);
 	free(db);
 	free(db_mask);
-	free(h);
-	free(masked_db);
 
 	return ret;
 }
-- 
2.25.1

[PATCH 3/3] lib: rsa: Update function padding_pss_verify (any-salt)

[SESA644425]

Modify function to support any salt length instead of max
length only. Function now detects salt length by parsing
the content of db buffer. Note that it works with (but is
not limited to) zero-length, digest-length and max-length

Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
---
Despite checkpath.pl recommendation, it is not possible to use u8 instead
of uint8_t. Proceeding with u8 breaks build with error: unknown type name u8
 lib/rsa/rsa-verify.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index de71234cfb..1d95cfbdee 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -204,9 +204,7 @@ out:
 /*
  * padding_pss_verify() - verify the pss padding of a signature
  *
- * Only works with a rsa_pss_saltlen:-2 (default value) right now
- * saltlen:-1 "set the salt length to the digest length" is currently
- * not supported.
+ * Works with any salt length
  *
  * msg is a concatenation of : masked_db + h + 0xbc
  * Once unmasked, db is a concatenation of : [0x00]* + 0x01 + salt
@@ -229,8 +227,8 @@ int padding_pss_verify(struct image_sign_info *info,
 	const uint8_t *h = NULL;
 	uint8_t *hprime = NULL;
 	int h_len = hash_len;
-	uint8_t	*salt = NULL;
-	int salt_len = msg_len - hash_len - 2;
+	uint8_t *db_nopad = NULL, *salt = NULL;
+	int db_padlen, salt_len;
 	uint8_t pad_zero[8] = { 0 };
 	int ret, i, leftmost_bits = 1;
 	uint8_t leftmost_mask;
@@ -277,15 +275,20 @@ int padding_pss_verify(struct image_sign_info *info,
 	db[0] &= 0xff >> leftmost_bits;
 
 	/* step 10 */
-	if (db[0] != 0x01) {
+	db_padlen = 0;
+	while (db[db_padlen] == 0x00 && db_padlen < (db_len - 1))
+		db_padlen++;
+	db_nopad = &db[db_padlen];
+	if (db_nopad[0] != 0x01) {
 		printf("%s: invalid pss padding ", __func__);
-		printf("(leftmost byte of db isn't 0x01)\n");
+		printf("(leftmost byte of db after 0-padding isn't 0x01)\n");
 		ret = EINVAL;
 		goto out;
 	}
 
 	/* step 11 */
-	salt = &db[1];
+	salt_len = db_len - db_padlen - 1;
+	salt = &db_nopad[1];
 
 	/* step 12 & 13 */
 	compute_hash_prime(checksum, pad_zero, 8,
-- 
2.25.1

Re: [PATCH 1/3] lib: rsa: Fix const-correctness of rsassa_pss functions

[Simon Glass]

On Wed, 9 Mar 2022 at 02:28, SESA644425 <giojahermann@gmail.com> wrote:
>
> Prior to introduction of modifications in rsassa_pss functions
> related to padding verification, doing a pass to update
> const-correctness in targeted functions to comply with
> coding-rules and avoid const-cast
>
> Signed-off-by: SESA644425 <gioja.hermann@non.se.com>

Please use your real name.

> ---
> Despite checkpath.pl recommendation, it is not possible to use u8 instead
> of uint8_t. Proceeding with u8 breaks build with error: unknown type name u8
>  include/image.h      |  2 +-
>  include/u-boot/rsa.h |  4 ++--
>  lib/rsa/rsa-verify.c | 14 +++++++-------
>  3 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/3] lib: rsa: Update function padding_pss_verify (any-salt)

[Simon Glass]

On Wed, 9 Mar 2022 at 02:28, SESA644425 <giojahermann@gmail.com> wrote:
>
> Modify function to support any salt length instead of max
> length only. Function now detects salt length by parsing
> the content of db buffer. Note that it works with (but is
> not limited to) zero-length, digest-length and max-length
>
> Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
> ---
> Despite checkpath.pl recommendation, it is not possible to use u8 instead
> of uint8_t. Proceeding with u8 breaks build with error: unknown type name u8
>  lib/rsa/rsa-verify.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Should we add a test for these cases to test_vboot.py ?

Re: [PATCH 2/3] lib: rsa: Leverage existing data buffer instead of systematic copy

[Simon Glass]

Hi,

On Wed, 9 Mar 2022 at 02:28, SESA644425 <giojahermann@gmail.com> wrote:
>
> Prior to introduction of modifications in rsassa_pss functions
> related to padding verification, doing a pass to reduce memory
> consumption of function by replacing memory copies of parts of
> const buffer by pointers to the original buffer (masked_db and
> h are subparts of msg buffer which is declared const, salt is a
> subpart of db which is a working buffer, unmodified after being
> filled). New pointers scope is limited to the function where
> they are declared (not returned to caller by any mean), zeroing
> risk of memory fault related to the change.
>
> Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
> ---
> Despite checkpath.pl recommendation, it is not possible to use u8 instead
> of uint8_t. Proceeding with u8 breaks build with error: unknown type name u8

Yes any code compiled by host tools must use uint8_t/

>  lib/rsa/rsa-verify.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 1/3] lib: rsa: Fix const-correctness of rsassa_pss functions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

On Wed, Mar 09, 2022 at 01:27:15AM -0800, SESA644425 wrote:

> Prior to introduction of modifications in rsassa_pss functions
> related to padding verification, doing a pass to update
> const-correctness in targeted functions to comply with
> coding-rules and avoid const-cast
> 
> Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] lib: rsa: Leverage existing data buffer instead of systematic copy

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Wed, Mar 09, 2022 at 01:27:16AM -0800, SESA644425 wrote:

> Prior to introduction of modifications in rsassa_pss functions
> related to padding verification, doing a pass to reduce memory
> consumption of function by replacing memory copies of parts of
> const buffer by pointers to the original buffer (masked_db and
> h are subparts of msg buffer which is declared const, salt is a
> subpart of db which is a working buffer, unmodified after being
> filled). New pointers scope is limited to the function where
> they are declared (not returned to caller by any mean), zeroing
> risk of memory fault related to the change.
> 
> Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/3] lib: rsa: Update function padding_pss_verify (any-salt)

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

On Wed, Mar 09, 2022 at 01:27:17AM -0800, SESA644425 wrote:

> Modify function to support any salt length instead of max
> length only. Function now detects salt length by parsing
> the content of db buffer. Note that it works with (but is
> not limited to) zero-length, digest-length and max-length
> 
> Signed-off-by: SESA644425 <gioja.hermann@non.se.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]