From eda9c80ab6d2d838520054c4f4de04b549055643 Mon Sep 17 00:00:00 2001 From: Alexander Piskun <13381981+bigcat88@users.noreply.github.com> Date: Thu, 22 Aug 2024 22:25:40 +0300 Subject: [PATCH] ability to enable bruteforce protection for ExApp routes (#368) ```xml ^api\/w\/nextcloud\/jobs\/.* GET,POST,PUT,DELETE PUBLIC [] [401, 500] ``` Looks like this. ExApps should not implement its own protection, we should provide a way to enable basic protection from Nextcloud/AppAPI side. --------- Signed-off-by: Alexander Piskun Signed-off-by: Andrey Borysenko Co-authored-by: Andrey Borysenko --- appinfo/info.xml | 2 +- docs/tech_details/api/routes.rst | 2 + lib/Controller/ExAppProxyController.php | 83 +++++++++++++------ lib/Db/ExAppMapper.php | 11 +++ .../Version3100Date20240822080316.php | 39 +++++++++ 5 files changed, 111 insertions(+), 26 deletions(-) create mode 100644 lib/Migration/Version3100Date20240822080316.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 0282b3c1..d7b32089 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -43,7 +43,7 @@ to join us in shaping a more versatile, stable, and secure app landscape. *Your insights, suggestions, and contributions are invaluable to us.* ]]> - 3.1.0 + 3.1.1 agpl Andrey Borysenko Alexander Piskun diff --git a/docs/tech_details/api/routes.rst b/docs/tech_details/api/routes.rst index ca098f42..afca4f2a 100644 --- a/docs/tech_details/api/routes.rst +++ b/docs/tech_details/api/routes.rst @@ -28,6 +28,7 @@ Example GET,POST,PUT,DELETE USER [] + [401, 500] @@ -37,6 +38,7 @@ where the fields are: - ``verb``: the HTTP verb that the route will accept, can be a comma separated list of verbs - ``access_level``: the name of the access level required to access the route, PUBLIC - public access without auth, USER - Nextcloud user auth required, ADMIN - admin user required - ``headers_to_exclude``: a json encoded string of an array of strings, the headers that the ExApp wants to be excluded from the request to it +- ``bruteforce_protection``: a json encoded string of an array of numbers, the HTTP status codes that must trigger the bruteforce protection Unregister diff --git a/lib/Controller/ExAppProxyController.php b/lib/Controller/ExAppProxyController.php index 1b48cd93..7fbed667 100644 --- a/lib/Controller/ExAppProxyController.php +++ b/lib/Controller/ExAppProxyController.php @@ -24,6 +24,7 @@ use OCP\Http\Client\IResponse; use OCP\IGroupManager; use OCP\IRequest; +use OCP\Security\Bruteforce\IThrottler; use Psr\Log\LoggerInterface; class ExAppProxyController extends Controller { @@ -37,6 +38,7 @@ public function __construct( private readonly ?string $userId, private readonly IGroupManager $groupManager, private readonly LoggerInterface $logger, + private readonly IThrottler $throttler, ) { parent::__construct(Application::APP_ID, $request); } @@ -84,7 +86,10 @@ private function createProxyResponse(string $path, IResponse $response, $cache = #[NoAdminRequired] #[NoCSRFRequired] public function ExAppGet(string $appId, string $other): Response { - $exApp = $this->checkAccess($appId, $other); + $route = []; + $bruteforceProtection = []; + $delay = 0; + $exApp = $this->prepareProxy($appId, $other, $route, $bruteforceProtection, $delay); if ($exApp === null) { return new NotFoundResponse(); } @@ -92,7 +97,7 @@ public function ExAppGet(string $appId, string $other): Response { $response = $this->service->requestToExApp2( $exApp, '/' . $other, $this->userId, 'GET', queryParams: $_GET, options: [ RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $this->service->getExAppDomain($exApp)), - RequestOptions::HEADERS => $this->buildHeadersWithExclude($exApp, $other, getallheaders()), + RequestOptions::HEADERS => $this->buildHeadersWithExclude($route, getallheaders()), RequestOptions::TIMEOUT => 0, ], request: $this->request, @@ -100,6 +105,8 @@ public function ExAppGet(string $appId, string $other): Response { if (is_array($response)) { return (new Response())->setStatus(500); } + + $this->processBruteforce($bruteforceProtection, $delay, $response->getStatusCode()); return $this->createProxyResponse($other, $response); } @@ -107,14 +114,17 @@ public function ExAppGet(string $appId, string $other): Response { #[NoAdminRequired] #[NoCSRFRequired] public function ExAppPost(string $appId, string $other): Response { - $exApp = $this->checkAccess($appId, $other); + $route = []; + $bruteforceProtection = []; + $delay = 0; + $exApp = $this->prepareProxy($appId, $other, $route, $bruteforceProtection, $delay); if ($exApp === null) { return new NotFoundResponse(); } $options = [ RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $this->service->getExAppDomain($exApp)), - RequestOptions::HEADERS => $this->buildHeadersWithExclude($exApp, $other, getallheaders()), + RequestOptions::HEADERS => $this->buildHeadersWithExclude($route, getallheaders()), RequestOptions::TIMEOUT => 0, ]; if (str_starts_with($this->request->getHeader('Content-Type'), 'multipart/form-data') || count($_FILES) > 0) { @@ -136,6 +146,8 @@ public function ExAppPost(string $appId, string $other): Response { if (is_array($response)) { return (new Response())->setStatus(500); } + + $this->processBruteforce($bruteforceProtection, $delay, $response->getStatusCode()); return $this->createProxyResponse($other, $response); } @@ -143,7 +155,10 @@ public function ExAppPost(string $appId, string $other): Response { #[NoAdminRequired] #[NoCSRFRequired] public function ExAppPut(string $appId, string $other): Response { - $exApp = $this->checkAccess($appId, $other); + $route = []; + $bruteforceProtection = []; + $delay = 0; + $exApp = $this->prepareProxy($appId, $other, $route, $bruteforceProtection, $delay); if ($exApp === null) { return new NotFoundResponse(); } @@ -152,7 +167,7 @@ public function ExAppPut(string $appId, string $other): Response { $options = [ RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $this->service->getExAppDomain($exApp)), RequestOptions::BODY => $stream, - RequestOptions::HEADERS => $this->buildHeadersWithExclude($exApp, $other, getallheaders()), + RequestOptions::HEADERS => $this->buildHeadersWithExclude($route, getallheaders()), RequestOptions::TIMEOUT => 0, ]; $response = $this->service->requestToExApp2( @@ -163,6 +178,8 @@ public function ExAppPut(string $appId, string $other): Response { if (is_array($response)) { return (new Response())->setStatus(500); } + + $this->processBruteforce($bruteforceProtection, $delay, $response->getStatusCode()); return $this->createProxyResponse($other, $response); } @@ -170,7 +187,10 @@ public function ExAppPut(string $appId, string $other): Response { #[NoAdminRequired] #[NoCSRFRequired] public function ExAppDelete(string $appId, string $other): Response { - $exApp = $this->checkAccess($appId, $other); + $route = []; + $bruteforceProtection = []; + $delay = 0; + $exApp = $this->prepareProxy($appId, $other, $route, $bruteforceProtection, $delay); if ($exApp === null) { return new NotFoundResponse(); } @@ -179,7 +199,7 @@ public function ExAppDelete(string $appId, string $other): Response { $options = [ RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $this->service->getExAppDomain($exApp)), RequestOptions::BODY => $stream, - RequestOptions::HEADERS => $this->buildHeadersWithExclude($exApp, $other, getallheaders()), + RequestOptions::HEADERS => $this->buildHeadersWithExclude($route, getallheaders()), RequestOptions::TIMEOUT => 0, ]; $response = $this->service->requestToExApp2( @@ -190,10 +210,15 @@ public function ExAppDelete(string $appId, string $other): Response { if (is_array($response)) { return (new Response())->setStatus(500); } + + $this->processBruteforce($bruteforceProtection, $delay, $response->getStatusCode()); return $this->createProxyResponse($other, $response); } - private function checkAccess(string $appId, string $other): ?ExApp { + private function prepareProxy( + string $appId, string $other, array &$route, array &$bruteforceProtection, int &$delay + ): ?ExApp { + $delay = 0; $exApp = $this->exAppService->getExApp($appId); if ($exApp === null) { $this->logger->debug( @@ -205,15 +230,33 @@ private function checkAccess(string $appId, string $other): ?ExApp { sprintf('Returning status 404 for "%s": ExApp is not enabled.', $other) ); return null; - } elseif (!$this->passesExAppProxyRoutesChecks($exApp, $other)) { + } + $route = $this->passesExAppProxyRoutesChecks($exApp, $other); + if (empty($route)) { $this->logger->debug( sprintf('Returning status 404 for "%s": route does not pass the access check.', $other) ); return null; } + $bruteforceProtection = isset($route['bruteforce_protection']) + ? json_decode($route['bruteforce_protection'], true) + : []; + if (!empty($bruteforceProtection)) { + $delay = $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), Application::APP_ID); + } return $exApp; } + private function processBruteforce(array $bruteforceProtection, int $delay, int $status): void { + if (!empty($bruteforceProtection)) { + if ($delay > 0 && ($status >= 200 && $status < 300)) { + $this->throttler->resetDelay($this->request->getRemoteAddress(), Application::APP_ID, []); + } elseif (in_array($status, $bruteforceProtection)) { + $this->throttler->registerAttempt(Application::APP_ID, $this->request->getRemoteAddress()); + } + } + } + private function buildProxyCookiesJar(array $cookies, string $domain): CookieJar { $cookieJar = new CookieJar(); foreach ($cookies as $name => $value) { @@ -250,16 +293,16 @@ private function buildMultipartFormData(array $bodyParams, array $files): array return $multipart; } - private function passesExAppProxyRoutesChecks(ExApp $exApp, string $exAppRoute): bool { + private function passesExAppProxyRoutesChecks(ExApp $exApp, string $exAppRoute): array { foreach ($exApp->getRoutes() as $route) { if (preg_match('/' . $route['url'] . '/i', $exAppRoute) === 1 && str_contains(strtolower($route['verb']), strtolower($this->request->getMethod())) && $this->passesExAppProxyRouteAccessLevelCheck($route['access_level']) ) { - return true; + return $route; } } - return false; + return []; } private function passesExAppProxyRouteAccessLevelCheck(int $accessLevel): bool { @@ -271,18 +314,8 @@ private function passesExAppProxyRouteAccessLevelCheck(int $accessLevel): bool { }; } - private function buildHeadersWithExclude(ExApp $exApp, string $exAppRoute, array $headers): array { - $headersToExclude = []; - foreach ($exApp->getRoutes() as $route) { - $matchesUrlPattern = preg_match('/' . $route['url'] . '/i', $exAppRoute) === 1; - $matchesVerb = str_contains(strtolower($route['verb']), strtolower($this->request->getMethod())); - if ($matchesUrlPattern && $matchesVerb) { - $headersToExclude = array_map(function ($headerName) { - return strtolower($headerName); - }, json_decode($route['headers_to_exclude'], true)); - break; - } - } + private function buildHeadersWithExclude(array $route, array $headers): array { + $headersToExclude = json_decode($route['headers_to_exclude'], true); if (!in_array('x-origin-ip', $headersToExclude)) { $headersToExclude[] = 'x-origin-ip'; } diff --git a/lib/Db/ExAppMapper.php b/lib/Db/ExAppMapper.php index b90212d9..43c60335 100644 --- a/lib/Db/ExAppMapper.php +++ b/lib/Db/ExAppMapper.php @@ -37,6 +37,7 @@ public function findAll(?int $limit = null, ?int $offset = null): array { 'r.verb', 'r.access_level', 'r.headers_to_exclude', + 'r.bruteforce_protection', ) ->from($this->tableName, 'a') ->leftJoin('a', 'ex_apps_daemons', 'd', $qb->expr()->eq('a.daemon_config_name', 'd.name')) @@ -68,6 +69,7 @@ public function findByAppId(string $appId): Entity { 'r.verb', 'r.access_level', 'r.headers_to_exclude', + 'r.bruteforce_protection', ) ->from($this->tableName, 'a') ->leftJoin('a', 'ex_apps_daemons', 'd', $qb->expr()->eq('a.daemon_config_name', 'd.name')) @@ -125,6 +127,7 @@ private function buildExAppWithRoutes(array $result): array { 'verb' => $row['verb'], 'access_level' => $row['access_level'], 'headers_to_exclude' => $row['headers_to_exclude'], + 'bruteforce_protection' => $row['bruteforce_protection'], ]; $lastAppRoutes = $lastApp->getRoutes(); $lastAppRoutes[] = $route; @@ -195,6 +198,9 @@ public function registerExAppRoutes(ExApp $exApp, array $routes): int { $qb = $this->db->getQueryBuilder(); $count = 0; foreach ($routes as $route) { + if (isset($route['bruteforce_protection']) && is_string($route['bruteforce_protection'])) { + $route['bruteforce_protection'] = json_decode($route['bruteforce_protection'], false); + } $qb->insert('ex_apps_routes') ->values([ 'appid' => $qb->createNamedParameter($exApp->getAppid()), @@ -202,6 +208,11 @@ public function registerExAppRoutes(ExApp $exApp, array $routes): int { 'verb' => $qb->createNamedParameter($route['verb']), 'access_level' => $qb->createNamedParameter($route['access_level']), 'headers_to_exclude' => $qb->createNamedParameter(is_array($route['headers_to_exclude']) ? json_encode($route['headers_to_exclude']) : '[]'), + 'bruteforce_protection' => $qb->createNamedParameter( + isset($route['bruteforce_protection']) && is_array($route['bruteforce_protection']) + ? json_encode($route['bruteforce_protection']) + : '[]' + ), ]); $count += $qb->executeStatement(); } diff --git a/lib/Migration/Version3100Date20240822080316.php b/lib/Migration/Version3100Date20240822080316.php new file mode 100644 index 00000000..68252cb3 --- /dev/null +++ b/lib/Migration/Version3100Date20240822080316.php @@ -0,0 +1,39 @@ +getTable('ex_apps_routes'); + if (!$table->hasColumn('bruteforce_protection')) { + $table->addColumn('bruteforce_protection', Types::STRING, [ + 'notnull' => false, + 'length' => 512, + ]); + } + + return $schema; + } +}