Allow valid_origin? to accept subdomains

[Oliphaunte]

I am trying to process webauth requests on my primary and subdomains. The current checks are very strict on the domain, but setting the rp_id should allow you to process subdomains as well. The change I am proposing should abide by the webauth rules about origin/rp_id and accomplish the following:

• Credentials bound to RP ID "example.com" can be used to authenticate on https://example.com/ and https://subdomain.example.com/
• Credentials bound to "subdomain.example.com" can be used on https://subdomain.example.com/ but NOT on https://example.com/
• Credentials bound to "example.com" CANNOT be used on https://example.org/

There is probably a better way to accomplish this though, open to suggestions!

[tanguilp]

Thanks for the PR! Can you point out the section of the spec explaining what we can do (and can't!) with the origin?

As this is a security-related library, we need to be extra careful with any change affecting the various checks we do. Keep in mind this library was created from the first version of the spec, now we are at level 3. Maybe some things related to origin have changed since them. We need to check all the specs and changes on this topic. Thanks!

[Oliphaunte]

Sure! I'm going off the W3 spec:

• https://www.w3.org/TR/webauthn-3/#sctn-validating-origin
• https://www.w3.org/TR/webauthn-3/#rp-id

Specifically this part:

A web application served at a large set of domains that changes often might parse origin structurally and require that the URL scheme is https and that the authority equals or is any subdomain of the RP ID - for example, example.org or any subdomain of example.org).

Although re-reading the spec, this seems like a place where you sacrifice security for flexibility, as basically any subdomain becomes a vector for attack. Frankly, I'm confused why this would be allowed without some mitigating solution in the spec.

What I now think might be better is is just take in a list of approved domains instead, which is also part of the spec:

A web application served at a small number of domains might require origin to exactly equal some element of a list of allowed origins, for example the list ["https://example.org", "https://login.example.org"].

[tanguilp]

Better use the boolean and operator here. Ensure to run Dialyzer even though I think there are some errors.

[tanguilp]

Hi again and sorry for the late reply. Thanks for the proposed contribution.

What I now think might be better is is just take in a list of approved domains instead, which is also part of the spec:

I propose all of the options: the atom :auto, a domain, a domain with a wildcard and a list of domains (without wildcard).

I've left a few comments.

Talking about origin, it would be nice if this PR could support mobile origins as well. See https://github.com/joshrieken/wax/tree/mobile-support for what need to be done. Maybe @joshrieken can help here?

[Oliphaunte]

Sounds good! I can update the PR to account for all them, thanks!

I'll take a look into the mobile origins as well, but may defer that depending on future conversation

[tanguilp]

Yeah the mobile stuff might belong to another PR. Still it would be interesting to understand what was done there and why

[tanguilp]

Looks like one of my comments didn't get through.

I think we would accept a challenge for example.com signed by evil-site-example.com. This is a serious security vulnerability.

Treating origin must be done carefully. Please feel free to get inspiration from Phoenix https://github.com/phoenixframework/phoenix/blob/48ee8cc407f060eee1152f2c8f64bff49a6b4f15/lib/phoenix/socket/transport.ex#L583-L609

When accepting PRs for security libraries, I'd prefer the author to be a public identifiable person. It's an additional protection against introduction of vulnerabilities. Please consider creating a PR with such a profile.