Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed to parse MDS #462

Closed
sycamoreoak opened this issue Dec 19, 2024 · 15 comments
Closed

Failed to parse MDS #462

sycamoreoak opened this issue Dec 19, 2024 · 15 comments

Comments

@sycamoreoak
Copy link

sycamoreoak commented Dec 19, 2024

I did this

cd compat_tester/webauthn-rs-demo
cargo run 

(same issue for release mode)

I expected the following

webauthn demo go brr

What actually happened

2024-12-19T16:52:51.281585Z  INFO webauthn_rs_demo: Fetching MDS ...
2024-12-19T16:52:52.163261Z ERROR webauthn_rs_demo: Failed to parse MDS err=Serde

Version (and git commit)

commit 1347c6c

Operating System / Version

latest and greatest ubuntu

Any other comments

another FIDO schema change?

@sycamoreoak
Copy link
Author

maybe we could get some CI here to mitigate this in the future as well? In looking at the issues it seems that this has happened a number of times, and the MDS format is being updated fairly regularly.

@Be-ing
Copy link

Be-ing commented Dec 19, 2024

I can reproduce this:

webauthn-rs (1347c6c) [?] is 📦 v0.5.0 via 🦀 v1.83.0 on ☁  (auto) 
❯ cargo run --bin fido-mds-tool fetch
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.30s
     Running `target/debug/fido-mds-tool fetch`
2024-12-19T18:37:40.225225Z  INFO fido_mds_tool: Fetching from https://mds.fidoalliance.org/ to "/tmp/mds.blob.jwt"
^[[A2024-12-19T18:37:40.762629Z  INFO fido_mds_tool: Ok!

webauthn-rs (1347c6c) [?] is 📦 v0.5.0 via 🦀 v1.83.0 on ☁  (auto) 
❯ cargo run --bin fido-mds-tool query -o "status gte valid" > trusted-authenticators
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.31s
     Running `target/debug/fido-mds-tool query -o 'status gte valid'`
2024-12-19T18:37:45.057194Z ERROR fido_mds_tool: e=Serde

@sycamoreoak
Copy link
Author

sycamoreoak commented Dec 19, 2024

what about having the option to switch on serde deny_unknown_fields otherwise by default use serde flatten when parsing?

@Firstyear
Copy link
Member

I already fixed this in a pr,

We can't really switch these on or off at runtime :(

@sycamoreoak
Copy link
Author

sycamoreoak commented Dec 20, 2024

Not at runtime--I meant switch off deny_unknown_fields at compile time... I am assuming that substantially all of the breakage is FIDO alliance/MDS folks adding fields, and then the less strongly typed languages being okay with it but rust complaining?

@Firstyear
Copy link
Member

Firstyear commented Dec 20, 2024

maybe we could get some CI here to mitigate this in the future as well? In looking at the issues it seems that this has happened a number of times, and the MDS format is being updated fairly regularly.

The problem here btw is that fido are updating the MDS format without announcement and without updating their own MDS format specification. They are simply, "yolo, yeeting" new MDS values when they feel like.

We also think given that this is meant to be the equivalent of a "CA root", strict parsing should be the norm. It's really important we get this right.

But since we apparently are the only tool in the world that exists outside of fido to parse the MDS, we get the reports when it fails. When you run the tool, you can see all the errors that still exist in the MDS. We reported all of them to fido! They've had years to fix basic data entry issues and they simply haven't! So now they're just throwing out now MDS fields for the laughs, and we have to play whack a mole and respond, with no communication or recourse.

The final nail here is we want to help fido but they won't let us because I as an individual would have to pay a significant sum of money to join, just so that I can actually have these issues heard.

less strongly typed languages being okay with it but rust complaining?

We doubt any other parsers exist given the sheer volume of issues we found including broken authenticator CA certs and undocumented fields. We suspect we are the only parser that exists at all.

@sycamoreoak
Copy link
Author

sycamoreoak commented Dec 20, 2024

MDS format without announcement and without updating their own MDS format specification.

CI won't handle this situation and catch the yeets?

We suspect we are the only parser that exists at all.

haha, that's why I was saying to try relaxing things... maybe have separate "strict" and "nonstrict" parsers and try to make the strict parsing failure more of a warning? Weak/duck typing is a thing and this may simply come down to different ways of approaching engineering. In other systems I have run into a mismatch between the rusty way and folks coming from a javascript enterprise edition/python/etc world.

Also not to be a nudge but I see the pr now, is there an issue with merging it?

So now they're just throwing out now MDS fields for the laughs.

Right so the changes are simply additional fields, assuming "now" there means "new"?

@Firstyear
Copy link
Member

Yeah, I'm merging it today and releasing too.

@sycamoreoak
Copy link
Author

So if FIDO changes the MDS and how long until a cached MDS becomes invalid? Can a change in the MDS cause a functioning passkey authentication system to grind to a halt? Is there a repo somewhere of older but still valid MDS files?
I suppose one would need to be sure to have a cached (and probably backed up copy) of the MDS?

@Firstyear
Copy link
Member

"that depends". If you're always pulling the latest mds and applying it, yes, you will cause a system to grind to a halt. I think the way most people are using this is "intermittent updates" of the MDS so they don't have these issues.

And as well, depends on your threat model - the main reason you would stress about MDS updates is the risk of a compromised authenticator, so you want to be able to remove it from the attestation lists ASAP. But depending on your risk profile, that also may not need to happen rapidly etc. So it's hard to say what the right answer is here.

@sycamoreoak
Copy link
Author

sycamoreoak commented Dec 20, 2024

the risk of a compromised authenticator

As in an entire product line, right?

Do javascript implementations which consume the MDS also break when they change it?

@Firstyear
Copy link
Member

Do javascript implementations which consume the MDS also break when they change it?

I am not aware of any other MDS parser beside this one. Even fido were "shocked" that I wrote a parser when I told them. So I don't know.

As in an entire product line, right?

Yes. There are multiple compromised devices already that should not be used. Part of what makes this hard is that the MDS kind of "lies"about the situation, which is that if any firmware version of a device is compromised, you have to exclude all versions of that device, even if they have fixed firmware. The reason is that attestation doesn't provider the fw version to the provider, so you have to assume all devices are the broken one. There are some devices like yubikeys that have custom x509 fields for this, and I think there was some moves to add a fw field in the attestation data, but that doesn't help with all the devices that already exist and don't report their fw version.

@sycamoreoak
Copy link
Author

sycamoreoak commented Dec 20, 2024

The merge (22d8fdd) worked and the specific issue referenced here is fixed, so thanks very much.

It seems like there is a real need to make webauthn-rs tolerant to more changes in the MDS schema. Another problem here is there is no official storage location for older MDS files. I will open a separate issue for that.

@Firstyear
Copy link
Member

The merge worked and the specific issue referenced here is fixed, so thanks very much.

No problem. Sorry to seem so "negative" about this, but this has been an uphill battle with fido for a long time, and the vibe we get from them is they simply don't care unless you are a mega-corp who pays them money. :(

@Be-ing
Copy link

Be-ing commented Dec 20, 2024

I am not aware of any other MDS parser beside this one. Even fido were "shocked" that I wrote a parser when I told them. So I don't know.

AFAIK Kanidm and Keycloak are the only IdPs (nevermind server applications) that verify Webauthn attestation. Is anyone else doing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants