lbuchs / WebAuthn

A simple PHP WebAuthn (FIDO2/Passkey) server library
https://webauthn.lubu.ch
MIT License
419 stars 75 forks source link

_checkOrigin does not allow for complex localhost domains #64

Open tlokot opened 1 year ago

tlokot commented 1 year ago

My local dev environment makes use of complex localhost FQDN's such as ..localhost (eg. prod.abc.localhost)

I had to alter line 583 in WebAuthn.php to the following to allow these domain names to work:

if ($this->_rpId !== 'localhost' && !\str_ends_with($this->_rpId, '.localhost') && \parse_url($origin, PHP_URL_SCHEME) !== 'https') {
            return false;
}

Is anyone able to check this for security/compliance issues and submit a pull request (I'm happy to submit if someone can point me in the right direction)? I was able to use a Yubikey and TouchID locally after making this change.

lbuchs commented 1 year ago

localhost is not supposed to have any subdomains. To do so violates the approved RFC standards.

tlokot commented 1 year ago

I can appreciate that at an RFC level, but the localhost exception exists for local development and not everyone has the luxury of putting every development environment on the exact word "localhost". Even though it is against the RFC, MacOS, Windows and Linux all support mapping random aliases to localhost via the host files (or equivalent) and all major web server software matches those names just fine. Because of these mechanisms, the concept of "local" could just as easily be "prod.abc.local" or "prod.abc.dev", etc so long as they all map to 127.0.0.1

One alternative might be to introduce a dnslookup, but this creates too many potential problems for a live server.

Looking around at other libraries and how they've "solved" this issue, what do you think about developers being able to pass through a list of "safe" or "exception" domains, where localhost is always part of that list. eg. https://webauthn-doc.spomky-labs.com/pure-php/advanced-behaviours/dealing-with-localhost#the-hard-way

This keeps the onus of "secure" being placed on the developer, without introducing any dns lookups, while giving the developer control over their environment. It also removes a hard-coded assumption in the code. The code would then become something like:

if (!\in_array($this->_rpId, $secureDomains) && \parse_url($origin, PHP_URL_SCHEME) !== 'https') {
            return false;
}

where $secureDomains is defaulted to ['localhost'] and the developer can add to that list somehow?

Would a solution along these lines still satisfy the spirit of why this exception was created in the first place?

tlokot commented 1 year ago

@lbuchs Rather than altering the way _checkOrigin works internally, would you consider raising the internal visibility from private to protected for all methods and properties in WebAuthn.php so it can be easily extended and/or overridden? This would remove any compliance burden from you while making it easier to work with and it wouldn't alter the integrity of your library architecture.

CrownFish-Hsu commented 11 months ago

@lbuchs Android's request json origin is package name, not https domain. so check origin failed, is right?