Yubico / java-webauthn-server

Server-side Web Authentication library for Java https://www.w3.org/TR/webauthn/#rp-operations
Other
443 stars 139 forks source link

FIDO Metadata verification ignores trustRootCertificate? #294

Closed mmoayyed closed 1 year ago

mmoayyed commented 1 year ago

Using v2.4.1.

Earlier in the morning GST, FIDO metadata verification processes started to fail:

Caused by: java.security.cert.CertPathValidatorException: validity check failed
    at java.base/sun.security.provider.certpath.PKIXMasterCertPathValidator.validate(PKIXMasterCertPathValidator.java:135)
    at java.base/sun.security.provider.certpath.PKIXCertPathValidator.validate(PKIXCertPathValidator.java:224)
    at java.base/sun.security.provider.certpath.PKIXCertPathValidator.validate(PKIXCertPathValidator.java:144)
    at java.base/sun.security.provider.certpath.PKIXCertPathValidator.engineValidate(PKIXCertPathValidator.java:83)
    at java.base/java.security.cert.CertPathValidator.validate(CertPathValidator.java:309)
    at com.yubico.fido.metadata.FidoMetadataDownloader.verifyBlob(FidoMetadataDownloader.java:1097)
    at com.yubico.fido.metadata.FidoMetadataDownloader.parseAndVerifyBlob(FidoMetadataDownloader.java:1018)
    at com.yubico.fido.metadata.FidoMetadataDownloader.refreshBlobInternal(FidoMetadataDownloader.java:807)
    at com.yubico.fido.metadata.FidoMetadataDownloader.refreshBlob(FidoMetadataDownloader.java:795)
    ... 144 more
Caused by: java.security.cert.CertificateExpiredException: NotAfter: Wed Jun 14 01:26:44 GST 2023
    at java.base/sun.security.x509.CertificateValidity.valid(CertificateValidity.java:277)
    at java.base/sun.security.x509.X509CertImpl.checkValidity(X509CertImpl.java:627)
    at java.base/sun.security.provider.certpath.BasicChecker.verifyValidity(BasicChecker.java:190)
    at java.base/sun.security.provider.certpath.BasicChecker.check(BasicChecker.java:144)
    at java.base/sun.security.provider.certpath.PKIXMasterCertPathValidator.validate(PKIXMasterCertPathValidator.java:125)
    ... 154 more

I tracked this down to the following block in FidoMetadataDownloader:

    if (header.getX5u().isPresent()) {
      // Download cert chain
    } else if (header.getX5c().isPresent()) {
      certChain = header.getX5c().get();
    } else {
      certChain = Collections.singletonList(trustRootCertificate);
    }

If I am reading this correctly, this indicates that trustRootCertificate potentially can never be explicitly downloaded and used for verification processes if one of the above two conditions holds true? I can probably understand that the intent here is to allow for a seamless and quick fetching of the cert chain, allowing the blob to control the cert chain but I am not sure how we can get around a failure such as the one above, if there are delays or other issues from upstream.

If an explicit trustRootCertificate is configured, should it not always be used, regardless of what else exists? Or rather, use the given trust root certificate first, and only fallback if validation fails?

emlun commented 1 year ago

Hi! First off, make sure to use the right version of the source code when checking line numbers. Line 1097 in v2.4.1 is not the same as line 1097 on the current main branch: https://github.com/Yubico/java-webauthn-server/blob/2bebcbb5d6a8a2b82be8eb2843e8855fc75d83c6/webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java#L1097

So this has nothing to do with the trust root cert. Rather, the BLOB currently served by the FIDO MDS uses a leaf cert that expired last night (UTC). You can see this for yourself by inspecting the certificate:

$ curl -L https://mds3.fidoalliance.org/ | cut -d '.' -f 1 | basenc -d --base64url | jq -r .x5c[0] | basenc -d --base64 | openssl x509 -text -noout

So it seems quite correct that the downloader fails to verify the newly downloaded BLOB, at least.

But I see now that there's a pretty bad problem here: the library also attempts to verify the cached BLOB on every load. That verification should probably be dropped, as there's no sense discarding the cached BLOB if it was successfully verified once, just because the cert expired afterwards. This will need an update of the library to drop the signature verification on cache loads. I'll get on that ASAP.

mmoayyed commented 1 year ago

Thank you @emlun for clarifying the code blocks :)

I can see that even in the main branch, my previous analysis holds true[?]. The leaf cert is picked out as the first entry in the certChain. The cert chain is calculated using the same method as before, in my original post above. So, if I have provided my trustRootCertificate explicitly, how can I make sure the validation will use mine as the basis for the cert chain from which the leaf will get selected? I don't see how I can override this, or am I totally off here?

Ref: https://github.com/Yubico/java-webauthn-server/blob/main/webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java#L1075-L1099

emlun commented 1 year ago

Your given trust root certificate will always be used, see further down where the CertPathValidator is configured: https://github.com/Yubico/java-webauthn-server/blob/f207b279bdf740abfc0618398c92bc2cea2557e6/webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java#L1130

The logic you linked to corresponds to steps 4 and 5 of the FIDO MDS Metadata BLOB object processing rules. The certChain variable is rather the untrusted cert chain up to but not including the trust root cert, except in the last case where the trust root cert was directly used to sign the BLOB (which is usually not the case).

mmoayyed commented 1 year ago

I see what you mean; thank you!

emlun commented 1 year ago

In other news, FIDO MDS now serves a new BLOB with a new, valid certificate, so you should be able to resolve your issues by refreshing the BLOB. We'll still ship the fix in PR #295 eventually to prevent issues like this in the future.

emlun commented 1 year ago

The fix from PR #295 is now available in pre-release 2.5.0-RC1.