kanidm / webauthn-rs

An implementation of webauthn components for Rustlang servers
Mozilla Public License 2.0
483 stars 80 forks source link

Unknown transports #364

Closed smessmer closed 11 months ago

smessmer commented 11 months ago

Is your feature request related to a problem? Please describe. We have run into clients that send "cable" as a transport mechanism. This is not part of the webauthn spec but seems to happen in practice. Looking at the webauth spec, it says relying parties must accept and ignore invalid transport values. The current behavior of webauthn-rs is to crash instead because the credential can't be deserialized.

Describe the solution you'd like Deserialization of a PublicKeyCredential should not crash when unknown transports are present.

Firstyear commented 11 months ago

We should add cable as a mechanism then I think here.

yaleman commented 11 months ago

Deserialization of a PublicKeyCredential should not crash when unknown transports are present.

Where's the crash coming from? Hopefully it's just throwing an error?

smessmer commented 11 months ago

Yes, by "crash" I meant deserialization returns an Err value. It's not a panic.

Adding "cable" would solve this concrete instance of the issue, but if I'm reading the spec correctly, I think it calls for just allowing (and silently ignoring) any unknown strings in there, other methods might come up later.

Firstyear commented 11 months ago

Good it's not a panic, we wouldn't want that!

I think we need to set #[serde(other)] on a variant that says something like "invalid" or similar. That way we handle this case.

yaleman commented 11 months ago

The values SHOULD be members of AuthenticatorTransport but Relying Parties MUST ignore unknown values.

Yep, the spec says to just ignore it when the client sends nonsense.

Firstyear commented 11 months ago

Yep, the spec says to just ignore it when the client sends nonsense.

That's not always a reason to do something though. But in this case I'd rather add cable as a transport, and then invalid to catch other busted devices.

micolous commented 11 months ago

“cable” isn’t a standard transport label, so we should ignore that. From memory Android will present cable + hybrid + internal over caBLE, so we already catch it.

And yeah, Safari made up their own “smart-card” transport at one point (distinct from “nfc” for contact interfaces, unlike what Windows did and we did) which has since been standardised.

yaleman commented 11 months ago

Yep, the spec says to just ignore it when the client sends nonsense.

That's not always a reason to do something though. But in this case I'd rather add cable as a transport, and then invalid to catch other busted devices.

Sorry I meant we don't have to throw an error, we can set/flag it as invalid etc on unknown.

Firstyear commented 11 months ago

“cable” isn’t a standard transport label, so we should ignore that. From memory Android will present cable + hybrid + internal over caBLE, so we already catch it.

And yeah, Safari made up their own “smart-card” transport at one point (distinct from “nfc” for contact interfaces, unlike what Windows did and we did) which has since been standardised.

It's all a bit pointless anyway, as transports don't work properly in most browsers for device selection, and if they do, it's just likely to confuse users to why some devices do/don't work. Also transports aren't always set properly, like a yk 5 nfc is only sent with transport=usb, not transports=usb,nfc, so if an RP were to persist/filter on this, it won't properly show all the transports.

This is why we actually explicitly disabled the transport population code in assertions, because there were too many cases that legitimate authenticators would end up not working immediately after registration.

It's certainly one of those things of "standard as written" and "standard as used" and there is a lot of surface area in webauthn that exists, but doesn't work, and shouldn't be used.

Firstyear commented 11 months ago

Anyway, I'll PR a fix to this today. :)

smessmer commented 11 months ago

Thank you :)