Yubico / libfido2

Provides library functionality for FIDO2, including communication with a device over USB or NFC.
Other
590 stars 152 forks source link

Tests fail on x86 arch on Red Hat #692

Closed beldmit closed 1 year ago

beldmit commented 1 year ago

What version of libfido2 are you using? 1.13

What operating system are you running? RHEL 9.2

What application are you using in conjunction with libfido2? Native test suite

How does the problem manifest itself?

 make test
Running tests...
/usr/bin/ctest --force-new-ctest-process 
Test project /builddir/build/BUILD/libfido2-1.13.0/redhat-linux-build
    Start 1: regress_assert
1/8 Test #1: regress_assert ...................   Passed    0.01 sec
    Start 2: regress_cred
2/8 Test #2: regress_cred .....................Subprocess aborted***Exception:   0.31 sec
    Start 3: regress_dev
3/8 Test #3: regress_dev ......................   Passed   10.01 sec
    Start 4: regress_eddsa
4/8 Test #4: regress_eddsa ....................   Passed    0.01 sec
    Start 5: regress_es256
5/8 Test #5: regress_es256 ....................   Passed    0.01 sec
    Start 6: regress_es384
6/8 Test #6: regress_es384 ....................   Passed    0.01 sec
    Start 7: regress_rs256
7/8 Test #7: regress_rs256 ....................   Passed    0.01 sec
    Start 8: regress_compress
8/8 Test #8: regress_compress .................   Passed    0.00 sec

Is the problem reproducible? Yes

What are the steps that lead to the problem? cmake, make, make test

Does the problem happen with different authenticators?

Please include the output of fido2-token -L.

fido2-token -L
$ fido2-token -L

Please include the output of fido2-token -I.

fido2-token -I
$ fido2-token -I <device>

Please include the output of FIDO_DEBUG=1.

FIDO_DEBUG=1
$ export FIDO_DEBUG=1
$ <command1>
$ <command2>
(...)
$ <commandn>

beldmit commented 1 year ago

I'll try to reproduce it on a test machine but I need to setup it properly

beldmit commented 1 year ago

I've successfully reproduced it locally. It's related to RHEL/Fedora forbidding SHA1 for signatures. Will investigate tomorrow in more details

beldmit commented 1 year ago

The crash happens in tests, caused by https://github.com/Yubico/libfido2/blob/main/regress/cred.c#L2110 and https://github.com/Yubico/libfido2/blob/main/regress/cred.c#L2135. As SHA1 signatures are not supported in RHEL 9 (and can be forbidden in Fedora providing the necessary settings), I have 2 questions:

ikerexxe commented 1 year ago

@LDVG do you mind taking a look at this?

LDVG commented 1 year ago

Hi,

Thank you for looking into this. I agree with your assessment of the problem.

  • whether it's possible to use SHA2 hashes in the signature elaborated in fido_get_signed_hash_tpm

This function is involved in the process of verifying TPM Attestation Statements. The algorithm used to create the Attestation Signature is chosen by the authenticator itself and not something we are in control over. The only real world example we've seen so far has used RSASSA-PKCS1-v1_5 w/ SHA-1 (i.e. COSE specifier RS1).

  • what breaks if this function doesn't work properly?

Verification of (TPM) Attestation Signatures created using RS1.

If we can detect missing SHA-1 support, that part of the test could be skipped. Or, if SHA-1 can be (temporarily) enabled, do so for the duration of the tests.

beldmit commented 1 year ago

In OpenSSH I've implemented a downstream patch trying to sign smth using SHA1 with RSA key and, in case of failure, preserve this flag. There is no option to temporary switch it off, unfortunately.

But is there both signature and verification or verification only?

LDVG commented 1 year ago

But is there both signature and verification or verification only?

Could you elaborate on what you mean by this?

beldmit commented 1 year ago

Does this test only verifies the signature obtained from somewhere or also produces the signature?

LDVG commented 1 year ago

Does this test only verifies the signature obtained from somewhere or also produces the signature?

Thank you. No, the test does not produce the signature. The testing attestation statements containing RS1 signatures are regress/cred.c:attstmt_tpm_rs256 and regress/cred.c:attstmt_tpm_es256.

martelletto commented 1 year ago

Adding some context: the reason libfido2 needs to verify RSA signatures made with SHA-1 is that Microsoft is using RSA with SHA-1 to attest FIDO credentials stored in a TPM. Note that verification of a FIDO credential's attestation might happen in a different platform/OS than the one in which the credential was created.

beldmit commented 1 year ago

@martelletto thanks for your clarification! We can allow SHA1 verification on system-wide level if necessary, but for tests we use the default settings

PS. Looks like your Yubico email is not valid :(

martelletto commented 1 year ago

@beldmit Indeed, my yubico.com email no longer works. I also don't get any notifications on this repo, as they are all sent to that email. Please feel free to reach out to my ambientworks.net address (in this repo's commit history). Happy to help as always.

LDVG commented 1 year ago

Hi all,

Would it be helpful/make sense if we were to provide a toggle to skip the parts of the tests that exercise RS1 signature verification? This could then be used by packagers for systems that disallow SHA-1 use for signatures.

This could look something like

diff --git a/regress/cred.c b/regress/cred.c
index e4dc76ac..208d6bdf 100644
--- a/regress/cred.c
+++ b/regress/cred.c
@@ -9,6 +9,7 @@

 #include <assert.h>
 #include <string.h>
+#include <stdlib.h>

 #define _FIDO_INTERNAL

@@ -2094,7 +2095,7 @@ fmt_none(void)
 }

 static void
-valid_tpm_rs256_cred(void)
+valid_tpm_rs256_cred(int skip_rs1_verify)
 {
    fido_cred_t *c;

@@ -2107,7 +2108,8 @@ valid_tpm_rs256_cred(void)
    assert(fido_cred_set_uv(c, FIDO_OPT_TRUE) == FIDO_OK);
    assert(fido_cred_set_fmt(c, "tpm") == FIDO_OK);
    assert(fido_cred_set_attstmt(c, attstmt_tpm_rs256, sizeof(attstmt_tpm_rs256)) == FIDO_OK);
-   assert(fido_cred_verify(c) == FIDO_OK);
+   if (!skip_rs1_verify)
+       assert(fido_cred_verify(c) == FIDO_OK);
    assert(fido_cred_prot(c) == 0);
    assert(fido_cred_pubkey_len(c) == sizeof(pubkey_tpm_rs256));
    assert(memcmp(fido_cred_pubkey_ptr(c), pubkey_tpm_rs256, sizeof(pubkey_tpm_rs256)) == 0);
@@ -2119,7 +2121,7 @@ valid_tpm_rs256_cred(void)
 }

 static void
-valid_tpm_es256_cred(void)
+valid_tpm_es256_cred(int skip_rs1_verify)
 {
    fido_cred_t *c;

@@ -2132,7 +2134,8 @@ valid_tpm_es256_cred(void)
    assert(fido_cred_set_uv(c, FIDO_OPT_TRUE) == FIDO_OK);
    assert(fido_cred_set_fmt(c, "tpm") == FIDO_OK);
    assert(fido_cred_set_attstmt(c, attstmt_tpm_es256, sizeof(attstmt_tpm_es256)) == FIDO_OK);
-   assert(fido_cred_verify(c) == FIDO_OK);
+   if (!skip_rs1_verify)
+       assert(fido_cred_verify(c) == FIDO_OK);
    assert(fido_cred_prot(c) == 0);
    assert(fido_cred_pubkey_len(c) == sizeof(pubkey_tpm_es256));
    assert(memcmp(fido_cred_pubkey_ptr(c), pubkey_tpm_es256, sizeof(pubkey_tpm_es256)) == 0);
@@ -2146,6 +2149,8 @@ valid_tpm_es256_cred(void)
 int
 main(void)
 {
+   int skip_rs1_verify = getenv("FIDO_REGRESS_NO_RS1_VERIFY") != NULL;
+
    fido_init(0);

    empty_cred();
@@ -2172,8 +2177,8 @@ main(void)
    wrong_credprot();
    raw_authdata();
    fmt_none();
-   valid_tpm_rs256_cred();
-   valid_tpm_es256_cred();
+   valid_tpm_rs256_cred(skip_rs1_verify);
+   valid_tpm_es256_cred(skip_rs1_verify);

    exit(0);
 }

where, if applicable, tests would have to be run with

$ env FIDO_REGRESS_NO_RS1_VERIFY= make test

or equivalent.

beldmit commented 1 year ago

I'd support this solution

LDVG commented 1 year ago

We merged a slight variation of this, thank you.