| Message ID | 1406736017-4554-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hello Simon, On 30-07-14 18:00, Simon Glass wrote: > 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 <jeroen@myspectrum.nl> > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > lib/rsa/rsa-sign.c | 1 + > lib/rsa/rsa-verify.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c > index 83f5e87..6905131 100644 > --- a/lib/rsa/rsa-sign.c > +++ b/lib/rsa/rsa-sign.c > @@ -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); > diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c > index bcb9063..02e3eeb 100644 > --- a/lib/rsa/rsa-verify.c > +++ b/lib/rsa/rsa-verify.c > @@ -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]) 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
Hello Simon, >> { >> - 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 I meant key->len here of course ^ Regards, Jeroen
Hi Jeroen, On 30 July 2014 15:17, Jeroen Hofstee <dasuboot@myspectrum.nl> wrote: > Hello Simon, > > >>> { >>> - 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 > > > 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
On Wed, Jul 30, 2014 at 10:00:17AM -0600, Simon Glass wrote: > 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 <jeroen@myspectrum.nl> > Signed-off-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c index 83f5e87..6905131 100644 --- a/lib/rsa/rsa-sign.c +++ b/lib/rsa/rsa-sign.c @@ -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); diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c index bcb9063..02e3eeb 100644 --- a/lib/rsa/rsa-verify.c +++ b/lib/rsa/rsa-verify.c @@ -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])
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 <jeroen@myspectrum.nl> Signed-off-by: Simon Glass <sjg@chromium.org> --- lib/rsa/rsa-sign.c | 1 + lib/rsa/rsa-verify.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)