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

Logging in with username and password triggers exception in oauth2 and logs password in plain text #55

Open
peratoner-louis opened this issue Nov 20, 2024 · 1 comment

Comments

@peratoner-louis
Copy link

peratoner-louis commented Nov 20, 2024

TYPO3: >=12
OAuth2: >=3.1.0

The method initializeResourceServer() gets called in getUser() even when other login providers are used to log in:

Classes/Services/OAuth2LoginService.php:90

if (!$this->initializeResourceServer($request)) {

Since the resource-server-identifier parameter does not exist when logging in using the default UsernamePasswordLoginProvider, Registry::getResourceServerInstance throws an "" has not been registered as a ResourceServer exception:

Classes/ResourceServer/Registry.php:61

public static function getResourceServerInstance(string $identifier): AbstractResourceServer
{
    if (!array_key_exists($identifier, self::$registry)) {
        $message = sprintf('"%s" has not been registered as a ResourceServer', $identifier);
        throw new NotRegisteredException($message, 1558815703);
    }

Resulting in the try-catch to fail and logging an error message:

Classes/Services/OAuth2LoginService.php:133

protected function initializeResourceServer(ServerRequestInterface $request): ?AbstractResourceServer
{
    try {
        $resourceServerIdentifier = $request->getQueryParams()['resource-server-identifier']
            ?? $request->getParsedBody()['resource-server-identifier']
            ?? '';
        $this->resourceServer = Registry::getResourceServerInstance($resourceServerIdentifier);
    } catch (\Exception $exception) {
        $this->logger->error($exception->getMessage());
    }
    return $this->resourceServer;
}

In our system, this triggers a Sentry Issue which contains the username and password in plain text:

 --data "__RequestToken=%5BFiltered%5D&commandLI=&loginRefresh=&login_status=login&p_field=&redirect_url=&userident=PASSWORD_IN_PLAIN_TEXT&username=USERNAME" \
 "https://example.com/typo3/login?loginProvider=1433416747"

Screenshot 2024-11-20 at 14 38 41

In previous versions of the extension Registry::getResourceServerInstance did not get called, when the login provider used was not the OAuth2LoginProvider. I think a check like this should be reintroduced.

@tehplague
Copy link
Member

tehplague commented Nov 20, 2024

The fact that your Sentry instance is logging plain credentials is actually unrelated to this. This happens because your current POST request contains the form field values submitted by TYPO3's own login form. You should employ Sentry's facilities to anonymize known form parameters potentially containing sensitive details to prevent this from happening.

Regarding why Sentry had created an issue in the first place: The exception logging was meant to be able to catch errors caused during the creation of the resource server instance. Maybe we should shortcut that and just immediately return null whenever $resourceServerIdentifier is empty, because that means that the current request was not handled by a known OAuth2 resource server. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants