Skip to content

Commit

Permalink
feat(security): Add PHP \Attribute for remaining security annotations
Browse files Browse the repository at this point in the history
Signed-off-by: Joas Schilling <coding@schilljs.com>
  • Loading branch information
nickvergessen committed Apr 25, 2023
1 parent 2abefff commit ecb8b55
Show file tree
Hide file tree
Showing 24 changed files with 1,278 additions and 278 deletions.
8 changes: 8 additions & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,15 @@
'OCP\\AppFramework\\Http' => $baseDir . '/lib/public/AppFramework/Http.php',
'OCP\\AppFramework\\Http\\Attribute\\ARateLimit' => $baseDir . '/lib/public/AppFramework/Http/Attribute/ARateLimit.php',
'OCP\\AppFramework\\Http\\Attribute\\AnonRateLimit' => $baseDir . '/lib/public/AppFramework/Http/Attribute/AnonRateLimit.php',
'OCP\\AppFramework\\Http\\Attribute\\AuthorizedAdminSetting' => $baseDir . '/lib/public/AppFramework/Http/Attribute/AuthorizedAdminSetting.php',
'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => $baseDir . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
'OCP\\AppFramework\\Http\\Attribute\\CORS' => $baseDir . '/lib/public/AppFramework/Http/Attribute/CORS.php',
'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\PublicPage' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PublicPage.php',
'OCP\\AppFramework\\Http\\Attribute\\StrictCookiesRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/StrictCookiesRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\SubAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/SubAdminRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\UseSession' => $baseDir . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
'OCP\\AppFramework\\Http\\Attribute\\UserRateLimit' => $baseDir . '/lib/public/AppFramework/Http/Attribute/UserRateLimit.php',
'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => $baseDir . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',
Expand Down
8 changes: 8 additions & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,15 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http.php',
'OCP\\AppFramework\\Http\\Attribute\\ARateLimit' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/ARateLimit.php',
'OCP\\AppFramework\\Http\\Attribute\\AnonRateLimit' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/AnonRateLimit.php',
'OCP\\AppFramework\\Http\\Attribute\\AuthorizedAdminSetting' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/AuthorizedAdminSetting.php',
'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
'OCP\\AppFramework\\Http\\Attribute\\CORS' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/CORS.php',
'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\PublicPage' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PublicPage.php',
'OCP\\AppFramework\\Http\\Attribute\\StrictCookiesRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/StrictCookiesRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\SubAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/SubAdminRequired.php',
'OCP\\AppFramework\\Http\\Attribute\\UseSession' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
'OCP\\AppFramework\\Http\\Attribute\\UserRateLimit' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/UserRateLimit.php',
'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',
Expand Down
61 changes: 45 additions & 16 deletions lib/private/AppFramework/Middleware/Security/CORSMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,13 @@
use OC\User\Session;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\CORS;
use OCP\AppFramework\Http\Attribute\PublicPage;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\Response;
use OCP\AppFramework\Middleware;
use OCP\IRequest;
use ReflectionMethod;

/**
* This middleware sets the correct CORS headers on a response if the
Expand Down Expand Up @@ -81,9 +84,12 @@ public function __construct(IRequest $request,
* @since 6.0.0
*/
public function beforeController($controller, $methodName) {
$reflectionMethod = new ReflectionMethod($controller, $methodName);

// ensure that @CORS annotated API routes are not used in conjunction
// with session authentication since this enables CSRF attack vectors
if ($this->reflector->hasAnnotation('CORS') && (!$this->reflector->hasAnnotation('PublicPage') || $this->session->isLoggedIn())) {
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) &&
(!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) {
$user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null;
$pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null;

Expand All @@ -103,7 +109,28 @@ public function beforeController($controller, $methodName) {
}

/**
* This is being run after a successful controllermethod call and allows
* @template T
*
* @param ReflectionMethod $reflectionMethod
* @param string $annotationName
* @param class-string<T> $attributeClass
* @return boolean
*/
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
if ($this->reflector->hasAnnotation($annotationName)) {
return true;
}


if (!empty($reflectionMethod->getAttributes($attributeClass))) {
return true;
}

return false;
}

/**
* This is being run after a successful controller method call and allows
* the manipulation of a Response object. The middleware is run in reverse order
*
* @param Controller $controller the controller that is being called
Expand All @@ -114,23 +141,25 @@ public function beforeController($controller, $methodName) {
* @throws SecurityException
*/
public function afterController($controller, $methodName, Response $response) {
// only react if its a CORS request and if the request sends origin and
// only react if it's a CORS request and if the request sends origin and

if (isset($this->request->server['HTTP_ORIGIN']) &&
$this->reflector->hasAnnotation('CORS')) {
// allow credentials headers must not be true or CSRF is possible
// otherwise
foreach ($response->getHeaders() as $header => $value) {
if (strtolower($header) === 'access-control-allow-credentials' &&
strtolower(trim($value)) === 'true') {
$msg = 'Access-Control-Allow-Credentials must not be '.
'set to true in order to prevent CSRF';
throw new SecurityException($msg);
if (isset($this->request->server['HTTP_ORIGIN'])) {
$reflectionMethod = new ReflectionMethod($controller, $methodName);
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)) {
// allow credentials headers must not be true or CSRF is possible
// otherwise
foreach ($response->getHeaders() as $header => $value) {
if (strtolower($header) === 'access-control-allow-credentials' &&
strtolower(trim($value)) === 'true') {
$msg = 'Access-Control-Allow-Credentials must not be '.
'set to true in order to prevent CSRF';
throw new SecurityException($msg);
}
}
}

$origin = $this->request->server['HTTP_ORIGIN'];
$response->addHeader('Access-Control-Allow-Origin', $origin);
$origin = $this->request->server['HTTP_ORIGIN'];
$response->addHeader('Access-Control-Allow-Origin', $origin);
}
}
return $response;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@
use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException;
use OC\AppFramework\Utility\ControllerMethodReflector;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
use OCP\AppFramework\Middleware;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\ISession;
use OCP\IUserSession;
use OCP\User\Backend\IPasswordConfirmationBackend;
use ReflectionMethod;

class PasswordConfirmationMiddleware extends Middleware {
/** @var ControllerMethodReflector */
Expand Down Expand Up @@ -68,7 +70,9 @@ public function __construct(ControllerMethodReflector $reflector,
* @throws NotConfirmedException
*/
public function beforeController($controller, $methodName) {
if ($this->reflector->hasAnnotation('PasswordConfirmationRequired')) {
$reflectionMethod = new ReflectionMethod($controller, $methodName);

if ($this->hasAnnotationOrAttribute($reflectionMethod, 'PasswordConfirmationRequired', PasswordConfirmationRequired::class)) {
$user = $this->userSession->getUser();
$backendClassName = '';
if ($user !== null) {
Expand All @@ -89,4 +93,24 @@ public function beforeController($controller, $methodName) {
}
}
}

/**
* @template T
*
* @param ReflectionMethod $reflectionMethod
* @param string $annotationName
* @param class-string<T> $attributeClass
* @return boolean
*/
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
return true;
}

if ($this->reflector->hasAnnotation($annotationName)) {
return true;
}

return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public function afterException($controller, $methodName, \Exception $exception)

return new RedirectResponse($this->urlGenerator->linkToRouteAbsolute(
'core.login.showLoginForm',
['clear' => true] // this param the the code in login.js may be removed when the "Clear-Site-Data" is working in the browsers
['clear' => true] // this param the code in login.js may be removed when the "Clear-Site-Data" is working in the browsers
));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@
use OCP\App\AppPathNotFoundException;
use OCP\App\IAppManager;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting;
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
use OCP\AppFramework\Http\Attribute\PublicPage;
use OCP\AppFramework\Http\Attribute\StrictCookiesRequired;
use OCP\AppFramework\Http\Attribute\SubAdminRequired;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\Response;
Expand All @@ -61,6 +67,7 @@
use OCP\IUserSession;
use OCP\Util;
use Psr\Log\LoggerInterface;
use ReflectionMethod;

/**
* Used to do all the authentication and checking stuff for a controller method
Expand Down Expand Up @@ -145,22 +152,24 @@ public function beforeController($controller, $methodName) {
$this->navigationManager->setActiveEntry('spreed');
}

$reflectionMethod = new ReflectionMethod($controller, $methodName);

// security checks
$isPublicPage = $this->reflector->hasAnnotation('PublicPage');
$isPublicPage = $this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class);
if (!$isPublicPage) {
if (!$this->isLoggedIn) {
throw new NotLoggedInException();
}
$authorized = false;
if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) {
$authorized = $this->isAdminUser;

if (!$authorized && $this->reflector->hasAnnotation('SubAdminRequired')) {
if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) {
$authorized = $this->isSubAdmin;
}

if (!$authorized) {
$settingClasses = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings'));
$settingClasses = $this->getAuthorizedAdminSettingClasses($reflectionMethod);
$authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser());
foreach ($settingClasses as $settingClass) {
$authorized = in_array($settingClass, $authorizedClasses, true);
Expand All @@ -174,29 +183,30 @@ public function beforeController($controller, $methodName) {
throw new NotAdminException($this->l10n->t('Logged in user must be an admin, a sub admin or gotten special right to access this setting'));
}
}
if ($this->reflector->hasAnnotation('SubAdminRequired')
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
&& !$this->isSubAdmin
&& !$this->isAdminUser
&& !$authorized) {
throw new NotAdminException($this->l10n->t('Logged in user must be an admin or sub admin'));
}
if (!$this->reflector->hasAnnotation('SubAdminRequired')
&& !$this->reflector->hasAnnotation('NoAdminRequired')
if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
&& !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class)
&& !$this->isAdminUser
&& !$authorized) {
throw new NotAdminException($this->l10n->t('Logged in user must be an admin'));
}
}

// Check for strict cookie requirement
if ($this->reflector->hasAnnotation('StrictCookieRequired') || !$this->reflector->hasAnnotation('NoCSRFRequired')) {
if ($this->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class) ||
!$this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) {
if (!$this->request->passesStrictCookieCheck()) {
throw new StrictCookieMissingException();
}
}
// CSRF check - also registers the CSRF token since the session may be closed later
Util::callRegister();
if (!$this->reflector->hasAnnotation('NoCSRFRequired')) {
if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) {
/*
* Only allow the CSRF check to fail on OCS Requests. This kind of
* hacks around that we have no full token auth in place yet and we
Expand Down Expand Up @@ -232,6 +242,48 @@ public function beforeController($controller, $methodName) {
}
}

/**
* @template T
*
* @param ReflectionMethod $reflectionMethod
* @param string $annotationName
* @param class-string<T> $attributeClass
* @return boolean
*/
protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool {
if (!empty($reflectionMethod->getAttributes($attributeClass))) {
return true;
}

if ($this->reflector->hasAnnotation($annotationName)) {
return true;
}

return false;
}

/**
* @param ReflectionMethod $reflectionMethod
* @return string[]
*/
protected function getAuthorizedAdminSettingClasses(ReflectionMethod $reflectionMethod): array {
$classes = [];
if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
$classes = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings'));
}

$attributes = $reflectionMethod->getAttributes(AuthorizedAdminSetting::class);
if (!empty($attributes)) {
foreach ($attributes as $attribute) {
/** @var AuthorizedAdminSetting $setting */
$setting = $attribute->newInstance();
$classes[] = $setting->getSettings();
}
}

return $classes;
}

/**
* If an SecurityException is being caught, ajax requests return a JSON error
* response and non ajax requests redirect to the index
Expand Down
4 changes: 4 additions & 0 deletions lib/public/AppFramework/ApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
*/
namespace OCP\AppFramework;

use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
use OCP\AppFramework\Http\Attribute\PublicPage;
use OCP\AppFramework\Http\Response;
use OCP\IRequest;

Expand Down Expand Up @@ -70,6 +72,8 @@ public function __construct($appName,
* @PublicPage
* @since 7.0.0
*/
#[NoCSRFRequired]
#[PublicPage]
public function preflightedCors() {
if (isset($this->request->server['HTTP_ORIGIN'])) {
$origin = $this->request->server['HTTP_ORIGIN'];
Expand Down
11 changes: 10 additions & 1 deletion lib/public/AppFramework/AuthPublicShareController.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
*/
namespace OCP\AppFramework;

use OCP\AppFramework\Http\Attribute\BruteForceProtection;
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
use OCP\AppFramework\Http\Attribute\PublicPage;
use OCP\AppFramework\Http\Attribute\UseSession;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\IRequest;
Expand Down Expand Up @@ -70,6 +74,8 @@ public function __construct(string $appName,
*
* @since 14.0.0
*/
#[NoCSRFRequired]
#[PublicPage]
public function showAuthenticate(): TemplateResponse {
return new TemplateResponse('core', 'publicshareauth', [], 'guest');
}
Expand Down Expand Up @@ -129,7 +135,7 @@ protected function authFailed() {
}

/**
* Function called after successfull authentication
* Function called after successful authentication
*
* You can use this to do some logging for example
*
Expand All @@ -147,6 +153,9 @@ protected function authSucceeded() {
*
* @since 14.0.0
*/
#[BruteForceProtection(action: 'publicLinkAuth')]
#[PublicPage]
#[UseSession]
final public function authenticate(string $password = '', string $passwordRequest = 'no', string $identityToken = '') {
// Already authenticated
if ($this->isAuthenticated()) {
Expand Down
Loading

0 comments on commit ecb8b55

Please sign in to comment.