Validation asumes IdP entityId contains a slash?

[lonoak]

Hi!

if I'm correct reading this code:

https://github.com/simplesamlphp/simplesamlphp-module-webauthn/blob/71eb34354aebbd27724b0e1273d7a372287ea600/lib/WebAuthn/WebAuthnAbstractEvent.php#L281

..the module expects the IdP entityId to contain a slash after the domain name... problem is... what if there's only a domain name (no slashes) as in my case? or what if the entityId is an URN (which is perfectly valid to my known).

Could also be I'm not understanding the code... :)

I see a way to workaround this for my use case, but just commenting here to discuss a more general solution, unless I misunderstood the comparison being made to detect an invalid origin.

Greetings,

Jose.

[restena-sw]

Thanks for this in-depth question :-)

The origin as sent by the client conforms to RFC6454 and thus includes a scheme (typically "https://"), a hostname (say "foo.bar.com") and an optional port (say ":443").

The code then compares it to "the host simpleSAMLphp is running on". This is actually quite tricky, if you want to avoid trusting HTTP headers which are possibly subject to manipulation by the client.

The best I could find was that the webauthn plugin is only ever executed in a SAML IdP context, meaning that the identity of "self" is determined by the configured IdP that is handling the request. Unfortunately, the local IdP config does not include a hostname; it contains the IdP's entityID instead.

It is a bit of a hack to construct the expected Origin from the SAML metadata entityId, and it only works under the assumption that said entityId is of the form scheme+hostname(+optional port).

With all that in mind,

• it would be easy to change the trailing slash detection with an alternative "if no trailing slash, take entityId as-is". I guess this would solve one of your two use cases.
• it is much harder to fix a URN-style entityId because there is no reliable source of hostname info in that case.

To validate the first dash, you could replace line 284 with

$expectedOrigin = ($slash !== false) ? substr($this->idpEntityId, 0, $slash) : $this->idpEntityId;

If you want fix the second dash's more profound problem, we'd need to get reliable scheme, host and port information programmatically without involving unvetted header information. If you have an idea, please let me know!

[lonoak]

Thanks Stefan!

Do you think the 'host' param from the hosted IdP metadata could be used for this comparison?

Although I don't know if this information is available at this moment...

[restena-sw]

That gives the "host" part of the expected origin (if one stops using DEFAULT anyway). We also need "scheme" and in case the endpoint is on a custom port, the port info.

[tvdijen]

The code then compares it to "the host simpleSAMLphp is running on". This is actually quite tricky, if you want to avoid trusting HTTP headers which are possibly subject to manipulation by the client.

This is why we have the baseurlpath-setting where you have to configure the actual public url. All our methods on SimpleSAML\Utils\HTTP consider this setting, so if it's properly configured, you should get reliable scheme, host, port, etc.

[restena-sw]

Hm. according to the docs, specifying the scheme and hostname bits is optional. (And I have only ever specified the path component of it, only now realised that a full URI is possible)

Is it considered common that deployments specify the full URL there? It would be a bit sad if the webauthn module is the only one actually requiring that otherwise optional part, and therefore surprise-breaking stuff for many.

IOW: could baseurlpath be made more predictable, i.e. making the full URI required in all cases? More determinism is almost always better.

[tvdijen]

Is it considered common that deployments specify the full URL there?

Yes, especially setups with a reverse proxy involved will need to set this to a full URL, if the inside hostname is different from the outside (public) hostname, so it's perfectly fine to rely on this setting.

IOW: could baseurlpath be made more predictable, i.e. making the full URI required in all cases? More determinism is almost always better.

Just use SimpleSAML\Utils\HTTP::getBaseURL and you are guaranteed to get a full URI back. If you only specify a path in the config-setting, SSP will try it's best to guess the rest.. If anything goes wrong, we can be fairly sure the deployer didn't properly configure his SSP instance

[lonoak]

I'm in this situation (behind a load balancer, and using a baseurlpath value containing the scheme), so I'm happy to test if you need.

[restena-sw]

Tried that and it works :-) @lonoak if you want to give it a spin, here is the fairly trivial diff:

--- WebAuthnAbstractEvent.backup.php    2022-03-08 16:42:18.575942901 +0100
+++ WebAuthnAbstractEvent.php   2022-03-08 20:30:33.455253810 +0100
@@ -7,6 +7,7 @@
 use CBOR\Tag;
 use CBOR\StringStream;

+use SimpleSAML\Utils\HTTP as HTTPHelper;
 /**
  * FIDO2/WebAuthn Authentication Processing filter
  *
@@ -272,8 +273,8 @@
          * §7.1 STEP 6 : check if incoming origin matches our hostname (taken from IdP metadata prefix)
          * §7.2 STEP 10: check if incoming origin matches our hostname (taken from IdP metadata prefix)
          */
-        $slash = strpos($this->idpEntityId, '/', 8);
-        $expectedOrigin = ($slash !== false) ? substr($this->idpEntityId, 0, $slash) : $slash;
+       $slash = strpos(HTTPHelper::getBaseURL(), '/', 8);
+        $expectedOrigin = ($slash !== false) ? substr(HTTPHelper::getBaseURL(), 0, $slash) : $slash;
         if ($clientData['origin'] === $expectedOrigin) {
             $this->pass("Origin matches");
         } else {

[tvdijen]

This seems like a bug, because $slash is an integer (return-value of strpos), but substr returns a string. It's now basically reading: $expectedOrigin = ($slash !== false) ? string : int;.. That can't be right..

Also, getBaseURL() is guaranteed to end with a slash, so if you want to locate the slash, you could cheat by doing $slash = strlen($this->idpEntityId);

[restena-sw]

The expression now is unnecessarily convoluted, but not buggy.

substr returns either an integer (if found) or FALSE (if not found). So $slash is a boolean FALSE or an integer.

The ternary operator assigns the string with the origin, cutting at the slash or it assigns the value FALSE.

$clientData['origin'] is delivered by the authenticator, and never FALSE.

So the "if" clause compares the strings or compares a string to FALSE. It only passes the origin check if it compared two strings, and then only if the two strings were identical. In all other cases, it returns failure. Which is the expected validation procedure.

[tvdijen]

Nope, substr returns a string: https://www.php.net/manual/en/function.substr.php#refsect1-function.substr-returnvalues I think you're mixing up with strpos..

[restena-sw]

Yes, typo in the argumentation. strpos returns a FALSE if not found; subsequently substr() is only called in the case that the slash WAS found, and $slash is an integer. otherwise, it is FALSE.

(in your post you were slightly incorrect in that the statement is basically:

$expectedOrigin = ($slash !== false) ? string : FALSE;

With that, $expectedOrigin is either the string containing the origin (that will then be compared with the authenticator response) or is FALSE, which makes the authenticator validation fail. Both is intended.

[tvdijen]

Ahh, now I see! Forget what I said!

[restena-sw]

@lonoak did you ever try the patch from 8 Mar above? Is that okay to include in a future release?

[restena-sw]

The fix with getBaseURL is now in since more than a year and part of multiple releases. Closing this as completed.