search

davidearl / webauthn

An implementation of webauthn in PHP on the server side (e.g Yubico 2 and Google Titan keys)
https://webauthn.davidearl.uk
MIT License
130 stars 24 forks source link

incorrect check on domain #13

Closed vixducis closed 10 months ago

vixducis commented 5 years ago

In webauthnregister.js, following check is performed: if ('https://'+key.publicKey.rp.name != cd.origin) { return cb(false, 'key returned something unexpected (2)'); }

I know this project assumes that the rp.name and rp.id are identical and refer to the FQDN of where the project is hosted. However, the webauthn spec states the following: The RP ID must be equal to the origin's effective domain, or a registrable domain suffix of the origin's effective domain.

My use case requires the registration step and the authentication step to be on different subdomains of the same main domain, which makes me pass the main domain as the rp.id, which in turn makes the above check fail.

As browsers already check the rp.id requirement, I feel the above check is unnecessary and could be removed. Another option would be to perform a more advanced check conform to the spec.

AVGP commented 5 years ago

If I understand the spec correctly, the check for the origin & rpIdHash are mandatory steps, but I think a simple fix would be to check the publicKey.rp.id rather than the name?

vixducis commented 5 years ago

Checking publicKey.rp.id would be an improvement, but the problem goes a bit further than that. My specific use case requires the registration and authentication to happen on a different subdomain of the same main domain (so registration on a.example.com, authentication on b.example.com). This is allowed per the spec, if you pass 'example.com' as the rp.id. In this case the check would always fail because 'https://'+key.publicKey.rp.id would result in 'https://example.com', while cd.origin will contain 'https://a.example.com'. As a result, the check would still fail.

benjamindoe commented 5 years ago

Using publicKey.rp.id vs publicKey.rp.name doesn't make a difference for this library as they are both the same (webauthn.php#L72). However, I agree it would make more semantic sense to compare publcKey.rp.id against the suffix of cd.origin

Edit

The code has been refactored and you can now find the same line link here: https://github.com/davidearl/webauthn/blob/268cbc05e25c1e1444db2b961cdc042cd4534fc8/WebAuthn/WebAuthn.php#L85