Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
BTW there's a nice little bonus included here: it's *hugely* faster than NSS. On my laptop with signature checking enabled, 'time rpm -qa|wc -l' is 1.9s with NSS, with openssl it's down to 1.3s. Plain digest verification is faster too but not quite so dramatically. -- 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#issuecomment-280310030___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Closed #129. -- 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#event-964758921___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Thanks a lot for all the effort that went into this! Merged. -- 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#issuecomment-280296406___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Conan-Kudo approved this pull request. The changes look good to me for Linux, and it properly fails to allow usage of OpenSSL backend from the configure step on macOS. -- 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-21022178___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
My plan was to let @ffesti conduct the actual merge ceremony, web buttons doing stuff to code make me nervous :) -- 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#issuecomment-278659644___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Yeah, I'm going to squash them all down to a single commit. One moment. -- 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#issuecomment-278658414___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Looks good to me, thanks. -- 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#issuecomment-278657373___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@pmatilai OK, I added some text to the INSTALL file. Please proofread it. -- 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#issuecomment-278648068___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@sgallagher pushed 1 commit. 907a1a3 Add OpenSSL to INSTALL -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/129/files/992e94cbacdbafd4b42db24ba275f5bffc77b8f6..907a1a3cdfcd55a85506f0621738bcd82dcaaa9c ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
More or less. It wouldn't hurt to add a small blurb to INSTALL explaining the new --with-crypto option plus a disclaimer about the openssl license issue. -- 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#issuecomment-278565345___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@pmatilai @ffesti OK, so is this good to merge at this point? -- 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#issuecomment-278416863___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@t8m Just to confirm, does the current version correctly address your concerns about overwriting the BIGNUM values? -- 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#issuecomment-278388852___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
t8m approved this pull request. This looks good and it simplifies the patch, there is no point in using OpenSSL allocators for memory that is not later freed inside OpenSSL. -- 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-20781012___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Thanks, but you made me realize one problematic thing with that patch: it was now a situation where on OpenSSL 1.0.2, we are allocating with `malloc()` and freeing with `OPENSSL_free()`, which is not correctly paired. We really need to be doing one or the other, and it looks to me like there really isn't any advantage to using the OpenSSL versions where I have been anyway. So I'll strike them completely and simplify the code. -- 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#issuecomment-278340621___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Just FWIW, my previous comment was written before seeing your latest fixup, so it was more about general principle than the actual fixup. Didn't expect you to fix it that fast :) -- 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#issuecomment-278339101___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Hmm, well I was trying to remain consistent, but you may be right; the places where I'm using `OPENSSL_zalloc()` would all be happily satisfied by using regular malloc or the RPM allocators, so I think I'll just rework the patches to use those instead. Also, I noted yesterday that the long series of FIXUP patches I've been tacking on here don't actually apply cleanly. I think I'm going to just squash everything up to this point into a single patch and provide further changes as fixups. -- 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#issuecomment-278336521___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Mmh, I'd rather not have hundreds of lines of additional configure goo just to check for incompatibilities between a couple of versions of an optional crypto backend, lets try to keep the checks in check, so to speak. But if OPENSSL_malloc() and OPENSSL_free() are not compatible with common malloc() and free() then OPENSSL_zalloc() almost certainly should not be falling back to plain old malloc() + memset(), but OPENSSL_malloc(). The other question is, does openssl actually require the use of its allocators for these purposes? Life would be simpler if not... -- 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#issuecomment-278335434___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@sgallagher pushed 1 commit. f59f329 fixup! Add initial OpenSSL support -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/129/files/f70f78e6242b1a04a49cb6d679e096829ec3f544..f59f329148bfd9dc96d755a3b134cfd898c1e464 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Actually, that warning isn't harmless. I need to add a configure check for that. It's strange, neither `OPENSSL_malloc()` nor `OPENSSL_free()` are listed in the official 1.0.2 [docs](https://www.openssl.org/docs/man1.0.2/crypto/). That's why I created their compatibility functions. I need to be more specific, it seems. -- 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#issuecomment-278330830___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
mlschroe commented on this pull request.
> +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;
(That's a different kind of padding, it's how the provided hash is converted to
a bignum. See https://tools.ietf.org/html/rfc2313 sections 8.1 and 10.1 for the
gory details.)
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
Right, padding seems to be working now too and not seeing anything weird in brief testing (throw a big pile of both good and fuzzed packages at it, testsuite passes). I'm getting this (apparently harmless) warning on F25 (so openssl-1.0.2j): ``` digest_openssl.c:26:0: warning: "OPENSSL_free" redefined # define OPENSSL_free free In file included from /usr/include/openssl/bio.h:69:0, from /usr/include/openssl/evp.h:75, from digest_openssl.c:3: /usr/include/openssl/crypto.h:390:0: note: this is the location of the previous definition # define OPENSSL_free(addr) CRYPTO_free(addr) ``` Some other random notes, more on the stylistic side: - You don't need to check for NULL from the rpm allocators xcalloc() and xmalloc() aka rcalloc() and rmalloc() and the like, they exit() on OOM so NULL is not a possible return value. Checking nevertheless is mostly harmless but then it may give somebody the idea that it CAN fail. Regular malloc() etc where used are different of course. - Generally a single point of exit is preferred in functions, in particular those where resources are allocated and need to be freed on errors, pgpVerifySigRSA() is a good example of doing just that. There are some functions - at least rpmDigestDup() and rpmDigestInit() - that would probably be cleaner and simpler if they followed that "goto err" idiom. -- 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#issuecomment-278272825___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
I believe I have addressed all of the review comments thus far. I also ran the code through a Coverity static analysis, which came up clean. -- 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#issuecomment-278201836___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +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;
OK, I will drop the secure memory entirely. I was being overcautious.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +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;
Beecrypt is definitely doing some sort of padding:
https://github.com/sgallagher/rpm/blob/openssl/rpmio/digest_beecrypt.c#L288
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
pmatilai commented on this pull request.
> +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;
Oh, my recollection of beecrypt requiring padding is from some ancient comment
by somebody, somewhere, so either I'm misremembering, things have changed since
then or the commenter was wrong. Or any combination of the three. Sorry, didn't
intend to spread misinformation.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
mlschroe commented on this pull request.
> +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;
beecrypt needs manual zero-padding? Where in the code would that be?
Anyway, the current code doesn't do padding as sig->len is used as length. If
openssl needs padding (which it seems to do), you must use
EVP_PKEY_size(pkey_ctx) instead.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
t8m commented on this pull request.
> +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;
I'd probably try removing the conversion to BIGNUM and back if it solves the
padding problem. If not then it needs to be investigated how to handle the
padding properly.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
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 */
+
No, I mean when you assign the sig->r values you overwrite the previous values
that could be there which means the previous value will be leaked. If rpm
guarantees that pgpSet(Key|Sig)Mpi* functions won't be called on the same
key/sig twice with the same index it is not strictly necessary to handle this,
because you will have the previous value always set to NULL. But I do not know
whether rpm really guarantees that when reading some malformed signature/key
data.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
t8m commented on this pull request.
> +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 Panu says - as there are no secret/private key operations at all, there is
no point in using the secure memory in the patch at all.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
pmatilai commented on this pull request.
> +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;
FWIW, rpm only ever deals with public keys, signing is done with gpg.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
pmatilai commented on this pull request.
> +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;
Oh and FWIW, padding is a relatively rare condition. On my F25 laptop there are
3366 packages installed and 19 of them are "padding case", one of them being
(funny coincidence) that nss-softokn-3.28.1-1.0.fc25.x86_64.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
pmatilai commented on this pull request.
> +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;
NSS and beecrypt do need manual zero-padding, I don't know about openssl but
maybe its smarter. What I can tell you that the current code doesn't handle
padding correctly. Here's a reproducer package from the original NSS bug:
https://dl.fedoraproject.org/pub/archive/fedora/linux/releases/11/Fedora/x86_64/os/Packages/libuser-python-0.56.9-3.x86_64.rpm
but if you prefer a newer one, nss-softokn-3.28.1-1.0.fc25.x86_64 also
requires zero-padding (with NSS) and fails with the current openssl
implementation.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +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;
Mostly I was copying that from the beecrypt and NSS implementations, where the
signature was padded before comparison. Are you saying I can skip the padding
entirely at
https://github.com/rpm-software-management/rpm/pull/129/files/a7bc1e416e2f0f193be63317701bd3b18e2934b6#diff-763b93abc6d6f5549dafd08d9d07R523
?
This is definitely a place where I don't actually know what I'm doing...
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +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;
I figured that, given that RPM interaction is a computationally-rare event, it
was easier on me to just assume everything should be secure-allocated rather
than trying to figure out when it was appropriate and when it was safe to skip
it.
If you are telling me that I should just not use it at all in this code, I'll
drop it.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher 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 */
+
I'm not really sure what you mean by "release" them? The way this function is
expected to work is that it updates any values that aren't NULL and does not
change any that are. Or at least, that's the way that the OpenSSL 1.1.0 version
implements them.
Also, these are "set" not "assigned", so it's not expected that this function
should be owning their memory management either. Meaning I shouldn't be calling
BN_free() before assigning the new value.
Am I way off-base 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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@t8m Sorry, could you be more specific? I'm not sure which functions are risky or which references need to be freed. -- 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#issuecomment-277728498___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
t8m requested changes on this pull request. The various _set0_ functions you add for compatibility with OpenSSL 1.0.2 do not properly handle multiple calls of the same function on the object - the old references are not freed. I understand that in the current patch this probably isn't a problem but it might happen in future. It might be better to release the old references before assigning new ones. -- 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-20297097___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@Conan-Kudo I just pushed a couple additional configure.ac changes that should ensure that it's using at least OpenSSL 1.0 -- 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#issuecomment-277719485___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@sgallagher pushed 2 commits. de07401 fixup! Add initial OpenSSL support a7bc1e4 fixup! Add compatibility layer for OpenSSL 1.0.2 -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/129/files/d8e6b09913a4b383ad0dc0156e2539083f1fcb39..a7bc1e416e2f0f193be63317701bd3b18e2934b6 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@Conan-Kudo Thanks, I'll see if I can make the configure checks a little more strict. Just to confirm: those other two bullet points were you asserting that things worked correctly, right? (It's a little confusing when corrective feedback is in the same list). -- 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#issuecomment-277678554___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
So, I've had a chance to test this on MacOS... A few points of feedback: * The configure script _does not_ catch that OpenSSL 0.9.8zh is incompatible, and the build fails due to undeclared things (`EVP_PKEY_CTX` and `pkey_ctx` were mentioned before it bailed out). It needs some better checks to ensure it only works with compatible OpenSSL versions. * It does build properly with the new flag when I select `beecrypt` (as I have already built beecrypt for RPM before). * The `--with-beecrypt` error returns a useful error, telling the user to switch to `--with-crypto=beecrypt`. -- 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#issuecomment-277547616___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
t8m commented on this pull request.
> +}
+
+int rpmDigestFinal(DIGEST_CTX ctx, void ** datap, size_t *lenp, int asAscii)
+{
+int ret;
+unsigned char *digest = NULL;
+unsigned int digestlen;
+
+if (ctx == NULL) return -1;
+
+digestlen = EVP_MD_CTX_size(ctx->md_ctx);
+digest = xcalloc(digestlen, sizeof(*digest));
+if (digest == NULL) return -1;
+
+ret = EVP_DigestFinal_ex(ctx->md_ctx, digest, );
+if (ret != 1) goto done;
Yes, in most places there might be both 0 and -1 returns where 0 usually
indicates "regular" failures such as hash or signature verification failure and
-1 some kind of "fatal" error - such as unknown hash algorithm or memory
allocation error. So the safest thing for functions returning 1 as success is
to use `ret != 1` or even `ret <= 0` as test for failure.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +else {
+/* ASCII requested */
+if (lenp) *lenp = (2*digestlen) + 1;
+if (datap) {
+const uint8_t * s = (const uint8_t *) digest;
+*datap = pgpHexStr(s, digestlen);
+}
+}
+
+ret = 1;
+
+done:
+if (digest) {
+/* Zero the digest, just in case it's sensitive */
+memset(digest, 0, digestlen);
+ free(digest);
Somehow a tab snuck in there. Must have been originally some copy-pasted code
because I am tab-averse.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request. > +WITH_OPENSSL_INCLUDE= +WITH_OPENSSL_LIB= +if test "$with_openssl" = yes; then +# If we have pkgconfig make sure CPPFLAGS are setup correctly for the OpenSSL +# -I include path. +AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no], [$PATH:/usr/bin:/usr/local/bin]) +if test "x$PKGCONFIG" != "xno"; then + CPPFLAGS="$CPPFLAGS $($PKGCONFIG --cflags libcrypto)" +fi + +AC_CHECK_HEADERS([openssl/evp.h], [], [ + AC_MSG_ERROR([missing required OpenSSL header]) +]) + +AC_CHECK_LIB(crypto, EVP_DigestInit_ex, [ + WITH_OPENSSL_LIB=$($PKGCONFIG --libs libcrypto) Yeah, that's a bug. I'll fix it. -- 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___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +}
+
+int rpmDigestFinal(DIGEST_CTX ctx, void ** datap, size_t *lenp, int asAscii)
+{
+int ret;
+unsigned char *digest = NULL;
+unsigned int digestlen;
+
+if (ctx == NULL) return -1;
+
+digestlen = EVP_MD_CTX_size(ctx->md_ctx);
+digest = xcalloc(digestlen, sizeof(*digest));
+if (digest == NULL) return -1;
+
+ret = EVP_DigestFinal_ex(ctx->md_ctx, digest, );
+if (ret != 1) goto done;
In this case, that would work, but there are many places in OpenSSL where
success == 1 and any other value indicates an error. I kept it the same here
for consistency.
--
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___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
ignatenkobrain commented on this pull request.
> +else {
+/* ASCII requested */
+if (lenp) *lenp = (2*digestlen) + 1;
+if (datap) {
+const uint8_t * s = (const uint8_t *) digest;
+*datap = pgpHexStr(s, digestlen);
+}
+}
+
+ret = 1;
+
+done:
+if (digest) {
+/* Zero the digest, just in case it's sensitive */
+memset(digest, 0, digestlen);
+ free(digest);
I think something went wrong with style ;)
--
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-17257123___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
