fix: KeyAttestation#key failing when using libopenssl v1.0.2 #5

santiagorodriguez96 · 2020-05-13T19:17:04Z

What

There's is bug when using libopenssl v1.0.2 where KeyAttestation#Key blows up with:

Why

The error is produced in this line, after calling OpenSSL::PKey::RSA#set_key.

If we turn on the debug mode for OpenSSL – OpenSSL.debug = true, we get:

It seems that the error is caused because the we are not setting the field e, which is the exponent.

If we take a look into the OpenSSL man page for the method RSA_set0_key, we can see that it states:

The n, e and d parameter values can be set by calling RSA_set0_key() and passing the new values for n, e and d as parameters to the function. The values n and e must be non-NULL the first time this function is called on a given RSA object.

I still didn't figure out why it is failing only when using OpenSSL v1.0.2 – and not when using v1.1.1.

How

Given that we only support the default exponent as for now, we can just pass it to the method and it will work.

How come that we only support the default exponent?

As specified in TPMv2-Part2 section 12.2.3.5.

A TPM compatible with this specification and supporting RSA shall support two primes and an exponent of zero. Support for other values is optional. Use of other exponents in duplicated keys is not recommended because the resulting keys would not be interoperable with other TPMs.

NOTE: Implementations are not required to check that exponent is the default exponent. They may fail to load the key if exponent is not zero.

Steps taken

- Write tests for reproducing bugs
- Configure Travis for testing against openssl v1.0.2 and v1.1.1
- Fix bug

TBD

- Find a way reduce the time that it takes to run each job, as it's taking aprox. 4 minutes each.
- Find a way of allowing jobs that use ruby-head to fail

santiagorodriguez96 · 2020-05-13T19:20:46Z

spec/tpm/key_attestation_spec.rb

+ context "when is not valid" do
+ before do
+ expect(key_attestation).to receive(:valid?).and_return(false)
+ end
+
+ it 'returns nil' do
+ expect(key_attestation.key).to be nil
+ end
+ end


I wanted to avoid re-writing all the cases in which KeyAttestation could be invalid, since we doing that above, in order to avoid duplicated code

Sounds good 👍

grzuy

Excellent 👏

.travis.yml

lib/tpm/t_public.rb

grzuy · 2020-05-21T23:12:50Z

spec/tpm/key_attestation_spec.rb

+ context "when is not valid" do
+ before do
+ expect(key_attestation).to receive(:valid?).and_return(false)
+ end
+
+ it 'returns nil' do
+ expect(key_attestation.key).to be nil
+ end
+ end


Sounds good 👍

spec/tpm/key_attestation_spec.rb

santiagorodriguez96 · 2020-05-22T18:01:31Z

.travis.yml

+ - env: RB=ruby-head LIBSSL=1.0
+ - env: RB=ruby-head LIBSSL=1.1


After trying hundred times, I finally make it so that Travis ignores all the builds that have the environment variable ruby-head.
It's important to note that Travis won't work if you don't define the entry in the allow_failures matrix exactly as you define it above – see this stackoverflow post. For example, this wouldn't work for me:

- env: - RB=ruby-head LIBSSL=1.0 - RB=ruby-head LIBSSL=1.1 ... matrix: fast_finish: true allow_failures: - env: RB=ruby-head

In addition, Travis would not recognize your allowed failures on multiple environment variables entries if you define them as entries inside the same key, for example, this would also not work for me:

- env: - RB=ruby-head LIBSSL=1.0 - RB=ruby-head LIBSSL=1.1 ... matrix: fast_finish: true allow_failures: - env: - RB=ruby-head LIBSSL=1.0 - RB=ruby-head LIBSSL=1.1

Gotcha 👍

grzuy

Nice work! 💯

While working with OpenSSL 1.0.2, some test were failing with the error: 'OpenSSL::PKey::RSA missing key'. This error is caused because no exponent is passed to the method OpenSSL::PKey::RSA#set_key in 'lib/tpm/t_public.rb'. Given that we only support the default exponent as for now, we can just pass it to the method and it will work.

grzuy · 2020-05-27T20:38:22Z

Some libssl 1.0 jobs are failing on master https://travis-ci.org/github/cedarcode/tpm-key_attestation/builds/691891472

santiagorodriguez96 · 2020-05-28T13:33:03Z

I re-run the build and now the job that was failing went green but the one with ruby 2.6.6, libopenssl 1.0 and openssl default failed with the same error – seems that we have a flaky one here.

santiagorodriguez96 · 2020-05-28T15:30:46Z

There seems to be a race condition between the not_after attribute in the certificate and the execution.
The problem is that when we create a certificate, we make it that is only valid for just 60 seconds. Therefore, when we are checking if the key attestation is valid, we check if the aik certificate is comformant which fails if the execution takes long enough when checking if it's in use.
If we take a look at that in_use? method we can see the condition which raises this race condition: now < not_after – clearly this fails if 60 seconds passed from the creation of the certificate.

santiagorodriguez96 · 2020-05-28T18:55:24Z

Well... turns out that isn't why the test are being flaky 😅 .

The test are being flaky because some of them randomly have their certificates not considered trustworthy, being more specific, the test are returning false in this line. I'm still searching for the reason though.

santiagorodriguez96 · 2020-05-29T17:15:39Z

It seems that when checking if the certificate is valid, comparing Time.now with the attribute not_before of a certificate (that is also initiated with Time.now) is sometimes returning an error due to the difference being too short – it could be related with the comparison of floats that are close to each other.

I created a PR in an attempt to solve this issue.

grzuy · 2020-05-30T20:01:12Z

I created a PR in an attempt to solve this issue.

Thank you!

santiagorodriguez96 requested a review from grzuy May 13, 2020 19:17

santiagorodriguez96 commented May 13, 2020

View reviewed changes

santiagorodriguez96 added the bug Something isn't working label May 13, 2020

santiagorodriguez96 changed the title ~~test: add test coverage for KeyAttestation#key method~~ fix: KeyAttestation#key failing when using livopenssl v1.0.2 May 13, 2020

santiagorodriguez96 changed the title ~~fix: KeyAttestation#key failing when using livopenssl v1.0.2~~ fix: KeyAttestation#key failing when using libopenssl v1.0.2 May 13, 2020

santiagorodriguez96 force-pushed the fix-key-attestation-blowing-up-in-openssl-1.0.2 branch 17 times, most recently from f21d23e to efc4a77 Compare May 20, 2020 13:52

grzuy reviewed May 21, 2020

View reviewed changes

santiagorodriguez96 force-pushed the fix-key-attestation-blowing-up-in-openssl-1.0.2 branch from 6d240c3 to 07a6fe9 Compare May 22, 2020 15:06

santiagorodriguez96 requested a review from grzuy May 22, 2020 15:13

santiagorodriguez96 force-pushed the fix-key-attestation-blowing-up-in-openssl-1.0.2 branch 5 times, most recently from d37bb2c to 750cae6 Compare May 22, 2020 15:57

santiagorodriguez96 force-pushed the fix-key-attestation-blowing-up-in-openssl-1.0.2 branch 2 times, most recently from e3c9a9e to 28c484e Compare May 22, 2020 17:42

santiagorodriguez96 commented May 22, 2020

View reviewed changes

grzuy approved these changes May 24, 2020

View reviewed changes

test: add test coverage for KeyAttestation#key method

e0e8748

santiagorodriguez96 force-pushed the fix-key-attestation-blowing-up-in-openssl-1.0.2 branch 3 times, most recently from e645c13 to 4ce48c9 Compare May 25, 2020 18:18

santiagorodriguez96 mentioned this pull request May 25, 2020

fix: AttestationStatement::TPM#valid? blowing up when using libssl 1.0.2 cedarcode/webauthn-ruby#314

Merged

2 tasks

santiagorodriguez96 requested a review from grzuy May 26, 2020 12:33

santiagorodriguez96 added 2 commits May 26, 2020 11:53

refactor: configure travis to test against both openssl 1.1 and 1.0

f4e642d

santiagorodriguez96 force-pushed the fix-key-attestation-blowing-up-in-openssl-1.0.2 branch from 4ce48c9 to 868c6c0 Compare May 26, 2020 14:53

santiagorodriguez96 merged commit d7aa850 into master May 26, 2020

santiagorodriguez96 deleted the fix-key-attestation-blowing-up-in-openssl-1.0.2 branch May 26, 2020 18:54

santiagorodriguez96 mentioned this pull request May 28, 2020

fix: race condition caused by the attribute not_after in a certificate being too short #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: KeyAttestation#key failing when using libopenssl v1.0.2 #5

fix: KeyAttestation#key failing when using libopenssl v1.0.2 #5

santiagorodriguez96 commented May 13, 2020 •

edited

Loading

santiagorodriguez96 May 13, 2020

grzuy May 21, 2020

grzuy left a comment

grzuy May 21, 2020

santiagorodriguez96 May 22, 2020 •

edited

Loading

grzuy May 24, 2020

grzuy left a comment

grzuy commented May 27, 2020

santiagorodriguez96 commented May 28, 2020

santiagorodriguez96 commented May 28, 2020 •

edited

Loading

santiagorodriguez96 commented May 28, 2020 •

edited

Loading

santiagorodriguez96 commented May 29, 2020

grzuy commented May 30, 2020

		- env: RB=ruby-head LIBSSL=1.0
		- env: RB=ruby-head LIBSSL=1.1

fix: KeyAttestation#key failing when using libopenssl v1.0.2 #5

fix: KeyAttestation#key failing when using libopenssl v1.0.2 #5

Conversation

santiagorodriguez96 commented May 13, 2020 • edited Loading

What

Why

How

How come that we only support the default exponent?

Steps taken

TBD

santiagorodriguez96 May 13, 2020

Choose a reason for hiding this comment

grzuy May 21, 2020

Choose a reason for hiding this comment

grzuy left a comment

Choose a reason for hiding this comment

grzuy May 21, 2020

Choose a reason for hiding this comment

santiagorodriguez96 May 22, 2020 • edited Loading

Choose a reason for hiding this comment

grzuy May 24, 2020

Choose a reason for hiding this comment

grzuy left a comment

Choose a reason for hiding this comment

grzuy commented May 27, 2020

santiagorodriguez96 commented May 28, 2020

santiagorodriguez96 commented May 28, 2020 • edited Loading

santiagorodriguez96 commented May 28, 2020 • edited Loading

santiagorodriguez96 commented May 29, 2020

grzuy commented May 30, 2020

santiagorodriguez96 commented May 13, 2020 •

edited

Loading

santiagorodriguez96 May 22, 2020 •

edited

Loading

santiagorodriguez96 commented May 28, 2020 •

edited

Loading

santiagorodriguez96 commented May 28, 2020 •

edited

Loading