On Wed, Aug 28, 2013 at 02:39:35PM +0300, Panu Matilainen wrote:
> No objections to the changes as such, but please split the patch up a bit:
> at the very least separate the interface change from the beecrypt-cleanups,
> but I wouldn't mind the beecrypt cleanups further split up either. That bit
> of extra work pays itself back every time when the inevitable git-bisect
> time cometh...
git-bisect? You don't trust my code? Ts...
Anyway, appended as 4 patches in git format-patch form.
Cheers,
Michael.
--
Michael Schroeder m...@suse.de
SUSE LINUX Products GmbH, GF Jeff Hawn, HRB 16746 AG Nuernberg
main(_){while(_=~getchar())putchar(~_-1/(~(_|32)/13*2-11)*13);}
>From dbc6bbb8184e3766ffa074229fe2b55da3253074 Mon Sep 17 00:00:00 2001
From: Michael Schroeder <m...@suse.de>
Date: Wed, 28 Aug 2013 18:31:47 +0200
Subject: [PATCH 1/4] Check the mpi length before calling setmpi, remove pend
parameter
Simplifies the code a lot. Also check that that there's room for the
mpi len before calling pgpMpiLen().
---
rpmio/digest.h | 3 +--
rpmio/digest_beecrypt.c | 41 ++++++++++++++++++-----------------------
rpmio/digest_nss.c | 45 ++++++++++++++++-----------------------------
rpmio/rpmpgp.c | 16 ++++++++++++----
4 files changed, 47 insertions(+), 58 deletions(-)
diff --git a/rpmio/digest.h b/rpmio/digest.h
index f10f1c8..ee39ee0 100644
--- a/rpmio/digest.h
+++ b/rpmio/digest.h
@@ -5,8 +5,7 @@
typedef struct pgpDigAlg_s * pgpDigAlg;
-typedef int (*setmpifunc)(pgpDigAlg digp,
- int num, const uint8_t *p, const uint8_t *pend);
+typedef int (*setmpifunc)(pgpDigAlg digp, int num, const uint8_t *p);
typedef int (*verifyfunc)(pgpDigAlg pgpkey, pgpDigAlg pgpsig,
uint8_t *hash, size_t hashlen, int hash_algo);
typedef void (*freefunc)(pgpDigAlg digp);
diff --git a/rpmio/digest_beecrypt.c b/rpmio/digest_beecrypt.c
index 7ff7060..ec13716 100644
--- a/rpmio/digest_beecrypt.c
+++ b/rpmio/digest_beecrypt.c
@@ -213,18 +213,18 @@ static inline char * pgpHexCvt(char *t, const byte *s, int nbytes)
return t;
}
-static const char * pgpMpiHex(const byte *p, const byte *pend)
+static const char * pgpMpiHex(const byte *p)
{
static char prbuf[2048];
char *t = prbuf;
int nbytes = pgpMpiLen(p) - 2;
- if (nbytes > 1024 || nbytes > pend - (p + 2))
+ if (nbytes > 1024)
return NULL;
t = pgpHexCvt(t, p+2, nbytes);
return prbuf;
}
-static int pgpHexSet(int lbits, mpnumber * mpn, const byte * p, const byte * pend)
+static int pgpHexSet(int lbits, mpnumber * mpn, const byte * p)
{
unsigned int mbits = pgpMpiBits(p);
unsigned int nbits;
@@ -238,7 +238,7 @@ static int pgpHexSet(int lbits, mpnumber * mpn, const byte * p, const byte * pen
ix = 2 * ((nbits - mbits) >> 3);
if (ix > 0) memset(t, (int)'0', ix);
- strcpy(t+ix, pgpMpiHex(p, pend));
+ strcpy(t+ix, pgpMpiHex(p));
(void) mpnsethex(mpn, t);
t = _free(t);
return 0;
@@ -269,8 +269,7 @@ struct pgpDigKeyRSA_s {
rsapk rsa_pk;
};
-static int pgpSetSigMpiRSA(pgpDigAlg pgpsig, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetSigMpiRSA(pgpDigAlg pgpsig, int num, const uint8_t *p)
{
struct pgpDigSigRSA_s *sig = pgpsig->data;
int rc = 1;
@@ -278,15 +277,14 @@ static int pgpSetSigMpiRSA(pgpDigAlg pgpsig, int num,
switch (num) {
case 0:
sig = pgpsig->data = xcalloc(1, sizeof(*sig));
- (void) mpnsethex(&sig->c, pgpMpiHex(p, pend));
+ (void) mpnsethex(&sig->c, pgpMpiHex(p));
rc = 0;
break;
}
return rc;
}
-static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p)
{
struct pgpDigKeyRSA_s *key = pgpkey->data;
int rc = 1;
@@ -295,11 +293,11 @@ static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num,
key = pgpkey->data = xcalloc(1, sizeof(*key));
switch (num) {
case 0:
- (void) mpbsethex(&key->rsa_pk.n, pgpMpiHex(p, pend));
+ (void) mpbsethex(&key->rsa_pk.n, pgpMpiHex(p));
rc = 0;
break;
case 1:
- (void) mpnsethex(&key->rsa_pk.e, pgpMpiHex(p, pend));
+ (void) mpnsethex(&key->rsa_pk.e, pgpMpiHex(p));
rc = 0;
break;
}
@@ -386,8 +384,7 @@ struct pgpDigKeyDSA_s {
mpnumber y;
};
-static int pgpSetSigMpiDSA(pgpDigAlg pgpsig, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetSigMpiDSA(pgpDigAlg pgpsig, int num, const uint8_t *p)
{
struct pgpDigSigDSA_s *sig = pgpsig->data;
int rc = 1;
@@ -395,17 +392,16 @@ static int pgpSetSigMpiDSA(pgpDigAlg pgpsig, int num,
switch (num) {
case 0:
sig = pgpsig->data = xcalloc(1, sizeof(*sig));
- rc = pgpHexSet(160, &sig->r, p, pend);
+ rc = pgpHexSet(160, &sig->r, p);
break;
case 1:
- rc = pgpHexSet(160, &sig->s, p, pend);
+ rc = pgpHexSet(160, &sig->s, p);
break;
}
return rc;
}
-static int pgpSetKeyMpiDSA(pgpDigAlg pgpkey, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetKeyMpiDSA(pgpDigAlg pgpkey, int num, const uint8_t *p)
{
struct pgpDigKeyDSA_s *key = pgpkey->data;
int rc = 1;
@@ -415,19 +411,19 @@ static int pgpSetKeyMpiDSA(pgpDigAlg pgpkey, int num,
switch (num) {
case 0:
- mpbsethex(&key->p, pgpMpiHex(p, pend));
+ mpbsethex(&key->p, pgpMpiHex(p));
rc = 0;
break;
case 1:
- mpbsethex(&key->q, pgpMpiHex(p, pend));
+ mpbsethex(&key->q, pgpMpiHex(p));
rc = 0;
break;
case 2:
- mpnsethex(&key->g, pgpMpiHex(p, pend));
+ mpnsethex(&key->g, pgpMpiHex(p));
rc = 0;
break;
case 3:
- mpnsethex(&key->y, pgpMpiHex(p, pend));
+ mpnsethex(&key->y, pgpMpiHex(p));
rc = 0;
break;
}
@@ -450,8 +446,7 @@ static int pgpVerifySigDSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig, uint8_t *hash, si
return rc;
}
-static int pgpSetMpiNULL(pgpDigAlg pgpkey, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetMpiNULL(pgpDigAlg pgpkey, int num, const uint8_t *p)
{
return 1;
}
diff --git a/rpmio/digest_nss.c b/rpmio/digest_nss.c
index 777615a..7a0dd7d 100644
--- a/rpmio/digest_nss.c
+++ b/rpmio/digest_nss.c
@@ -224,8 +224,7 @@ static SECOidTag getHashAlg(unsigned int hashalgo)
return SEC_OID_UNKNOWN;
}
-static int pgpMpiSet(unsigned int lbits, uint8_t *dest,
- const uint8_t * p, const uint8_t * pend)
+static int pgpMpiSet(unsigned int lbits, uint8_t *dest, const uint8_t * p)
{
unsigned int mbits = pgpMpiBits(p);
unsigned int nbits;
@@ -233,9 +232,6 @@ static int pgpMpiSet(unsigned int lbits, uint8_t *dest,
uint8_t *t = dest;
unsigned int ix;
- if ((p + ((mbits+7) >> 3)) > pend)
- return 1;
-
if (mbits > lbits)
return 1;
@@ -250,14 +246,10 @@ static int pgpMpiSet(unsigned int lbits, uint8_t *dest,
return 0;
}
-static SECItem *pgpMpiItem(PRArenaPool *arena, SECItem *item,
- const uint8_t *p, const uint8_t *pend)
+static SECItem *pgpMpiItem(PRArenaPool *arena, SECItem *item, const uint8_t *p)
{
size_t nbytes = pgpMpiLen(p)-2;
- if (p + nbytes + 2 > pend)
- return NULL;
-
if (item == NULL) {
if ((item=SECITEM_AllocItem(arena, item, nbytes)) == NULL)
return item;
@@ -310,8 +302,7 @@ static SECKEYPublicKey *pgpNewPublicKey(KeyType type)
#define DSA_MIN_Q_BITS DSA_Q_BITS
#endif
-static int pgpSetSigMpiDSA(pgpDigAlg pgpsig, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetSigMpiDSA(pgpDigAlg pgpsig, int num, const uint8_t *p)
{
SECItem *sig = pgpsig->data;
unsigned int subprlen = (num == 0) ? pgpMpiLen(p) - 2 : sig->len / 2;
@@ -325,11 +316,11 @@ static int pgpSetSigMpiDSA(pgpDigAlg pgpsig, int num,
sig = pgpsig->data = SECITEM_AllocItem(NULL, NULL, siglen);
if (sig) {
memset(sig->data, 0, siglen);
- rc = pgpMpiSet(qbits, sig->data, p, pend);
+ rc = pgpMpiSet(qbits, sig->data, p);
}
break;
case 1:
- if (sig && pgpMpiSet(qbits, sig->data+subprlen, p, pend) == 0) {
+ if (sig && pgpMpiSet(qbits, sig->data+subprlen, p) == 0) {
SECItem *signew = SECITEM_AllocItem(NULL, NULL, 0);
if (signew == NULL)
break;
@@ -345,8 +336,7 @@ static int pgpSetSigMpiDSA(pgpDigAlg pgpsig, int num,
return rc;
}
-static int pgpSetKeyMpiDSA(pgpDigAlg pgpkey, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetKeyMpiDSA(pgpDigAlg pgpkey, int num, const uint8_t *p)
{
SECItem *mpi = NULL;
SECKEYPublicKey *key = pgpkey->data;
@@ -357,16 +347,16 @@ static int pgpSetKeyMpiDSA(pgpDigAlg pgpkey, int num,
if (key) {
switch (num) {
case 0:
- mpi = pgpMpiItem(key->arena, &key->u.dsa.params.prime, p, pend);
+ mpi = pgpMpiItem(key->arena, &key->u.dsa.params.prime, p);
break;
case 1:
- mpi = pgpMpiItem(key->arena, &key->u.dsa.params.subPrime, p, pend);
+ mpi = pgpMpiItem(key->arena, &key->u.dsa.params.subPrime, p);
break;
case 2:
- mpi = pgpMpiItem(key->arena, &key->u.dsa.params.base, p, pend);
+ mpi = pgpMpiItem(key->arena, &key->u.dsa.params.base, p);
break;
case 3:
- mpi = pgpMpiItem(key->arena, &key->u.dsa.publicValue, p, pend);
+ mpi = pgpMpiItem(key->arena, &key->u.dsa.publicValue, p);
break;
}
}
@@ -391,21 +381,19 @@ static int pgpVerifySigDSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig,
return (rc != SECSuccess);
}
-static int pgpSetSigMpiRSA(pgpDigAlg pgpsig, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetSigMpiRSA(pgpDigAlg pgpsig, int num, const uint8_t *p)
{
SECItem *sigitem = NULL;
if (num == 0) {
- sigitem = pgpMpiItem(NULL, pgpsig->data, p, pend);
+ sigitem = pgpMpiItem(NULL, pgpsig->data, p);
if (sigitem)
pgpsig->data = sigitem;
}
return (sigitem == NULL);
}
-static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p)
{
SECItem *kitem = NULL;
SECKEYPublicKey *key = pgpkey->data;
@@ -416,10 +404,10 @@ static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num,
if (key) {
switch (num) {
case 0:
- kitem = pgpMpiItem(key->arena, &key->u.rsa.modulus, p, pend);
+ kitem = pgpMpiItem(key->arena, &key->u.rsa.modulus, p);
break;
case 1:
- kitem = pgpMpiItem(key->arena, &key->u.rsa.publicExponent, p, pend);
+ kitem = pgpMpiItem(key->arena, &key->u.rsa.publicExponent, p);
break;
}
}
@@ -474,8 +462,7 @@ static void pgpFreeKeyRSADSA(pgpDigAlg ka)
ka->data = NULL;
}
-static int pgpSetMpiNULL(pgpDigAlg pgpkey, int num,
- const uint8_t *p, const uint8_t *pend)
+static int pgpSetMpiNULL(pgpDigAlg pgpkey, int num, const uint8_t *p)
{
return 1;
}
diff --git a/rpmio/rpmpgp.c b/rpmio/rpmpgp.c
index e70cf70..5893222 100644
--- a/rpmio/rpmpgp.c
+++ b/rpmio/rpmpgp.c
@@ -499,11 +499,15 @@ static int pgpPrtSigParams(pgpTag tag, uint8_t pubkey_algo, uint8_t sigtype,
int i;
pgpDigAlg sigalg = pgpSignatureNew(pubkey_algo);
- for (i = 0; p < pend && i < sigalg->mpis; i++, p += pgpMpiLen(p)) {
+ for (i = 0; i < sigalg->mpis && p + 2 <= pend; i++) {
+ int mpil = pgpMpiLen(p);
+ if (p + mpil > pend)
+ break;
if (sigtype == PGPSIGTYPE_BINARY || sigtype == PGPSIGTYPE_TEXT) {
- if (sigalg->setmpi(sigalg, i, p, pend))
+ if (sigalg->setmpi(sigalg, i, p))
break;
}
+ p += mpil;
}
/* Does the size and number of MPI's match our expectations? */
@@ -650,9 +654,13 @@ static int pgpPrtPubkeyParams(uint8_t pubkey_algo,
int i;
pgpDigAlg keyalg = pgpPubkeyNew(pubkey_algo);
- for (i = 0; p < pend && i < keyalg->mpis; i++, p += pgpMpiLen(p)) {
- if (keyalg->setmpi(keyalg, i, p, pend))
+ for (i = 0; i < keyalg->mpis && p + 2 <= pend; i++) {
+ int mpil = pgpMpiLen(p);
+ if (p + mpil > pend)
+ break;
+ if (keyalg->setmpi(keyalg, i, p))
break;
+ p += mpil;
}
/* Does the size and number of MPI's match our expectations? */
--
1.8.1.4
>From 6afcb6319988a1318c00bb9fe164105434fb29b6 Mon Sep 17 00:00:00 2001
From: Michael Schroeder <m...@suse.de>
Date: Wed, 28 Aug 2013 18:38:45 +0200
Subject: [PATCH 2/4] digest_beecrypt: plug memory leaks
Free the beecrypt numbers when freeing the key/sig data.
---
rpmio/digest_beecrypt.c | 66 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 48 insertions(+), 18 deletions(-)
diff --git a/rpmio/digest_beecrypt.c b/rpmio/digest_beecrypt.c
index ec13716..f0743b0 100644
--- a/rpmio/digest_beecrypt.c
+++ b/rpmio/digest_beecrypt.c
@@ -244,20 +244,6 @@ static int pgpHexSet(int lbits, mpnumber * mpn, const byte * p)
return 0;
}
-static void pgpFreeSigRSADSA(pgpDigAlg sa)
-{
- if (sa->data)
- free(sa->data);
- sa->data = 0;
-}
-
-static void pgpFreeKeyRSADSA(pgpDigAlg sa)
-{
- if (sa->data)
- free(sa->data);
- sa->data = 0;
-}
-
/****************************** RSA **************************************/
@@ -369,6 +355,25 @@ static int pgpVerifySigRSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig, uint8_t *hash, si
return rc;
}
+static void pgpFreeSigRSA(pgpDigAlg pgpsig)
+{
+ struct pgpDigSigRSA_s *sig = pgpsig->data;
+ if (sig) {
+ mpnfree(&sig->c);
+ pgpsig->data = _free(sig);
+ }
+}
+
+static void pgpFreeKeyRSA(pgpDigAlg pgpkey)
+{
+ struct pgpDigKeyRSA_s *key = pgpkey->data;
+ if (key) {
+ mpbfree(&key->rsa_pk.n);
+ mpnfree(&key->rsa_pk.e);
+ pgpkey->data = _free(key);
+ }
+}
+
/****************************** DSA **************************************/
@@ -446,6 +451,31 @@ static int pgpVerifySigDSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig, uint8_t *hash, si
return rc;
}
+static void pgpFreeSigDSA(pgpDigAlg pgpsig)
+{
+ struct pgpDigSigDSA_s *sig = pgpsig->data;
+ if (sig) {
+ mpnfree(&sig->r);
+ mpnfree(&sig->s);
+ pgpsig->data = _free(sig);
+ }
+}
+
+static void pgpFreeKeyDSA(pgpDigAlg pgpkey)
+{
+ struct pgpDigKeyDSA_s *key = pgpkey->data;
+ if (key) {
+ mpbfree(&key->p);
+ mpbfree(&key->q);
+ mpnfree(&key->g);
+ mpnfree(&key->y);
+ pgpkey->data = _free(key);
+ }
+}
+
+
+/****************************** NULL **************************************/
+
static int pgpSetMpiNULL(pgpDigAlg pgpkey, int num, const uint8_t *p)
{
return 1;
@@ -464,12 +494,12 @@ pgpDigAlg pgpPubkeyNew(int algo)
switch (algo) {
case PGPPUBKEYALGO_RSA:
ka->setmpi = pgpSetKeyMpiRSA;
- ka->free = pgpFreeKeyRSADSA;
+ ka->free = pgpFreeKeyRSA;
ka->mpis = 2;
break;
case PGPPUBKEYALGO_DSA:
ka->setmpi = pgpSetKeyMpiDSA;
- ka->free = pgpFreeKeyRSADSA;
+ ka->free = pgpFreeKeyDSA;
ka->mpis = 4;
break;
default:
@@ -490,13 +520,13 @@ pgpDigAlg pgpSignatureNew(int algo)
switch (algo) {
case PGPPUBKEYALGO_RSA:
sa->setmpi = pgpSetSigMpiRSA;
- sa->free = pgpFreeSigRSADSA;
+ sa->free = pgpFreeSigRSA;
sa->verify = pgpVerifySigRSA;
sa->mpis = 1;
break;
case PGPPUBKEYALGO_DSA:
sa->setmpi = pgpSetSigMpiDSA;
- sa->free = pgpFreeSigRSADSA;
+ sa->free = pgpFreeSigDSA;
sa->verify = pgpVerifySigDSA;
sa->mpis = 2;
break;
--
1.8.1.4
_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
