Merge pull request #55 from oat-sa/bugfix/TR-2645

fix: convert exception message to html for response
oat-sa · Dec 16, 2021 · 4497c21 · 4497c21
2 parents 4ad373f + 41ae560
commit 4497c21
Show file tree

Hide file tree

Showing 13 changed files with 310 additions and 17 deletions.
diff --git a/Action/Platform/Message/OidcAuthenticationAction.php b/Action/Platform/Message/OidcAuthenticationAction.php
@@ -22,6 +22,7 @@
 
 namespace OAT\Bundle\Lti1p3Bundle\Action\Platform\Message;
 
+use Throwable;
 use OAT\Library\Lti1p3Core\Exception\LtiExceptionInterface;
 use OAT\Library\Lti1p3Core\Security\Oidc\OidcAuthenticator;
 use Psr\Log\LoggerInterface;
@@ -62,7 +63,15 @@ public function __invoke(Request $request): Response
         } catch (LtiExceptionInterface $exception) {
             $this->logger->error(sprintf('OidcAuthenticationAction: %s', $exception->getMessage()));
 
-            return new Response($exception->getMessage(), Response::HTTP_UNAUTHORIZED);
+            return new Response(
+                $this->convertThrowableMessageToHtml($exception),
+                Response::HTTP_UNAUTHORIZED
+            );
         }
     }
+
+    private function convertThrowableMessageToHtml(Throwable $throwable): string
+    {
+        return htmlspecialchars($throwable->getMessage(), ENT_QUOTES);
+    }
 }
diff --git a/Action/Tool/Message/OidcInitiationAction.php b/Action/Tool/Message/OidcInitiationAction.php
@@ -22,6 +22,7 @@
 
 namespace OAT\Bundle\Lti1p3Bundle\Action\Tool\Message;
 
+use Throwable;
 use OAT\Library\Lti1p3Core\Exception\LtiExceptionInterface;
 use OAT\Library\Lti1p3Core\Security\Oidc\OidcInitiator;
 use Psr\Log\LoggerInterface;
@@ -63,7 +64,15 @@ public function __invoke(Request $request): Response
         } catch (LtiExceptionInterface $exception) {
             $this->logger->error(sprintf('OidcInitiationAction: %s', $exception->getMessage()));
 
-            return new Response($exception->getMessage(), Response::HTTP_BAD_REQUEST);
+            return new Response(
+                $this->convertThrowableMessageToHtml($exception),
+                Response::HTTP_BAD_REQUEST
+            );
         }
     }
+
+    private function convertThrowableMessageToHtml(Throwable $throwable): string
+    {
+        return htmlspecialchars($throwable->getMessage(), ENT_QUOTES);
+    }
 }
diff --git a/Security/Authentication/Provider/Message/LtiPlatformMessageAuthenticationProvider.php b/Security/Authentication/Provider/Message/LtiPlatformMessageAuthenticationProvider.php
@@ -77,7 +77,7 @@ public function authenticate(TokenInterface $token): TokenInterface
         } catch (Throwable $exception) {
             throw new AuthenticationException(
                 sprintf('LTI platform message request authentication failed: %s', $exception->getMessage()),
-                $exception->getCode(),
+                (int) $exception->getCode(),
                 $exception
             );
         }

diff --git a/Security/Authentication/Provider/Message/LtiToolMessageAuthenticationProvider.php b/Security/Authentication/Provider/Message/LtiToolMessageAuthenticationProvider.php
@@ -77,7 +77,7 @@ public function authenticate(TokenInterface $token): TokenInterface
         } catch (Throwable $exception) {
             throw new AuthenticationException(
                 sprintf('LTI tool message request authentication failed: %s', $exception->getMessage()),
-                $exception->getCode(),
+                (int) $exception->getCode(),
                 $exception
             );
         }

diff --git a/Security/Authentication/Provider/Service/LtiServiceAuthenticationProvider.php b/Security/Authentication/Provider/Service/LtiServiceAuthenticationProvider.php
@@ -71,7 +71,7 @@ public function authenticate(TokenInterface $token): TokenInterface
         } catch (Throwable $exception) {
             throw new AuthenticationException(
                 sprintf('LTI service request authentication failed: %s', $exception->getMessage()),
-                $exception->getCode(),
+                (int) $exception->getCode(),
                 $exception
             );
         }

diff --git a/Security/Exception/LtiToolMessageExceptionHandler.php b/Security/Exception/LtiToolMessageExceptionHandler.php
@@ -45,7 +45,7 @@ public function __construct(?ParserInterface $parser = null)
      */
     public function handle(Throwable $exception, Request $request): Response
     {
-        $payload = new LtiMessagePayload($this->parser->parse($request->get('id_token')));
+        $payload = new LtiMessagePayload($this->parser->parse($this->getIdTokenFromRequest($request)));
 
         $launchPresentation = $payload->getLaunchPresentation();
 
@@ -67,4 +67,14 @@ public function handle(Throwable $exception, Request $request): Response
 
         throw $exception;
     }
+
+    private function getIdTokenFromRequest(Request $request): string
+    {
+        $idTokenFromQuery = $request->query->get('id_token');
+        if (null !== $idTokenFromQuery) {
+            return (string) $idTokenFromQuery;
+        }
+
+        return (string) $request->request->get('id_token');
+    }
 }
diff --git a/Security/Firewall/Message/LtiPlatformMessageAuthenticationListener.php b/Security/Firewall/Message/LtiPlatformMessageAuthenticationListener.php
@@ -59,7 +59,7 @@ public function __construct(
 
     public function supports(Request $request): ?bool
     {
-        return null !== $request->get('JWT');
+        return null !== $this->getJwtFromRequest($request);
     }
 
     public function authenticate(RequestEvent $event): void
@@ -71,6 +71,15 @@ public function authenticate(RequestEvent $event): void
         $token->setAttribute('firewall_config', $this->firewallMap->getFirewallConfig($request));
 
         $this->storage->setToken($this->manager->authenticate($token));
+    }
+
+    private function getJwtFromRequest(Request $request): ?string
+    {
+        $jwtFromQuery = $request->query->get('JWT');
+        if (null !== $jwtFromQuery) {
+            return $jwtFromQuery;
+        }
 
+        return $request->request->get('JWT');
     }
 }
diff --git a/Security/Firewall/Message/LtiToolMessageAuthenticationListener.php b/Security/Firewall/Message/LtiToolMessageAuthenticationListener.php
@@ -66,7 +66,7 @@ public function __construct(
 
     public function supports(Request $request): ?bool
     {
-        return null !== $request->get('id_token');
+        return null !== $this->getIdTokenFromRequest($request);
     }
 
     public function authenticate(RequestEvent $event): void
@@ -83,4 +83,14 @@ public function authenticate(RequestEvent $event): void
             $event->setResponse($this->handler->handle($exception, $request));
         }
     }
+
+    private function getIdTokenFromRequest(Request $request): ?string
+    {
+        $idTokenFromQuery = $request->query->get('id_token');
+        if (null !== $idTokenFromQuery) {
+            return $idTokenFromQuery;
+        }
+
+        return $request->request->get('id_token');
+    }
 }
diff --git a/Tests/Resources/Kernel/Lti1p3TestKernel.php b/Tests/Resources/Kernel/Lti1p3TestKernel.php
@@ -29,22 +29,30 @@
 use Symfony\Component\Config\Loader\LoaderInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\HttpKernel\Kernel;
+use Symfony\Component\Routing\Loader\Configurator\RoutingConfigurator;
 use Symfony\Component\Routing\RouteCollectionBuilder;
 
 class Lti1p3TestKernel extends Kernel
 {
     use MicroKernelTrait;
 
-    public function registerBundles()
+    public function registerBundles(): iterable
     {
-        return [
-            new FrameworkBundle(),
-            new SecurityBundle(),
-            new Lti1p3Bundle()
+        $bundles = [
+            FrameworkBundle::class,
+            SecurityBundle::class,
+            Lti1p3Bundle::class
         ];
+
+        foreach ($bundles as $class) {
+            yield new $class();
+        }
     }
 
-    protected function configureRoutes(RouteCollectionBuilder $routes)
+    /**
+     * @param RouteCollectionBuilder|RoutingConfigurator $routes
+     */
+    protected function configureRoutes($routes): void
     {
         // bundle jwks route
         $routes->import(__DIR__  . '/../../../Resources/config/routing/jwks.yaml');
@@ -60,7 +68,7 @@ protected function configureRoutes(RouteCollectionBuilder $routes)
         $routes->import(__DIR__  . '/config/routes.yaml');
     }
 
-    protected function configureContainer(ContainerBuilder $container, LoaderInterface $loader)
+    protected function configureContainer(ContainerBuilder $container, LoaderInterface $loader): void
     {
         // bundle config
         $loader->load(__DIR__ . '/../../../Resources/config/services.yaml');
@@ -71,12 +79,12 @@ protected function configureContainer(ContainerBuilder $container, LoaderInterfa
         $loader->load(__DIR__ . '/config/lti1p3.yaml');
     }
 
-    public function getCacheDir()
+    public function getCacheDir(): string
     {
         return sys_get_temp_dir() . DIRECTORY_SEPARATOR . $this->environment . '/cache/' . spl_object_hash($this);
     }
 
-    public function getLogDir()
+    public function getLogDir(): string
     {
         return sys_get_temp_dir() . DIRECTORY_SEPARATOR . $this->environment . '/logs/' . spl_object_hash($this);
     }

diff --git a/Tests/Unit/Action/Platform/Message/OidcAuthenticationActionTest.php b/Tests/Unit/Action/Platform/Message/OidcAuthenticationActionTest.php
@@ -0,0 +1,112 @@
+<?php
+
+/**
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; under version 2
+ * of the License (non-upgradable).
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright (c) 2021 (original work) Open Assessment Technologies SA;
+ */
+
+declare(strict_types=1);
+
+namespace OAT\Bundle\Lti1p3Bundle\Tests\Unit\Action\Platform\Message;
+
+use OAT\Bundle\Lti1p3Bundle\Action\Platform\Message\OidcAuthenticationAction;
+use OAT\Library\Lti1p3Core\Exception\LtiException;
+use OAT\Library\Lti1p3Core\Security\Oidc\OidcInitiator;
+use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Log\LoggerInterface;
+use Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface;
+use Symfony\Component\HttpFoundation\Request;
+
+final class OidcAuthenticationActionTest extends TestCase
+{
+    /**
+     * @var HttpMessageFactoryInterface
+     */
+    private $factory;
+
+    /**
+     * @var OidcInitiator
+     */
+    private $initiator;
+
+    /**
+     * @var LoggerInterface
+     */
+    private $logger;
+
+    /**
+     * @var Request
+     */
+    private $request;
+
+    /**
+     * @var OidcAuthenticationAction
+     */
+    private $oidcAuthenticationAction;
+
+    protected function setUp(): void
+    {
+        $this->factory = $this->createMock(HttpMessageFactoryInterface::class);
+        $this->initiator = $this->createMock(OidcInitiator::class);
+        $this->logger = $this->createMock(LoggerInterface::class);
+        $this->request = $this->createMock(Request::class);
+
+        $this->oidcAuthenticationAction = new OidcAuthenticationAction(
+            $this->factory,
+            $this->initiator,
+            $this->logger
+        );
+    }
+
+    /**
+     * @dataProvider providerXssAttack
+     */
+    public function testXssAttackWhenExceptionThrown(string $expected, string $message): void
+    {
+        $this->factory->method('createRequest')->willReturn(
+            $this->createMock(ServerRequestInterface::class)
+        );
+
+        $this->initiator->method('initiate')->willThrowException(new LtiException($message));
+
+        $response = $this->oidcAuthenticationAction->__invoke($this->request);
+
+        self::assertSame($expected, $response->getContent());
+    }
+
+    public function providerXssAttack(): array
+    {
+        return [
+            'On Error' => [
+                '&lt;img+src=https://localhost+onerror=alert(1)&gt;',
+                '<img+src=https://localhost+onerror=alert(1)>'
+            ],
+            'Mouse Over' => [
+                '&lt;b onmouseover=alert(&#039;Wufff!&#039;)&gt;click me!&lt;/b&gt;',
+                '<b onmouseover=alert(\'Wufff!\')>click me!</b>'
+            ],
+            'URI Schemes' => [
+                '&lt;IMG SRC=j&amp;#X41vascript:alert(&#039;test2&#039;)&gt;',
+                '<IMG SRC=j&#X41vascript:alert(\'test2\')>'
+            ],
+            'Base64' => [
+                '&lt;img onload=&quot;eval(atob(&#039;d2luZG93LmFsZXJ0KCcxJyk=&#039;))&quot;&gt;',
+                '<img onload="eval(atob(\'d2luZG93LmFsZXJ0KCcxJyk=\'))">'
+            ]
+        ];
+    }
+}