Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OpenSSL crypto backend #129

Closed

Conversation

Projects
None yet
7 participants
@sgallagher
Copy link
Contributor

sgallagher commented Jan 18, 2017

These patches add support for building RPM against OpenSSL 1.1.0 to provide hashing and signature functionality. It is enabled by passing --with-openssl to configure. (For backwards compatibility, it retains the use of --with[out]-nss as a toggle between beecrypt and nss).

I have tested these patches with the following scenarios:

  • Sign a package using a 2048-bit RSA key with RPM compiled against OpenSSL and verify it with RPM compiled against OpenSSL.
  • Sign a package using a 2048-bit RSA key with RPM compiled against OpenSSL and verify it with RPM compiled against NSS.
  • Sign a package using a 1024-bit DSA key with RPM compiled against OpenSSL and verify it with RPM compiled against OpenSSL.
  • Sign a package using a 1024-bit DSA key with RPM compiled against OpenSSL and verify it with RPM compiled against NSS.
  • Sign a package using a 2048-bit RSA key with RPM compiled against NSS and verify it with RPM compiled against OpenSSL.
  • Sign a package using a 1024-bit DSA key with RPM compiled against NSS and verify it with RPM compiled against OpenSSL.

See also: #119

])

AC_CHECK_LIB(crypto, EVP_DigestInit_ex, [
WITH_OPENSSL_LIB=$($PKGCONFIG --libs libcrypto)

This comment has been minimized.

Copy link
@ignatenkobrain

ignatenkobrain Jan 18, 2017

Member

I think this will crash if --with-openssl is set, but pkg-config is not found

This comment has been minimized.

Copy link
@sgallagher

sgallagher Jan 18, 2017

Author Contributor

Yeah, that's a bug. I'll fix it.

if (digest) {
/* Zero the digest, just in case it's sensitive */
memset(digest, 0, digestlen);
free(digest);

This comment has been minimized.

Copy link
@ignatenkobrain

ignatenkobrain Jan 18, 2017

Member

I think something went wrong with style ;)

This comment has been minimized.

Copy link
@sgallagher

sgallagher Jan 18, 2017

Author Contributor

Somehow a tab snuck in there. Must have been originally some copy-pasted code because I am tab-averse.

if (digest == NULL) return -1;

ret = EVP_DigestFinal_ex(ctx->md_ctx, digest, &digestlen);
if (ret != 1) goto done;

This comment has been minimized.

Copy link
@ignatenkobrain

ignatenkobrain Jan 18, 2017

Member

shouldn't comparison be if (!ret)? I have not found much docs about return values, but I guess it is supposed to be used in such manner)

This comment has been minimized.

Copy link
@sgallagher

sgallagher Jan 18, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@t8m

t8m Jan 18, 2017

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.

@Conan-Kudo
Copy link
Member

Conan-Kudo left a comment

I'd prefer to have autotools change so that it would detect and auto-select based on what's available. Since the usage of these are mutually exclusive, it would also make sense to change the switches so that it reflects this more correct behavior (maybe --with-digest-backend=(nss|openssl|beecrypt)).

While I get the idea of backward compatibility, I do not think it is as important with build system changes, as the changes can just be documented so that as packagers upgrade RPM, they can change accordingly.

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Jan 19, 2017

@Conan-Kudo I'm willing to do that, if that's what the RPM upstream maintainers would prefer. I'd like to head from @pmatilai and @ffesti before I do, though.

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Jan 19, 2017

Yeah, --with-crypto=(beecrypt|nss|openssl|...) would be nice.
Compatibility in the build system is not particularly important - rpm is not something joerandom is likely to build anyway and we expect people upgrading rpm for distros to pay attention.

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Jan 19, 2017

OK, I will make that change. Would you like me to include a canary for --with-nss to fail the build if it is specified? That might make the transition easier.

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Jan 19, 2017

I don't have any strong opinions on that, but in general being explicit about failures never hurts.

@sgallagher sgallagher force-pushed the sgallagher:openssl branch from 910c59a to d8e6b09 Jan 25, 2017

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Jan 25, 2017

OK, new version of my patches, now supporting both OpenSSL 1.1.0 and 1.0.2

I implemented --with-crypto=CRYPTO_LIB and made --with-beecrypt report an error.

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Feb 5, 2017

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.

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 6, 2017

@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).

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Feb 6, 2017

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 6, 2017

@Conan-Kudo I just pushed a couple additional configure.ac changes that should ensure that it's using at least OpenSSL 1.0

@t8m
Copy link

t8m left a comment

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.

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 6, 2017

@t8m Sorry, could you be more specific? I'm not sure which functions are risky or which references need to be freed.

return 1;
}
#endif /* HAVE_DSA_SIG_SET0 */

This comment has been minimized.

Copy link
@t8m

t8m Feb 6, 2017

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

This comment has been minimized.

Copy link
@sgallagher

sgallagher Feb 6, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@t8m

t8m Feb 7, 2017

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.

This comment has been minimized.

Copy link
@sgallagher

sgallagher Feb 7, 2017

Author Contributor

OK, I think I finally see what you mean. I was thrown off because the comment was associated with the code where it's assigned to the final object (which can only happen once), but the issue was before this. I will fix it.

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;

This comment has been minimized.

Copy link
@t8m

t8m Feb 6, 2017

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.


if(!key) {
key = pgpkey->data = OPENSSL_secure_zalloc(sizeof(*key));
if (!key) return 1;

This comment has been minimized.

Copy link
@t8m

t8m Feb 6, 2017

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

This comment has been minimized.

Copy link
@sgallagher

sgallagher Feb 6, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@pmatilai

pmatilai Feb 7, 2017

Contributor

FWIW, rpm only ever deals with public keys, signing is done with gpg.

This comment has been minimized.

Copy link
@t8m

t8m Feb 7, 2017

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.

This comment has been minimized.

Copy link
@sgallagher

sgallagher Feb 7, 2017

Author Contributor

OK, I will drop the secure memory entirely. I was being overcautious.

retrieve this value with appropriate
padding. */
bn = BN_bin2bn(p+2, mlen, bn);
if (!bn) return 1;

This comment has been minimized.

Copy link
@t8m

t8m Feb 6, 2017

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?

This comment has been minimized.

Copy link
@sgallagher

sgallagher Feb 6, 2017

Author Contributor

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-763b444493abc6d6f5549dafd08d9d07R523 ?

This is definitely a place where I don't actually know what I'm doing...

This comment has been minimized.

Copy link
@pmatilai

pmatilai Feb 7, 2017

Contributor

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.

This comment has been minimized.

Copy link
@pmatilai

pmatilai Feb 7, 2017

Contributor

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.

This comment has been minimized.

Copy link
@t8m

t8m Feb 7, 2017

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.

This comment has been minimized.

Copy link
@mlschroe

mlschroe Feb 7, 2017

Contributor

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.

This comment has been minimized.

Copy link
@pmatilai

pmatilai Feb 7, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sgallagher

sgallagher Feb 7, 2017

Author Contributor

Beecrypt is definitely doing some sort of padding: https://github.com/sgallagher/rpm/blob/openssl/rpmio/digest_beecrypt.c#L288

This comment has been minimized.

Copy link
@sgallagher

sgallagher Feb 7, 2017

Author Contributor

@mlschroe Thanks, I wasn't entirely sure where to get the correct padding length. I will try that and see if it addresses the example package @pmatilai provided.

This comment has been minimized.

Copy link
@mlschroe

mlschroe Feb 8, 2017

Contributor

(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.)

return 1;
}

/* Create the DSA key */

This comment has been minimized.

Copy link
@t8m

t8m Feb 6, 2017

Nitpick: you mean 'Create the DSA signature' here

@sgallagher sgallagher force-pushed the sgallagher:openssl branch from a7bc1e4 to f70f78e Feb 8, 2017

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 8, 2017

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.

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Feb 8, 2017

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.
@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 8, 2017

Actually, that warning isn't harmless. I need to add a configure check for that.

It's strange, neither OPENSSL_zalloc() nor OPENSSL_free() are listed in the official 1.0.2 docs. That's why I created their compatibility functions. I need to be more specific, it seems.

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Feb 8, 2017

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...

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 8, 2017

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.

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Feb 8, 2017

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 :)

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 8, 2017

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.

@sgallagher sgallagher force-pushed the sgallagher:openssl branch from f59f329 to 992e94c Feb 8, 2017

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 8, 2017

I squashed the patches as I said I was going to and then added a fixup patch for the allocators so it's easy to review. Thanks again to everyone who has chimed in on this.

@t8m

t8m approved these changes Feb 8, 2017

Copy link

t8m left a comment

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.

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 8, 2017

@t8m Just to confirm, does the current version correctly address your concerns about overwriting the BIGNUM values?

@t8m

This comment has been minimized.

Copy link

t8m commented Feb 8, 2017

It mostly does. There is no case where it could be triggered in the current code anymore. So I am ok with it as is.

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 8, 2017

@pmatilai @ffesti OK, so is this good to merge at this point?

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Feb 9, 2017

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.

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 9, 2017

@pmatilai OK, I added some text to the INSTALL file. Please proofread it.

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Feb 9, 2017

Looks good to me, thanks.

@pmatilai
Copy link
Contributor

pmatilai left a comment

At this point, ACK for the whole lot.

@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Feb 9, 2017

@sgallagher Before having @pmatilai merge it, can you squash your fixup commits into the non fixup ones? We don't have autosquashing for them here...

@sgallagher

This comment has been minimized.

Copy link
Contributor Author

sgallagher commented Feb 9, 2017

Yeah, I'm going to squash them all down to a single commit. One moment.

Add OpenSSL support for digest and signatures
Autotools: add --with-crypto=openssl
This enables RPM to locate the appropriate flags for compiling
against OpenSSL for digest and hash functions.

This implementation changes the old behavior of
--with[out]-beecrypt toggling between beecrypt and nss. It will
now throw an error if attempting to use --with-beecrypt
indicating that the user should instead use --with-crypto=

See also:
#119

@sgallagher sgallagher force-pushed the sgallagher:openssl branch from 907a1a3 to ea79fa9 Feb 9, 2017

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Feb 9, 2017

My plan was to let @ffesti conduct the actual merge ceremony, web buttons doing stuff to code make me nervous :)

@Conan-Kudo
Copy link
Member

Conan-Kudo left a comment

The changes look good to me for Linux, and it properly fails to allow usage of OpenSSL backend from the configure step on macOS.

@ffesti

This comment has been minimized.

Copy link
Contributor

ffesti commented Feb 16, 2017

Thanks a lot for all the effort that went into this! Merged.

@ffesti ffesti closed this Feb 16, 2017

@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Feb 16, 2017

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.

@pmatilai pmatilai added this to the rpm-4.14.0 milestone Feb 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.