Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.2.1] Custom Relying Party Id does not pass CheckRelyingPartyIdContained pipe #50

Closed
2 of 3 tasks
Bubka opened this issue Aug 25, 2023 · 4 comments · Fixed by #65
Closed
2 of 3 tasks

[1.2.1] Custom Relying Party Id does not pass CheckRelyingPartyIdContained pipe #50

Bubka opened this issue Aug 25, 2023 · 4 comments · Fixed by #65
Assignees
Labels
bug Something isn't working

Comments

@Bubka
Copy link
Contributor

Bubka commented Aug 25, 2023

PHP & Platform

8.1.22

Database

No response

Laravel version

10.16.1

Have you done this?

  • I have checked my logs and I'm sure is a bug in this package.
  • I can reproduce this bug in isolation (vanilla Laravel install)
  • I can suggest a workaround as a Pull Request

Expectation

Setting a custom Relying Party Id in the .env file should allow to register new webauthn devices.

Description

Following the Laragear/webauthn documentation:

WebAuthn/README.md

Lines 605 to 609 in 7e62ec9

Instead of modifying the config file, you should use the environment variables to set the name and ID for WebAuthn.
```dotenv
WEBAUTHN_NAME=SecureBank
WEBAUTHN_ID=https://auth.securebank.com

If I set WEBAUTHN_ID=https://my.domain.com the registration ceremony fails because the RP-ID is not valid. Indeed, regarding the Webauthn W3C recommandation, the RP ID should be a domain, not an URL. Using such an URL makes the webauthn API throwing a SecurityError (see https://www.w3.org/TR/webauthn-2/#CreateCred-DetermineRpId)

But if I set WEBAUTHN_ID=my.domain.com, the registration ceremony also fails but this time because the CheckRelyingPartyIdContained does not pass because of the way $current is defined:

$current = parse_url(
$this->config->get('webauthn.relying_party.id') ?? $this->config->get('app.url'), PHP_URL_HOST
);

Using parse_url() with the PHP_URL_HOST flag will return nothing, causing the next evaluation to fail:

if (hash_equals($current, $host) || Str::is("*.$current", $host)) {
return $next($validation);
}

Reproduction

// Set WEBAUTHN_ID=my.domain.com and try to register a new device

Stack trace & logs

No response

@DarkGhostHunter
Copy link
Contributor

DarkGhostHunter commented Aug 29, 2023

Okay. Basically, I need to fix this by extracting the domain from the APP_URL, and if it's empty, use WEBAUTHN_ID verbatim if it doesn't start with https://.

@asivaneswaran
Copy link

asivaneswaran commented Sep 2, 2023

I am going to add also other steps.

So I was able to get through this (for now) by doing this:

$current = parse_url(
            'http://' . $this->config->get('webauthn.relying_party.id') ?? $this->config->get('app.url'), PHP_URL_HOST
        );

I would suggest maybe adding a config for this or something. But I run into another issue here:

 public function hasNotSameRPIdHash(string $relyingPartyId, bool $hash = true): bool
    {
        return ! $this->hasSameRPIdHash($relyingPartyId, $hash);
    }

File: Attestation/AuthenticatorData Line 117
Because the relyingPartId is null and it comes from Attestation/Validator/Pipes/CheckRelyingPartyHashSame.php Line 39:

    protected function relyingPartyId(AssertionValidation|AttestationValidation $validation): string
    {
        return 'http://' . $this->config->get('webauthn.relying_party.id') ?? $this->config->get('app.url');
    }

This registers properly. I know this isn't the cleanest, but I had to work on my development while waiting on a fix.

EDIT:
Also need to change this in Assertion/Validator/Pipes/CheckRelyingPartyHashSame.php:

/**
   * Return the Relying Party ID from the config or credential.
   *
   * @param  \Laragear\WebAuthn\Assertion\Validator\AssertionValidation|\Laragear\WebAuthn\Attestation\Validator\AttestationValidation  $validation
   * @return string
   */
  protected function relyingPartyId(AssertionValidation|AttestationValidation $validation): string
  {
      return 'http://' . $validation->credential->rp_id;
  }
  ```

@asivaneswaran
Copy link

@DarkGhostHunter Will you be able to create the fix for this or would you like me to create a PR for this as well?
Thanks!

@DarkGhostHunter
Copy link
Contributor

Please, help me with the PR. Currently hands full.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants