Require OpenSSL >= 1.0.2 explicitly #1487

zabbal · 2019-12-12T13:25:41Z

Before our code used RSA_generate_key() deprecated since 0.9.8 while checking for OpenSSL < 1.1.0

We also did not epxlicitly check whether OpenSSL's random number generator was properly seeded.

Let's make sure that library users won't generate predictable keys by accident:
* get rid of deprecated function
* explicitly check for random number generator state

pattivacek · 2019-12-12T13:57:01Z

I think this is fine, since it reflects the current reality and it's good to make that explicit. Would you mind specifying it in the README as well, since that mentions a few other minimum versions already?

codecov-io · 2019-12-12T14:01:06Z

Codecov Report

Merging #1487 into master will decrease coverage by 0.23%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master    #1487      +/-   ##
==========================================
- Coverage      82%   81.76%   -0.24%     
==========================================
  Files         186      186              
  Lines       11398    11403       +5     
==========================================
- Hits         9347     9324      -23     
- Misses       2051     2079      +28

Impacted Files	Coverage Δ
src/libaktualizr/crypto/crypto.cc	`85.99% <40%> (-0.12%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.h	`60% <0%> (-40%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.cc	`74.32% <0%> (-4.73%)`	⬇️
src/libaktualizr/storage/sqlstorage.cc	`76.94% <0%> (-1.52%)`	⬇️
src/libaktualizr/storage/sql_utils.h	`84.72% <0%> (-1.39%)`	⬇️
src/libaktualizr/package_manager/ostreemanager.cc	`79.02% <0%> (-0.75%)`	⬇️
src/libaktualizr/primary/sotauptaneclient.cc	`90.01% <0%> (-0.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ead19c...1609aa5. Read the comment docs.

zabbal · 2019-12-12T14:01:55Z

Would you mind specifying it in the README as well, since that mentions a few other minimum versions already?

Updated.

pattivacek

Thanks!

pattivacek · 2019-12-12T14:10:07Z

Small thing: according to our contribution guidelines, in general we like to have a brief description of the fix after the ticket number (if applicable) in the git branch name. Not a big deal, but useful especially for people who don't have access to our ticketing system. No need to change this PR/branch, though.

lbonn · 2019-12-12T14:37:41Z

It breaks CI on xenial

zabbal · 2019-12-12T14:41:14Z

It breaks CI on xenial

Doh! And it's supported till 2021. Is there reason why we use it instead of bionic?
To double-check: we don't test ostree support in CI? From what I can see xenial don't have it.

lbonn · 2019-12-12T14:47:57Z

We mostly test on bionic but keep a minimum compatibility with Xenial as it's still a supported LTS.

Though, removing this CI job has already been brought to the table as it is annoying to support (mainly boost reasons).

zabbal · 2019-12-12T14:50:47Z

I'd vote for dropping xenial unless we have some customer willing to pay for support.

pattivacek · 2019-12-12T15:19:56Z

I would've liked to wait until 20.04 was released; we traditionally tried to support the latest two LTEs. There is not really a particularly good reason or any consistency or documentation with that decision. Maybe we could move to using 18.04 and 19.10 until 20.04 is released?

zabbal · 2019-12-12T15:29:42Z

I'm fine either way. But to clarify things, by:

we traditionally tried to support the latest two LTEs
you mean that we have supported compilation without ostree?

This seems to make sense as ostree is hardly native packaging solution for ubuntu anyway but what's the actual intended use-case for aktualizr on ubuntu: is there particular feature set which should be working?

pattivacek · 2019-12-12T15:38:53Z

you mean that we have supported compilation without ostree?

We do, but that's a separate topic.

what's the actual intended use-case for aktualizr on ubuntu: is there particular feature set which should be working?

It's arguably more about the garage tools. We want to make sure they work on the systems we expect people to use them on. It's less interesting for aktualizr, but it is still nice to be able to run it on host systems for development and testing. In the past, this partially reflected the build environments of our team. Now I think we're somewhat more standardized, but it still makes some sense to keep our software reasonably easy to build with slightly older OSes.

eu-siemann · 2019-12-13T14:30:22Z

BTW, we already have several places with conditional compilation using #define AKTUALIZR_OPENSSL_PRE_11 (OPENSSL_VERSION_NUMBER < 0x10100000) macro.

zabbal · 2019-12-18T12:30:47Z

This should hopefully make CI happy.

pattivacek · 2019-12-18T14:42:25Z

src/libaktualizr/crypto/crypto.cc

-                                        nullptr,  /* callback - can be NULL if we aren't displaying progress */
-                                        nullptr), /* callback argument - not needed in this case */
-                       RSA_free);
-#else


This looks like we're dropping the minimal support for pre-1.1 that we had. Am I misreading it?

We're dropping support for pre-0.9.8 - the check against 1.1 was incorrect. Otherwise CI on xenial would barf.

Do we have a way to test that 0.9.8 will work? My memory is that xenial is still something like 1.0.2. Although if the xenial tests are passing with this as it is, that's probably good enough. I don't think we've ever tested something before 1.0.

If we hadn't tested 0.9.8 before than I don't think it's worth it to start now.

pattivacek · 2019-12-18T14:43:29Z

src/libaktualizr/crypto/crypto.cc

+    if (ret != 1) { /* seed data was NOT generated */
+      return {nullptr, EVP_PKEY_free};
+    }
+  }


Can you elucidate a bit what the goal of this chunk is? If both of these calls fail, is that a problem, or is silently failing and moving on acceptable?

Yes, it's a problem and we should fail. The likelyhood of this in practice is pretty low though:

we got to be linked against pre-1.1 (if I understood docs correctly from 1.1 onwards RSA_generate_key_ex() always does re-seeding correctly)

we've got to be called at early boot stage with not enough entropy in the pool

Still, I'd rather be paranoid and fail than accidentially generate predictable keys.
It might also be the case that I'm misinterpreting OpenSSL docs so further references are more than welcomed.

It looks like we will fail as expected when this function returns null in p11engine.cc but not in Crypto::generateRSAKeyPair(), or at least not immediately. Minor complaint, and other than that, I think this looks fine after some further consideration. @eu-smirnov @lbonn would either of you mind taking a look as well to confirm that this makes sense to you?

@zabbal Can you give some references from what you saw in the docs?

https://www.openssl.org/docs/man1.0.2/man3/RSA_generate_key_ex.html
https://www.openssl.org/docs/man1.1.1/man3/RSA_generate_key_ex.html

Is your concern based on this sentence present in the 1.1.1 docs and not in the 1.0.2?

If the automatic seeding or reseeding of the OpenSSL CSPRNG fails due to external circumstances (see RAND(7)), the operation will fail.

Is your concern based on this sentence present in the 1.1.1 docs and not in the 1.0.2?

That and the "The pseudo-random number generator must be seeded prior to calling RSA_generate_key_ex()" which is present in both versions.

See also https://www.openssl.org/docs/manmaster/man3/RAND_add.html - in particular "RAND_status() indicates whether or not the random generator has been sufficiently seeded".

I'm not too concerned because if the operation fails it will return an error and not silently give us a bad key. If you still think that's best we can still keep these lines I suppose.

Well, it's not best in a sense "this guarantees lack of bad keys". It's best in a sense "we follow docs closely" and "we won't have bad keys provided docs are correct". The latter might not be the case of course but that would be separate fix on top of this.

lbonn · 2019-12-19T14:47:20Z

src/libaktualizr/crypto/crypto.cc

  int ret;
+
+  ret = RAND_status();


I've had a look at OpenSSL and I'm not convinced this is needed, as the same check is done in rand_bytes: https://github.com/openssl/openssl/blob/27007233db5d6f8b91ed474c4e09dd7014871cc6/crypto/rand/md_rand.c#L389.

A check like that only verifies that the library is self-consistent but the bad scenario is actually when openssl thinks everything was seeded properly but it read from /dev/urandom, blindly trusting that the kernel had finished seeding (urandom will never block).

After a quick strace, on recent distributions, openssl 1.1.1d uses getrandom() to seed its internal rng (safe). On xenial with Openssl 1.0.2g, it seems it's using urandom directly and uses the NONBLOCK + check for EAGAIN mentioned here https://lwn.net/Articles/606141/: https://github.com/openssl/openssl/blob/27007233db5d6f8b91ed474c4e09dd7014871cc6/crypto/rand/rand_unix.c#L377. I was afraid that it would read from urandom blindly which can be unsafe at early boot...

But maybe Openssl 1.0.0 with Linux >= 4.8 is unsafe, according to this recent comment? https://github.com/openssl/openssl/blob/0ab6fc79a9a63370be1a615729dc2a6ed0d6c89b/crypto/rand/rand_unix.c#L37

Maybe the simplest fix would be to have a globabl blocking getrandom() followed by RAND_poll()

Another interesting reference: openssl/openssl#9595

Hm actually I misread the LWN post and the EAGAIN check probably does not guarantee anything. The last solution might actually be the better one.

as the same check is done in rand_bytes

Erm, I'm kinda confused. The RAND_status() and RAND_poll() are not the same. Also I'd rather avoid depending on OpenSSL's internal implementation details. If RSA_generate_key_ex() man says that RNG should be seeded than I see no harm in explicitly checking that it's seeded - even in OpenSSL does the same internally.

Maybe the simplest fix would be to have a globabl blocking getrandom()

This would introduce dependency on Linux kernel 3.17+ and glibc 2.25+ which is not available on xenial AFAIK. I'm also not sure how it differs from current PR - could you elaborate?

bad scenario is actually when openssl thinks everything was seeded properly

Indeed, my concern as well. On possible workaround could be using https://www.freedesktop.org/software/systemd/man/systemd-random-seed.service.html with entropy crediting to make sure we start after entropy pool has been taken care of.

Maybe the simplest fix would be to have a globabl blocking getrandom()

This would introduce dependency on Linux kernel 3.17+ and glibc 2.25+ which is not available on xenial AFAIK. I'm also not sure how it differs from current PR - could you elaborate?

The syscall can also be used without going through glibc.
It differs from current PR because it would ensure that the systemd rng used by /dev/urandom is indeed seeded then force a reseed of openssl's RNG (which will use the system's RNG). Whereas your PR checks that openssl's RNG is seeded, though it may have used a bad source by reading /dev/urandom too early.

zabbal · 2019-12-19T16:00:30Z

To summarize: I can digg through OpenSSL sources to see if RAND_status() can bullshit us in the versions we're concerned about but this might result in another PR on top of this.

Is there reason not to merge this on in a meantime?

lbonn · 2019-12-19T16:36:58Z

Not related to your changes but since @patrickvacek mentioned it and it's easy to fix, we can add a check for nullptr after the call to generateRSAKeyPair() in Crypto::generateRSAKeyPair(). It would probably lead to an error later but better fail early.

zabbal · 2019-12-20T09:59:16Z

we can add a check for nullptr after the call to generateRSAKeyPair() in Crypto::generateRSAKeyPair().

Agree, thought it's indeed independent of this PR - the function can already drop nullptr so the fix can be done in parallel/ahead of this PR. Feel free to add me as reviwer.

lbonn · 2019-12-20T16:32:18Z

Ok for the code change themselves then.

But I had a look at the openssl versions at https://en.wikipedia.org/wiki/OpenSSL#Major_version_releases and https://www.openssl.org/policies/releasestrat.html.

0.9.8 is really outdated and not supported at all these days, so it's a bit ridiculous to add it as a minimum requirement.

Yocto has had 1.0.2+ since Fido (April 2015) and also on Morty. So maybe we can raise the requirement to that?

pattivacek · 2019-12-23T07:57:19Z

Yocto has had 1.0.2+ since Fido (April 2015) and also on Morty. So maybe we can raise the requirement to that?

I second that. I don't think we've ever tested 0.9.8, but we have used morty, so we know 1.0.2 should be fine.

lbonn · 2020-01-17T17:27:05Z

@zabbal what are you thoughts on changing it to 1.0.2 (see the previous two comments)?

zabbal · 2020-01-20T13:06:22Z

@zabbal what are you thoughts on changing it to 1.0.2 (see the previous two comments)?

Sure, will rebase/update in next revision.

Before our code used RSA_generate_key() deprecated since 0.9.8 while checking for OpenSSL < 1.1.0 We also did not epxlicitly check whether OpenSSL's random number generator was properly seeded. Let's make sure that library users won't generate predictable keys by accident: * get rid of deprecated function * explicitly check for random number generator state Signed-off-by: Max <max.suraev@here.com>

zabbal requested review from lbonn and pattivacek December 12, 2019 13:25

zabbal force-pushed the fix/OTA-966 branch from 8830232 to 24de62a Compare December 12, 2019 13:26

pattivacek requested a review from eu-siemann December 12, 2019 13:56

zabbal force-pushed the fix/OTA-966 branch from 24de62a to aeab11a Compare December 12, 2019 14:01

pattivacek previously approved these changes Dec 12, 2019

View reviewed changes

zabbal dismissed pattivacek’s stale review via 61ba90c December 18, 2019 12:27

zabbal force-pushed the fix/OTA-966 branch from aeab11a to 61ba90c Compare December 18, 2019 12:27

zabbal changed the title ~~Request OpenSSL >= 1.1.0 explicitly~~ Require OpenSSL >= 0.9.8 explicitly Dec 18, 2019

pattivacek reviewed Dec 18, 2019

View reviewed changes

lbonn reviewed Dec 19, 2019

View reviewed changes

lbonn mentioned this pull request Dec 20, 2019

More careful error checking around key generation #1499

Merged

zabbal force-pushed the fix/OTA-966 branch from 61ba90c to 1609aa5 Compare January 20, 2020 15:45

zabbal changed the title ~~Require OpenSSL >= 0.9.8 explicitly~~ Require OpenSSL >= 1.0.2 explicitly Jan 20, 2020

lbonn approved these changes Jan 21, 2020

View reviewed changes

pattivacek approved these changes Jan 21, 2020

View reviewed changes

lbonn merged commit f8d4d25 into master Jan 21, 2020

lbonn deleted the fix/OTA-966 branch January 21, 2020 15:20

Require OpenSSL >= 1.0.2 explicitly #1487

Require OpenSSL >= 1.0.2 explicitly #1487

Conversation

zabbal commented Dec 12, 2019 • edited Loading

pattivacek commented Dec 12, 2019

codecov-io commented Dec 12, 2019 • edited Loading

Codecov Report

zabbal commented Dec 12, 2019

pattivacek left a comment

Choose a reason for hiding this comment

pattivacek commented Dec 12, 2019 • edited Loading

lbonn commented Dec 12, 2019

zabbal commented Dec 12, 2019 • edited Loading

lbonn commented Dec 12, 2019

zabbal commented Dec 12, 2019

pattivacek commented Dec 12, 2019

zabbal commented Dec 12, 2019

pattivacek commented Dec 12, 2019

eu-siemann commented Dec 13, 2019

zabbal commented Dec 18, 2019

Choose a reason for hiding this comment

zabbal Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zabbal Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbonn Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbonn Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

zabbal commented Dec 19, 2019

lbonn commented Dec 19, 2019

zabbal commented Dec 20, 2019

lbonn commented Dec 20, 2019

pattivacek commented Dec 23, 2019

lbonn commented Jan 17, 2020

zabbal commented Jan 20, 2020

zabbal commented Dec 12, 2019 •

edited

Loading

codecov-io commented Dec 12, 2019 •

edited

Loading

pattivacek commented Dec 12, 2019 •

edited

Loading

zabbal commented Dec 12, 2019 •

edited

Loading

zabbal Dec 18, 2019 •

edited

Loading

zabbal Dec 18, 2019 •

edited

Loading

lbonn Dec 19, 2019 •

edited

Loading

lbonn Dec 19, 2019 •

edited

Loading