[Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)

[Tomáš Mráz]

t8m commented on this pull request.



> +int DSA_SIG_set0(DSA_SIG *sig, BIGNUM *r, BIGNUM *s)
+{
+    if (!sig) return 0;
+
+    if (r) {
+        sig->r = r;
+    }
+
+    if (s) {
+        sig->s = s;
+    }
+
+    return 1;
+}
+#endif /* HAVE_DSA_SIG_SET0 */
+

In above functions you do not release previous component BIGNUMs before assigning new values. You should add that.

> +    struct pgpDigKeyRSA_s *key = pgpkey->data;
+
+    if(!key) {
+        key = pgpkey->data = OPENSSL_secure_zalloc(sizeof(*key));
+        if (!key) return 1;
+    }
+
+    switch(num) {
+    case 0:
+        /* Modulus */
+        key->nbytes = mlen;
+        /* Create a BIGNUM from the pointer.
+           Note: this assumes big-endian data. This
+           could be a bug on little-endian systems. */
+        key->n = BN_bin2bn(p+2, mlen, NULL);
+        if (!key->n) return 1;

You do not handle a theoretical double call with the same num on the same pgpkey - it will leak memory. I do not know if this can happen (for example with malformed data) though.

> +        EVP_PKEY_free(key->evp_pkey);
+        key->evp_pkey = NULL;
+        RSA_free(rsa);
+    }
+
+    return 1;
+}
+
+static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p)
+{
+    size_t mlen = pgpMpiLen(p) - 2;
+    struct pgpDigKeyRSA_s *key = pgpkey->data;
+
+    if(!key) {
+        key = pgpkey->data = OPENSSL_secure_zalloc(sizeof(*key));
+        if (!key) return 1;

As the keys are public-only is it worth it using the secure memory allocator?

> +    switch (num) {
+    case 0:
+        if (!bn) {
+            bn = sig->bn = BN_new();
+        }
+        if (!bn) return 1;
+
+        /* Create a BIGNUM from the signature pointer.
+           Note: this assumes big-endian data as required
+           by the PGP multiprecision integer format
+           (RFC4880, Section 3.2)
+           This will be useful later, as we can
+           retrieve this value with appropriate
+           padding. */
+        bn = BN_bin2bn(p+2, mlen, bn);
+        if (!bn) return 1;

It's unclear to me why do you convert the RSA signature to BIGNUM and then back when calling the verification. Wouldn't it be better to just store the data directly?

> +    BIGNUM *r;
+    BIGNUM *s;
+
+    DSA_SIG *dsa_sig;
+};
+
+static int constructDSASignature(struct pgpDigSigDSA_s *sig)
+{
+    int rc;
+
+    if (sig->dsa_sig) {
+        /* We've already constructed it, so just reuse it */
+        return 1;
+    }
+
+    /* Create the DSA key */

Nitpick: you mean 'Create the DSA signature' here

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/129#pullrequestreview-20301305
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20170206/511b70ab/attachment.html>