Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Require OpenSSL >= 1.0.2 explicitly #1487

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Require OpenSSL >= 1.0.2 explicitly #1487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ endif()

find_package(Boost 1.57.0 COMPONENTS ${BOOST_COMPONENTS} REQUIRED)
find_package(CURL REQUIRED)
find_package(OpenSSL REQUIRED)
find_package(OpenSSL 1.0.2 REQUIRED)
find_package(Threads REQUIRED)
find_package(LibArchive REQUIRED)
find_package(sodium REQUIRED)
Expand Down
1 change: 1 addition & 0 deletions README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ The default versions packaged in recent Debian/Ubuntu releases are generally new

* cmake (>= 3.5)
* curl (>= 7.47)
* openssl (>= 1.0.2)
* libboost-* (>= 1.58.0)
* libcurl4-openssl-dev (>= 7.47)
* libpthread-stubs0-dev (>= 0.3)
Expand Down
17 changes: 9 additions & 8 deletions src/libaktualizr/crypto/crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,14 +341,16 @@ StructGuard<EVP_PKEY> Crypto::generateRSAKeyPairEVP(KeyType key_type) {
return {nullptr, EVP_PKEY_free};
}

#if AKTUALIZR_OPENSSL_PRE_11
StructGuard<RSA> rsa(RSA_generate_key(bits, /* number of bits for the key - 2048 is a sensible value */
RSA_F4, /* exponent - RSA_F4 is defined as 0x10001L */
nullptr, /* callback - can be NULL if we aren't displaying progress */
nullptr), /* callback argument - not needed in this case */
RSA_free);
#else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@zabbal zabbal Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

int ret;

ret = RAND_status();
Copy link
Contributor

@lbonn lbonn Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another interesting reference: openssl/openssl#9595

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@lbonn lbonn Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (ret != 1) { /* random generator has NOT been seeded with enough data */
ret = RAND_poll();
if (ret != 1) { /* seed data was NOT generated */
return {nullptr, EVP_PKEY_free};
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@zabbal zabbal Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


StructGuard<BIGNUM> bne(BN_new(), BN_free);
ret = BN_set_word(bne.get(), RSA_F4);
if (ret != 1) {
Expand All @@ -361,7 +363,6 @@ StructGuard<EVP_PKEY> Crypto::generateRSAKeyPairEVP(KeyType key_type) {
if (ret != 1) {
return {nullptr, EVP_PKEY_free};
}
#endif

StructGuard<EVP_PKEY> pkey(EVP_PKEY_new(), EVP_PKEY_free);
// release the rsa pointer here, pkey is the new owner
Expand Down