Skip to contentSkip to conversation

rsa: Fix two errors in the implementation

PATCH In review Simon Glass 4 participants 5 messages Jul 30, 2014 – Aug 10, 2014

lore
Share & tools
lore

Simon Glass

author
#0 lore raw

PATCH 1/1 rsa: Fix two errors in the implementation

1. Failure to set the return code correctly
2. Failure to detect the loop end condition when the value is equal to the modulus.

Reported-by Jeroen Hofstee <[email protected]>
Signed-off-by Simon Glass <[email protected]>

---

 lib/rsa/rsa-sign.c   | 1 +
 lib/rsa/rsa-verify.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)
lib/rsa/rsa-sign.c +1 -0
index 83f5e87..6905131 100644
@@ -76,6 +76,7 @@ static int rsa_get_pub_key(const char *keydir, const char *name, RSA **rsap)
 	rsa = EVP_PKEY_get1_RSA(key);
 	if (!rsa) {
 		rsa_err("Couldn't convert to a RSA style key");
+		ret = -EINVAL;
 		goto err_rsa;
 	}
 	fclose(f);
lib/rsa/rsa-verify.c +2 -2
index bcb9063..02e3eeb 100644
@@ -54,9 +54,9 @@ static void subtract_modulus(const struct rsa_public_key *key, uint32_t num[])
 static int greater_equal_modulus(const struct rsa_public_key *key,
 				 uint32_t num[])
 {
-	uint32_t i;
+	int i;
 
-	for (i = key->len - 1; i >= 0; i--) {
+	for (i = (int)key->len - 1; i >= 0; i--) {
 		if (num[i] < key->modulus[i])
 			return 0;
 		if (num[i] > key->modulus[i])
Signature · 1 line
2.0.0.526.g5318336
#1 lore raw

PATCH 1/1 rsa: Fix two errors in the implementation

Hello Simon,

Quoted 30 lines · Simon Glass

1. Failure to set the return code correctly
2. Failure to detect the loop end condition when the value is equal to the modulus.

Reported-by Jeroen Hofstee <[email protected]>
Signed-off-by Simon Glass <[email protected]>

---

  lib/rsa/rsa-sign.c   | 1 +
  lib/rsa/rsa-verify.c | 4 ++--
  2 files changed, 3 insertions(+), 2 deletions(-)
lib/rsa/rsa-sign.c +1 -0
index 83f5e87..6905131 100644
@@ -76,6 +76,7 @@ static int rsa_get_pub_key(const char *keydir, const char *name, RSA **rsap)
  	rsa = EVP_PKEY_get1_RSA(key);
  	if (!rsa) {
  		rsa_err("Couldn't convert to a RSA style key");
+		ret = -EINVAL;
  		goto err_rsa;
  	}
  	fclose(f);
lib/rsa/rsa-verify.c +0 -0
index bcb9063..02e3eeb 100644
@@ -54,9 +54,9 @@ static void subtract_modulus(const struct rsa_public_key *key, uint32_t num[])
  static int greater_equal_modulus(const struct rsa_public_key *key,

lib/rsa/rsa-verify.c static void subtract_modulus(const struct rsa_public_key *key, uint32_t num[]) · line 62

Quoted 10 lines
lib/rsa/rsa-verify.c +2 -2
  				 uint32_t num[])
  {
-	uint32_t i;
+	int i;
 
-	for (i = key->len - 1; i >= 0; i--) {
+	for (i = (int)key->len - 1; i >= 0; i--) {
  		if (num[i] < key->modulus[i])
  			return 0;
  		if (num[i] > key->modulus[i])

I did indeed not post a patch, since I do not know how this code is used and how critical it is. And I still haven't bothered to look it up.

So just a general comment, which might not make any sense at all for the actual usage. If num can somehow be controlled by an evil source, passing a large enough value or 0 now causes this function to return equal. I have no idea if this causes any practical issue.

Warnings / error wise, this seems fine, thanks!

Regards,
Jeroen

#2 lore raw

PATCH 1/1 rsa: Fix two errors in the implementation

Hello Simon,

Quoted 9 lines

lib/rsa/rsa-verify.c static void subtract_modulus(const struct rsa_public_key *key, uint32_t num[]) · line 62

Quoted 8 lines
  {
lib/rsa/rsa-verify.c +2 -1
-    uint32_t i;
+    int i;
  -    for (i = key->len - 1; i >= 0; i--) {
+    for (i = (int)key->len - 1; i >= 0; i--) {
          if (num[i] < key->modulus[i])
              return 0;
          if (num[i] > key->modulus[i])
Quoted 6 lines

I did indeed not post a patch, since I do not know how this code is used and how critical it is. And I still haven't bothered to look it up.

So just a general comment, which might not make any sense at all for the actual usage. If num can somehow be controlled by an

I meant key->len here of course ^

Regards,
Jeroen

author
#3 lore raw

PATCH 1/1 rsa: Fix two errors in the implementation

Hi Jeroen,

Quoted 17 lines · Jeroen Hofstee

Hello Simon,

Quoted 14 lines

lib/rsa/rsa-verify.c static void subtract_modulus(const struct rsa_public_key *key, uint32_t num[]) · line 62

Quoted 8 lines
  {
lib/rsa/rsa-verify.c +2 -1
-    uint32_t i;
+    int i;
  -    for (i = key->len - 1; i >= 0; i--) {
+    for (i = (int)key->len - 1; i >= 0; i--) {
          if (num[i] < key->modulus[i])
              return 0;
          if (num[i] > key->modulus[i])

I did indeed not post a patch, since I do not know how this code is used and how critical it is. And I still haven't bothered to look it up.

Quoted 5 lines
Quoted 2 lines

So just a general comment, which might not make any sense at all for the actual usage. If num can somehow be controlled by an

I meant key->len here of course ^

OK I see. Well the key length is range-checked in pow_mod(). If a key length of 0 were used, it would not be a valid signature - this function might do strange things. But the key length has to match the public key, so something like that would juts cause a verification failure higher up the stack.

Regards,
Simon

#4 lore raw
In reply to Simon Glass
Quoted 6 lines · Simon Glass

1. Failure to set the return code correctly
2. Failure to detect the loop end condition when the value is equal to the modulus.

Reported-by Jeroen Hofstee <[email protected]>
Signed-off-by Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140810/e1849bc7/attachment.pgp>