From 526c8e4c89bdee2c3a4a4abb0c01f8d487b8c3c7 Mon Sep 17 00:00:00 2001 From: Jan Tvrdik Date: Sat, 7 May 2016 20:09:08 +0200 Subject: [PATCH 1/3] WIP --- src/SecuredRouter.php | 142 +++++++++++++++++++++++++++++++++ src/SecuredRouterFlagBased.php | 99 +++++++++++++++++++++++ 2 files changed, 241 insertions(+) create mode 100644 src/SecuredRouter.php create mode 100644 src/SecuredRouterFlagBased.php diff --git a/src/SecuredRouter.php b/src/SecuredRouter.php new file mode 100644 index 0000000..cf4f4d9 --- /dev/null +++ b/src/SecuredRouter.php @@ -0,0 +1,142 @@ +inner = $inner; + $this->presenterFactory = $presenterFactory; + $this->session = $session; + } + + + /** + * @inheritdoc + */ + public function match(Nette\Http\IRequest $httpRequest) + { + $appRequest = $this->inner->match($httpRequest); + + if ($appRequest !== NULL && $this->isSignatureRequired($appRequest) && !$this->isSignatureValid($appRequest)) { + return NULL; + } + + return $appRequest; + } + + + /** + * @inheritdoc + */ + public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) + { + if ($this->isSignatureRequired($appRequest)) { + $signature = $this->getSignature($appRequest); + $appRequest->setParameters(['_sec' => $signature] + $appRequest->getParameters()); + } + + return $this->inner->constructUrl($appRequest, $refUrl); + } + + + /** + * @param Request $appRequest + * @return bool + */ + protected function isSignatureRequired(Request $appRequest) + { + $presenterName = $appRequest->getPresenterName(); + $presenterClass = $this->presenterFactory->getPresenterClass($presenterName); + + if (!is_a($presenterClass, Presenter::class)) { + return FALSE; + } + + $params = $appRequest->getParameters(); + + if (isset($params['action'])) { + $methodName = $presenterClass::formatActionMethod($params['action']); + $methodRef = new ReflectionMethod($presenterClass, $methodName); + if ($this->isSecured($methodRef)) { + return TRUE; + } + } + + if (isset($params['do'])) { + $methodName = $presenterClass::formatSignalMethod($params['do']); + $methodRef = new ReflectionMethod($presenterClass, $methodName); + if ($this->isSecured($methodRef)) { + return TRUE; + } + } + + return FALSE; + } + + + /** + * @param ReflectionMethod $ref + * @return bool + */ + public function isSecured(ReflectionMethod $ref) + { + return (bool) preg_match('#^[ \t*]*@secured(\s|$)#m', $ref->getDocComment()); + } + + + /** + * @param Request $appRequest + * @return bool + */ + private function isSignatureValid(Request $appRequest) + { + $signature = $appRequest->getParameter('_sec'); + return ($signature !== NULL && hash_equals($this->getSignature($appRequest), $signature)); + } + + + /** + * @param Request $appRequest + * @return string + */ + private function getSignature(Request $appRequest) + { + $sessionSection = $this->session->getSection('Nextras.Application.UI.SecuredLinksPresenterTrait'); + if (!isset($sessionSection->token)) { + $sessionSection->token = function_exists('random_bytes') + ? random_bytes(16) + : Nette\Utils\Random::generate(16, "\x00-\xFF"); + } + + $data = [$this->session->getId(), $appRequest->getPresenterName(), $appRequest->getParameters()]; + $hash = hash_hmac('sha1', serialize($data), $sessionSection->token); + return substr($hash, 0, 6); + } +} diff --git a/src/SecuredRouterFlagBased.php b/src/SecuredRouterFlagBased.php new file mode 100644 index 0000000..002daac --- /dev/null +++ b/src/SecuredRouterFlagBased.php @@ -0,0 +1,99 @@ +inner = $inner; + $this->presenterFactory = $presenterFactory; + $this->session = $session; + } + + + /** + * @inheritdoc + */ + public function match(Nette\Http\IRequest $httpRequest) + { + $appRequest = $this->inner->match($httpRequest); + + if ($appRequest !== NULL && $this->isSignatureValid($appRequest)) { + $appRequest->setFlag(self::SIGNED); + } + + return $appRequest; + } + + + /** + * @inheritdoc + */ + public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) + { + if ($appRequest->hasFlag(self::SIGNED)) { + $signature = $this->getSignature($appRequest); + $appRequest->setParameters(['_sec' => $signature] + $appRequest->getParameters()); + } + + return $this->inner->constructUrl($appRequest, $refUrl); + } + + + /** + * @param Request $appRequest + * @return bool + */ + private function isSignatureValid(Request $appRequest) + { + $signature = $appRequest->getParameter('_sec'); + return ($signature !== NULL && hash_equals($this->getSignature($appRequest), $signature)); + } + + + /** + * @param Request $appRequest + * @return string + */ + private function getSignature(Request $appRequest) + { + $sessionSection = $this->session->getSection('Nextras.Application.UI.SecuredLinksPresenterTrait'); + if (!isset($sessionSection->token)) { + $sessionSection->token = function_exists('random_bytes') + ? random_bytes(16) + : Nette\Utils\Random::generate(16, "\x00-\xFF"); + } + + $data = [$this->session->getId(), $appRequest->getPresenterName(), $appRequest->getParameters()]; + $hash = hash_hmac('sha1', json_encode($data), $sessionSection->token); + return substr($hash, 0, 6); + } +} From 1c5f608a60e57425c4ed816ef0fbb1fe6c1113ec Mon Sep 17 00:00:00 2001 From: Jan Tvrdik Date: Sun, 8 May 2016 23:54:33 +0200 Subject: [PATCH 2/3] WIP: solution based on CompilerExtension --- composer.json | 7 +- src/SecuredLinksControlTrait.php | 75 --------- src/SecuredLinksExtension.php | 172 +++++++++++++++++++++ src/SecuredLinksPresenterTrait.php | 127 --------------- src/SecuredRouter.php | 121 ++++++++------- src/SecuredRouterFactory.php | 22 +++ src/SecuredRouterFlagBased.php | 99 ------------ tests/cases/SecuredLinksExtensionTest.phpt | 103 ++++++++++++ tests/fixtures/TestPresenter.php | 58 +++++++ 9 files changed, 427 insertions(+), 357 deletions(-) delete mode 100644 src/SecuredLinksControlTrait.php create mode 100644 src/SecuredLinksExtension.php delete mode 100644 src/SecuredLinksPresenterTrait.php create mode 100644 src/SecuredRouterFactory.php delete mode 100644 src/SecuredRouterFlagBased.php create mode 100644 tests/cases/SecuredLinksExtensionTest.phpt create mode 100644 tests/fixtures/TestPresenter.php diff --git a/composer.json b/composer.json index 99d2fc4..333dcab 100644 --- a/composer.json +++ b/composer.json @@ -12,12 +12,15 @@ } ], "require": { - "php": ">=5.4", + "php": ">=5.6", "nette/application": "~2.2", "nette/utils": "~2.2" }, "require-dev": { + "nette/di": "~2.2", + "nette/robot-loader": "~2.2", "nette/tester": "~1.3", + "tracy/tracy": "~2.2", "mockery/mockery": "~0.9" }, "extra": { @@ -26,7 +29,7 @@ } }, "autoload": { - "psr-4": { "Nextras\\Application\\UI\\": "src/" } + "psr-4": { "Nextras\\SecuredLinks\\": "src/" } }, "replace": { "nextras/application": "self.version" diff --git a/src/SecuredLinksControlTrait.php b/src/SecuredLinksControlTrait.php deleted file mode 100644 index a2ad601..0000000 --- a/src/SecuredLinksControlTrait.php +++ /dev/null @@ -1,75 +0,0 @@ -getPresenter()->createSecuredLink($this, $link, $destination); - } - - - /** - * For @secured annotated signal handler methods checks if URL parameters has not been changed - * - * @param string $signal - * @throws Nette\Application\UI\BadSignalException if there is no handler method or the security token does not match - * @throws \LogicException if there is no redirect in a secured signal - */ - public function signalReceived($signal) - { - $method = $this->formatSignalMethod($signal); - $secured = FALSE; - - if (method_exists($this, $method)) { - $reflection = new Nette\Reflection\Method($this, $method); - $secured = $reflection->hasAnnotation('secured'); - if ($secured) { - $params = array($this->getUniqueId()); - if ($this->params) { - foreach ($reflection->getParameters() as $param) { - if ($param->isOptional()) { - continue; - } - if (isset($this->params[$param->name])) { - $params[$param->name] = $this->params[$param->name]; - } - } - } - - if (!isset($this->params['_sec']) || $this->params['_sec'] !== $this->getPresenter()->getCsrfToken(get_class($this), $method, $params)) { - throw new Nette\Application\UI\BadSignalException("Invalid security token for signal '$signal' in class {$this->reflection->name}."); - } - } - } - - parent::signalReceived($signal); - - if ($secured && !$this->getPresenter()->isAjax()) { - throw new \LogicException("Secured signal '$signal' did not redirect. Possible csrf-token reveal by http referer header."); - } - } - -} diff --git a/src/SecuredLinksExtension.php b/src/SecuredLinksExtension.php new file mode 100644 index 0000000..20313f1 --- /dev/null +++ b/src/SecuredLinksExtension.php @@ -0,0 +1,172 @@ + 'secured', // can be NULL to disable + 'destinations' => [], + ]; + + + /** + * @inheritdoc + */ + public function beforeCompile() + { + $builder = $this->getContainerBuilder(); + $builder->addDefinition($this->prefix('routerFactory')) + ->setImplement(SecuredRouterFactory::class) + ->setParameters(['Nette\Application\IRouter innerRouter']) + ->setArguments([ + $builder->literal('$innerRouter'), + '@Nette\Application\IPresenterFactory', + '@Nette\Http\Session', + $this->findSecuredRequests() + ]); + + $innerRouter = $builder->getByType(IRouter::class); + $builder->getDefinition($innerRouter) + ->setAutowired(FALSE); + + $builder->addDefinition($this->prefix('router')) + ->setClass(IRouter::class) + ->setFactory("@{$this->name}.routerFactory::create", ["@$innerRouter"]) + ->setAutowired(TRUE); + } + + + /** + * @return array + */ + private function findSecuredRequests() + { + $securedRequests = []; + + foreach ($this->findSecuredDestinations() as $presenterClass => $destinations) { + foreach ($destinations as $destination => $ignoredParams) { + if (Strings::endsWith($destination, '!')) { + $key = 'do'; + $value = Strings::substring($destination, 0, -1); + + } else { + $key = 'action'; + $value = $destination; + } + + $securedRequests[$presenterClass][$key][$value] = $ignoredParams; + } + } + + return $securedRequests; + } + + + /** + * @return Generator + */ + private function findSecuredDestinations() + { + $config = $this->validateConfig($this->defaults); + + if ($config['annotation']) { + $presenters = $this->getContainerBuilder()->findByType(Presenter::class); + foreach ($presenters as $presenterDef) { + $presenterClass = $presenterDef->getClass(); + $presenterRef = new \ReflectionClass($presenterClass); + yield $presenterClass => $this->findSecuredMethods($presenterRef); + } + } + + foreach ($config['destinations'] as $presenterClass => $destinations) { + yield $presenterClass => $destinations; + } + } + + + /** + * @param ReflectionClass $classRef + * @return Generator + */ + private function findSecuredMethods(ReflectionClass $classRef) + { + foreach ($this->findTargetMethods($classRef) as $destination => $methodRef) { + if ($this->isSecured($methodRef, $ignoredParams)) { + yield $destination => $ignoredParams; + } + } + } + + + /** + * @param ReflectionClass $classRef + * @return Generator|ReflectionMethod[] + */ + private function findTargetMethods(ReflectionClass $classRef) + { + foreach ($classRef->getMethods() as $methodRef) { + $methodName = $methodRef->getName(); + + if (Strings::startsWith($methodName, 'action') && $classRef->isSubclassOf(Presenter::class)) { + $destination = Strings::firstLower(Strings::after($methodName, 'action')); + yield $destination => $methodRef; + + } elseif (Strings::startsWith($methodName, 'handle')) { + $destination = Strings::firstLower(Strings::after($methodName, 'handle')) . '!'; + yield $destination => $methodRef; + + } elseif (Strings::startsWith($methodName, 'createComponent')) { + $returnType = PhpReflection::getReturnType($methodRef); + if ($returnType !== NULL) { + $returnTypeRef = new ReflectionClass($returnType); + $componentName = Strings::firstLower(Strings::after($methodName, 'createComponent')); + foreach ($this->findTargetMethods($returnTypeRef) as $innerDestination => $innerRef) { + yield "$componentName-$innerDestination" => $innerRef; + } + } + } + } + } + + + /** + * @param ReflectionMethod $ref + * @param array|bool $params + * @return bool + */ + private function isSecured(ReflectionMethod $ref, & $params) + { + $annotation = preg_quote(isset($this->config['annotation']) + ? $this->config['annotation'] + : $this->defaults['annotation'], + '#' + ); + + if (preg_match("#^[ \\t/*]*@$annotation(?:[ \\t]+(\\[.*?\\])?|$)#m", $ref->getDocComment(), $matches)) { + $params = !empty($matches[1]) ? Neon::decode($matches[1]) : TRUE; + return TRUE; + + } else { + return FALSE; + } + } +} diff --git a/src/SecuredLinksPresenterTrait.php b/src/SecuredLinksPresenterTrait.php deleted file mode 100644 index 7e64450..0000000 --- a/src/SecuredLinksPresenterTrait.php +++ /dev/null @@ -1,127 +0,0 @@ -getLastCreatedRequest(); - - do { - if ($lastRequest === NULL) { - break; - } - - $params = $lastRequest->getParameters(); - if (!isset($params[Nette\Application\UI\Presenter::SIGNAL_KEY])) { - break; - } - - if (($pos = strpos($destination, '#')) !== FALSE) { - $destination = substr($destination, 0, $pos); - } - - $a = strpos($destination, '//'); - if ($a !== FALSE) { - $destination = substr($destination, $a + 2); - } - - $signal = strtr(rtrim($destination, '!'), ':', '-'); - $a = strrpos($signal, '-'); - if ($a !== FALSE) { - if ($component instanceof Nette\Application\UI\Presenter && substr($destination, -1) !== '!') { - break; - } - - $component = $component->getComponent(substr($signal, 0, $a)); - $signal = (string) substr($signal, $a + 1); - } - - if ($signal == NULL) { // intentionally == - throw new Nette\Application\UI\InvalidLinkException('Signal must be non-empty string.'); - } - - // only PresenterComponent - if (!$component instanceof PresenterComponent) { - break; - } - - $reflection = $component->getReflection(); - $method = $component->formatSignalMethod($signal); - $signalReflection = $reflection->getMethod($method); - - if (!$signalReflection->hasAnnotation('secured')) { - break; - } - - $origParams = $lastRequest->getParameters(); - $protectedParams = array($component->getUniqueId()); - foreach ($signalReflection->getParameters() as $param) { - if ($param->isOptional()) { - continue; - } - if (isset($origParams[$component->getParameterId($param->name)])) { - $protectedParams[$param->name] = $origParams[$component->getParameterId($param->name)]; - } - } - - $protectedParam = $this->getCsrfToken(get_class($component), $method, $protectedParams); - - if (($pos = strpos($link, '#')) === FALSE) { - $fragment = ''; - } else { - $fragment = substr($link, $pos); - $link = substr($link, 0, $pos); - } - - $link .= (strpos($link, '?') !== FALSE ? '&' : '?') . $component->getParameterId('_sec') . '=' . $protectedParam . $fragment; - } while (FALSE); - - return $link; - } - - - /** - * Returns unique token for method and params - * @param string $control - * @param string $method - * @param array $params - * @return string - */ - public function getCsrfToken($control, $method, $params) - { - $session = $this->getSession('Nextras.Application.UI.SecuredLinksPresenterTrait'); - if (!isset($session->token)) { - $session->token = Nette\Utils\Random::generate(); - } - - $params = Nette\Utils\Arrays::flatten($params); - $params = implode('|', array_keys($params)) . '|' . implode('|', array_values($params)); - return substr(md5($control . $method . $params . $session->token . $this->getSession()->getId()), 0, 8); - } - -} diff --git a/src/SecuredRouter.php b/src/SecuredRouter.php index cf4f4d9..5d520d0 100644 --- a/src/SecuredRouter.php +++ b/src/SecuredRouter.php @@ -1,19 +1,28 @@ inner = $inner; $this->presenterFactory = $presenterFactory; $this->session = $session; + $this->secured = $secured; } @@ -43,8 +57,7 @@ public function __construct(IRouter $inner, IPresenterFactory $presenterFactory, public function match(Nette\Http\IRequest $httpRequest) { $appRequest = $this->inner->match($httpRequest); - - if ($appRequest !== NULL && $this->isSignatureRequired($appRequest) && !$this->isSignatureValid($appRequest)) { + if ($appRequest !== NULL && !$this->isSignatureOk($appRequest)) { return NULL; } @@ -57,9 +70,10 @@ public function match(Nette\Http\IRequest $httpRequest) */ public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) { - if ($this->isSignatureRequired($appRequest)) { - $signature = $this->getSignature($appRequest); - $appRequest->setParameters(['_sec' => $signature] + $appRequest->getParameters()); + if ($this->isSignatureRequired($appRequest, $ignoredParams)) { + $params = $appRequest->getParameters(); + $params[self::SECURITY_KEY] = $this->getSignature($appRequest, $ignoredParams); + $appRequest->setParameters($params); } return $this->inner->constructUrl($appRequest, $refUrl); @@ -67,32 +81,40 @@ public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) /** - * @param Request $appRequest + * @param Request $appRequest + * @return bool + */ + private function isSignatureOk(Request $appRequest) + { + if ($this->isSignatureRequired($appRequest, $ignoredParams)) { + $actualSignature = $appRequest->getParameter(self::SECURITY_KEY); + $expectedSignature = $this->getSignature($appRequest, $ignoredParams); + return ($actualSignature !== NULL && hash_equals($expectedSignature, $actualSignature)); + + } else { + return TRUE; + } + } + + + /** + * @param Request $appRequest + * @param array|bool $ignoredParams * @return bool */ - protected function isSignatureRequired(Request $appRequest) + protected function isSignatureRequired(Request $appRequest, & $ignoredParams) { $presenterName = $appRequest->getPresenterName(); $presenterClass = $this->presenterFactory->getPresenterClass($presenterName); - if (!is_a($presenterClass, Presenter::class)) { + if (!isset($this->secured[$presenterClass])) { return FALSE; } $params = $appRequest->getParameters(); - - if (isset($params['action'])) { - $methodName = $presenterClass::formatActionMethod($params['action']); - $methodRef = new ReflectionMethod($presenterClass, $methodName); - if ($this->isSecured($methodRef)) { - return TRUE; - } - } - - if (isset($params['do'])) { - $methodName = $presenterClass::formatSignalMethod($params['do']); - $methodRef = new ReflectionMethod($presenterClass, $methodName); - if ($this->isSecured($methodRef)) { + foreach ($this->secured[$presenterClass] as $key => $foobar) { + if (isset($params[$key], $this->secured[$presenterClass][$key][$params[$key]])) { + $ignoredParams = $this->secured[$presenterClass][$key][$params[$key]]; return TRUE; } } @@ -102,41 +124,32 @@ protected function isSignatureRequired(Request $appRequest) /** - * @param ReflectionMethod $ref - * @return bool - */ - public function isSecured(ReflectionMethod $ref) - { - return (bool) preg_match('#^[ \t*]*@secured(\s|$)#m', $ref->getDocComment()); - } - - - /** - * @param Request $appRequest - * @return bool - */ - private function isSignatureValid(Request $appRequest) - { - $signature = $appRequest->getParameter('_sec'); - return ($signature !== NULL && hash_equals($this->getSignature($appRequest), $signature)); - } - - - /** - * @param Request $appRequest + * @param Request $appRequest + * @param array|bool $ignoredParams * @return string */ - private function getSignature(Request $appRequest) + private function getSignature(Request $appRequest, $ignoredParams) { - $sessionSection = $this->session->getSection('Nextras.Application.UI.SecuredLinksPresenterTrait'); - if (!isset($sessionSection->token)) { + $sessionSection = $this->session->getSection('Nextras.SecuredLinks'); + if (!isset($sessionSection->token) || strlen($sessionSection->token) !== self::SECURITY_TOKEN_LENGTH) { $sessionSection->token = function_exists('random_bytes') - ? random_bytes(16) - : Nette\Utils\Random::generate(16, "\x00-\xFF"); + ? random_bytes(self::SECURITY_TOKEN_LENGTH) + : Nette\Utils\Random::generate(self::SECURITY_TOKEN_LENGTH, "\x00-\xFF"); + } + + if ($ignoredParams === TRUE) { + $params = $appRequest->getParameters(); + } elseif ($ignoredParams === FALSE) { + $params = []; + } else { + $params = $appRequest->getParameters(); + foreach ($ignoredParams as $key) { + unset($params[$key]); + } } - $data = [$this->session->getId(), $appRequest->getPresenterName(), $appRequest->getParameters()]; - $hash = hash_hmac('sha1', serialize($data), $sessionSection->token); + $data = [$this->session->getId(), $appRequest->getPresenterName(), $params]; + $hash = hash_hmac('sha1', json_encode($data), $sessionSection->token); return substr($hash, 0, 6); } } diff --git a/src/SecuredRouterFactory.php b/src/SecuredRouterFactory.php new file mode 100644 index 0000000..8bfb4b1 --- /dev/null +++ b/src/SecuredRouterFactory.php @@ -0,0 +1,22 @@ +inner = $inner; - $this->presenterFactory = $presenterFactory; - $this->session = $session; - } - - - /** - * @inheritdoc - */ - public function match(Nette\Http\IRequest $httpRequest) - { - $appRequest = $this->inner->match($httpRequest); - - if ($appRequest !== NULL && $this->isSignatureValid($appRequest)) { - $appRequest->setFlag(self::SIGNED); - } - - return $appRequest; - } - - - /** - * @inheritdoc - */ - public function constructUrl(Request $appRequest, Nette\Http\Url $refUrl) - { - if ($appRequest->hasFlag(self::SIGNED)) { - $signature = $this->getSignature($appRequest); - $appRequest->setParameters(['_sec' => $signature] + $appRequest->getParameters()); - } - - return $this->inner->constructUrl($appRequest, $refUrl); - } - - - /** - * @param Request $appRequest - * @return bool - */ - private function isSignatureValid(Request $appRequest) - { - $signature = $appRequest->getParameter('_sec'); - return ($signature !== NULL && hash_equals($this->getSignature($appRequest), $signature)); - } - - - /** - * @param Request $appRequest - * @return string - */ - private function getSignature(Request $appRequest) - { - $sessionSection = $this->session->getSection('Nextras.Application.UI.SecuredLinksPresenterTrait'); - if (!isset($sessionSection->token)) { - $sessionSection->token = function_exists('random_bytes') - ? random_bytes(16) - : Nette\Utils\Random::generate(16, "\x00-\xFF"); - } - - $data = [$this->session->getId(), $appRequest->getPresenterName(), $appRequest->getParameters()]; - $hash = hash_hmac('sha1', json_encode($data), $sessionSection->token); - return substr($hash, 0, 6); - } -} diff --git a/tests/cases/SecuredLinksExtensionTest.phpt b/tests/cases/SecuredLinksExtensionTest.phpt new file mode 100644 index 0000000..ffd6da3 --- /dev/null +++ b/tests/cases/SecuredLinksExtensionTest.phpt @@ -0,0 +1,103 @@ +createContainer(); + $router = $dic->getByType(IRouter::class); + Assert::type(SecuredRouter::class, $router); + + $linkGenerator = $dic->getByType(LinkGenerator::class); + + Assert::same( + 'http://example.com/test?action=delete&_sec=e646b0', + $linkGenerator->link('Test:delete') + ); + + Assert::same( + 'http://example.com/test?do=pay&action=default&_sec=eed6d6', + $linkGenerator->link('Test:default', ['do' => 'pay']) + ); + + Assert::same( + 'http://example.com/test?do=pay&amount=1&action=default&_sec=7eda1c', + $linkGenerator->link('Test:default', ['do' => 'pay', 'amount' => 1]) + ); + + Assert::same( + 'http://example.com/test?do=pay&amount=2&action=default&_sec=f9cc2b', + $linkGenerator->link('Test:default', ['do' => 'pay', 'amount' => 2]) + ); + + Assert::same( + 'http://example.com/test?do=pay2&amount=1&action=default&_sec=51a97a', + $linkGenerator->link('Test:default', ['do' => 'pay2', 'amount' => 1]) + ); + + Assert::same( + 'http://example.com/test?do=pay2&amount=2&action=default&_sec=51a97a', // intentionally the same hash + $linkGenerator->link('Test:default', ['do' => 'pay2', 'amount' => 2]) + ); + } + + + /** + * @return \Nette\DI\Container + */ + private function createContainer() + { + $compiler = new Nette\DI\Compiler; + $compiler->addExtension('nette.http', new HttpExtension); + $compiler->addExtension('nette.http.sessions', new SessionExtension); + $compiler->addExtension('nette.application', new ApplicationExtension(TRUE, [__DIR__ . '/../fixtures'])); + $compiler->addExtension('nextras.securedLinks', new SecuredLinksExtension); + + eval($compiler->compile( + [ + 'services' => [new Nette\DI\Statement(Route::class, ['//example.com/'])] + ], + 'SecuredLinksExtensionContainer' + )); + + $sessionSection = Mockery::mock('alias:Nette\Http\SessionSection'); + $sessionSection->token = 'abcdabcdabcdabcd'; + + $session = Mockery::mock('Nette\Http\Session'); + $session->shouldReceive('getSection')->with('Nextras.SecuredLinks')->andReturn($sessionSection); + $session->shouldReceive('getId')->times(8)->andReturn('session_id_1'); + + /** @var Nette\DI\Container $dic */ + $dic = new \SecuredLinksExtensionContainer(); + $dic->removeService('nette.http.sessions.session'); + $dic->addService('nette.http.sessions.session', $session); + return $dic; + } +} + +(new SecuredLinksExtensionTest)->run(); diff --git a/tests/fixtures/TestPresenter.php b/tests/fixtures/TestPresenter.php new file mode 100644 index 0000000..f8399be --- /dev/null +++ b/tests/fixtures/TestPresenter.php @@ -0,0 +1,58 @@ +terminate(); + } + + + /** @secured */ + public function handlePay($amount) + { + } + + + /** @secured [amount] */ + public function handlePay2($amount) + { + } + + + /** @secured */ + public function handleList(array $sections) + { + } + + + /** + * @secured + */ + public function actionDelete() + { + + } + + + /** + * @return TestControl + */ + protected function createComponentMyControl() + { + return new TestControl(); + } +} From 70bf9d4ac628dda86d6bc269cd69507a95fc6fa3 Mon Sep 17 00:00:00 2001 From: Jan Tvrdik Date: Tue, 10 May 2016 00:21:31 +0200 Subject: [PATCH 3/3] better --- composer.json | 8 +- .../NetteDI}/SecuredLinksExtension.php | 53 ++++- src/Bridges/PhpParser/ReturnTypeResolver.php | 188 ++++++++++++++++++ src/RedirectChecker.php | 33 +++ src/SecuredRouter.php | 7 +- src/deprecated/SecuredLinksControlTrait.php | 42 ++++ src/deprecated/SecuredLinksPresenterTrait.php | 119 +++++++++++ tests/cases/SecuredLinksExtensionTest.phpt | 3 +- tests/fixtures/TestPresenter.php | 44 +++- 9 files changed, 483 insertions(+), 14 deletions(-) rename src/{ => Bridges/NetteDI}/SecuredLinksExtension.php (74%) create mode 100644 src/Bridges/PhpParser/ReturnTypeResolver.php create mode 100644 src/RedirectChecker.php create mode 100644 src/deprecated/SecuredLinksControlTrait.php create mode 100644 src/deprecated/SecuredLinksPresenterTrait.php diff --git a/composer.json b/composer.json index 333dcab..f25864a 100644 --- a/composer.json +++ b/composer.json @@ -18,10 +18,14 @@ }, "require-dev": { "nette/di": "~2.2", - "nette/robot-loader": "~2.2", "nette/tester": "~1.3", "tracy/tracy": "~2.2", - "mockery/mockery": "~0.9" + "mockery/mockery": "~0.9", + "nikic/php-parser": "~2.0" + }, + "suggest": { + "nette/di": "to use SecuredLinksExtension", + "nikic/php-parser": "to detect return types without @return annotation" }, "extra": { "branch-alias": { diff --git a/src/SecuredLinksExtension.php b/src/Bridges/NetteDI/SecuredLinksExtension.php similarity index 74% rename from src/SecuredLinksExtension.php rename to src/Bridges/NetteDI/SecuredLinksExtension.php index 20313f1..3d5edbb 100644 --- a/src/SecuredLinksExtension.php +++ b/src/Bridges/NetteDI/SecuredLinksExtension.php @@ -2,11 +2,12 @@ /** * This file is part of the Nextras Secured Links library. + * * @license MIT * @link https://github.com/nextras/secured-links */ -namespace Nextras\SecuredLinks; +namespace Nextras\SecuredLinks\Bridges\NetteDI; use Generator; use Nette; @@ -15,16 +16,22 @@ use Nette\DI\PhpReflection; use Nette\Neon\Neon; use Nette\Utils\Strings; +use Nextras\SecuredLinks\Bridges\PhpParser\ReturnTypeResolver; +use Nextras\SecuredLinks\RedirectChecker; +use Nextras\SecuredLinks\SecuredRouterFactory; +use PhpParser\Node; use ReflectionClass; use ReflectionMethod; class SecuredLinksExtension extends Nette\DI\CompilerExtension { + /** @var array */ public $defaults = [ 'annotation' => 'secured', // can be NULL to disable 'destinations' => [], + 'strictMode' => TRUE, ]; @@ -52,6 +59,12 @@ public function beforeCompile() ->setClass(IRouter::class) ->setFactory("@{$this->name}.routerFactory::create", ["@$innerRouter"]) ->setAutowired(TRUE); + + $builder->addDefinition($this->prefix('redirectChecker')) + ->setClass(RedirectChecker::class); + + $builder->getDefinition($builder->getByType(Nette\Application\Application::class)) + ->addSetup('?->onResponse[] = [?, ?]', ['@self', '@Nextras\SecuredLinks\RedirectChecker', 'checkResponse']); } @@ -88,18 +101,20 @@ private function findSecuredDestinations() { $config = $this->validateConfig($this->defaults); + foreach ($config['destinations'] as $presenterClass => $destinations) { + yield $presenterClass => $destinations; + } + if ($config['annotation']) { $presenters = $this->getContainerBuilder()->findByType(Presenter::class); foreach ($presenters as $presenterDef) { $presenterClass = $presenterDef->getClass(); - $presenterRef = new \ReflectionClass($presenterClass); - yield $presenterClass => $this->findSecuredMethods($presenterRef); + if (!isset($config['destinations'][$presenterClass])) { + $presenterRef = new \ReflectionClass($presenterClass); + yield $presenterClass => $this->findSecuredMethods($presenterRef); + } } } - - foreach ($config['destinations'] as $presenterClass => $destinations) { - yield $presenterClass => $destinations; - } } @@ -135,13 +150,20 @@ private function findTargetMethods(ReflectionClass $classRef) yield $destination => $methodRef; } elseif (Strings::startsWith($methodName, 'createComponent')) { - $returnType = PhpReflection::getReturnType($methodRef); + $returnType = $this->getMethodReturnType($methodRef); if ($returnType !== NULL) { $returnTypeRef = new ReflectionClass($returnType); $componentName = Strings::firstLower(Strings::after($methodName, 'createComponent')); foreach ($this->findTargetMethods($returnTypeRef) as $innerDestination => $innerRef) { yield "$componentName-$innerDestination" => $innerRef; } + + } elseif ($this->config['strictMode']) { + $className = $methodRef->getDeclaringClass()->getName(); + throw new \LogicException( + "Unable to deduce return type for method $className::$methodName(); " . + "add @return annotation, install nikic/php-parser or disable strictMode in config" + ); } } } @@ -169,4 +191,19 @@ private function isSecured(ReflectionMethod $ref, & $params) return FALSE; } } + + + /** + * @param ReflectionMethod $methodRef + * @return NULL|string + */ + private function getMethodReturnType(ReflectionMethod $methodRef) + { + $returnType = PhpReflection::getReturnType($methodRef); + if ($returnType !== NULL || !interface_exists(\PhpParser\Node::class)) { + return $returnType; + } else { + return ReturnTypeResolver::getReturnType($methodRef); + } + } } diff --git a/src/Bridges/PhpParser/ReturnTypeResolver.php b/src/Bridges/PhpParser/ReturnTypeResolver.php new file mode 100644 index 0000000..ae95125 --- /dev/null +++ b/src/Bridges/PhpParser/ReturnTypeResolver.php @@ -0,0 +1,188 @@ +className = $className; + $this->methodName = $methodName; + $this->varTypes['this'][] = $className; + } + + + /** + * @param ReflectionMethod $methodRef + * @return NULL|string + */ + public static function getReturnType(ReflectionMethod $methodRef) + { + $fileContent = file_get_contents($methodRef->getDeclaringClass()->getFileName()); + + $traverser = new NodeTraverser(); + $traverser->addVisitor(new NameResolver); + $traverser->addVisitor($resolver = new self($methodRef->getDeclaringClass()->getName(), $methodRef->getName())); + $traverser->traverse((new ParserFactory)->create(ParserFactory::PREFER_PHP7)->parse($fileContent)); + + return count($resolver->returnTypes) === 1 ? $resolver->returnTypes[0] : NULL; + } + + + /** + * @inheritdoc + */ + public function enterNode(Node $node) + { + if ($node instanceof Node\Stmt\Class_ && $node->name === $this->className) { + $this->inClass = TRUE; + + } elseif ($this->inClass && $node instanceof Node\Stmt\ClassMethod && $node->name === $this->methodName) { + $this->inMethod = TRUE; + + } elseif ($this->inMethod) { + if ($node instanceof Node\Stmt\Return_ && $node->expr !== NULL) { + foreach ($this->getExpressionTypes($node->expr) as $type) { + $this->addReturnType($type); + } + + } elseif ($node instanceof Node\Expr\Assign) { + foreach ($this->getExpressionTypes($node->expr) as $type) { + $this->addVarType($node, $type); + } + } + } + } + + + /** + * @inheritdoc + */ + public function leaveNode(Node $node) + { + if ($this->inMethod && $node instanceof Node\Stmt\ClassMethod) { + $this->inMethod = FALSE; + + } elseif ($this->inClass && $node instanceof Node\Stmt\Class_) { + $this->inClass = FALSE; + } + } + + + /** + * @param Node\Expr $expr + * @return string[] + */ + private function getExpressionTypes(Node\Expr $expr) + { + $result = []; + + if ($expr instanceof Node\Expr\New_) { + if ($expr->class instanceof Node\Name) { + $result[] = (string) $expr->class; + } + + } elseif ($expr instanceof Node\Expr\Variable) { + if (is_string($expr->name) && isset($this->varTypes[$expr->name])) { + $result = $this->varTypes[$expr->name]; + } + + } elseif ($expr instanceof Node\Expr\PropertyFetch) { + if (is_string($expr->name)) { + foreach ($this->getExpressionTypes($expr->var) as $objType) { + $propertyRef = new \ReflectionProperty($objType, $expr->name); + $type = PhpReflection::parseAnnotation($propertyRef, 'var'); + $type = $type ? PhpReflection::expandClassName($type, PhpReflection::getDeclaringClass($propertyRef)) : NULL; + $result[] = $type; + } + } + + } elseif ($expr instanceof Node\Expr\MethodCall) { + if (is_string($expr->name)) { + foreach ($this->getExpressionTypes($expr->var) as $objType) { + $methodRef = new \ReflectionMethod($objType, $expr->name); + $result[] = PhpReflection::getReturnType($methodRef); + } + } + + } elseif ($expr instanceof Node\Expr\Assign) { + foreach ($this->getExpressionTypes($expr->expr) as $type) { + $this->addVarType($expr, $type); + $result[] = $type; + } + + } elseif ($expr instanceof Node\Expr\Clone_) { + $result = $this->getExpressionTypes($expr->expr); + } + + return $result; + } + + + /** + * @param string $exprType + * @return void + */ + private function addReturnType($exprType) + { + if ($exprType !== NULL && class_exists($exprType) && !in_array($exprType, $this->returnTypes)) { + $this->returnTypes[] = $exprType; + } + } + + + /** + * @param Node\Expr\Assign $node + * @param string $exprType + * @return void + */ + private function addVarType($node, $exprType) + { + if ($node->var instanceof Node\Expr\Variable && is_string($node->var->name) + && (empty($this->varTypes[$node->var->name]) || !in_array($exprType, $this->varTypes[$node->var->name])) + && $exprType !== NULL && class_exists($exprType) + ) { + $this->varTypes[$node->var->name][] = $exprType; + } + } +} diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php new file mode 100644 index 0000000..b470181 --- /dev/null +++ b/src/RedirectChecker.php @@ -0,0 +1,33 @@ +getRequests(); + $request = $requests[count($requests) - 1]; + + if ($request->hasFlag(SecuredRouter::SIGNED) && !$response instanceof RedirectResponse) { + throw new \LogicException('Secured request did not redirect. Possible CSRF-token reveal by HTTP referer header.'); + } + } +} diff --git a/src/SecuredRouter.php b/src/SecuredRouter.php index 5d520d0..c99e765 100644 --- a/src/SecuredRouter.php +++ b/src/SecuredRouter.php @@ -17,6 +17,9 @@ class SecuredRouter implements IRouter { + /** signed flag, marks requests which has been signed */ + const SIGNED = 'signed'; + /** length of secret token stored in session */ const SECURITY_TOKEN_LENGTH = 16; @@ -57,8 +60,8 @@ public function __construct(IRouter $inner, IPresenterFactory $presenterFactory, public function match(Nette\Http\IRequest $httpRequest) { $appRequest = $this->inner->match($httpRequest); - if ($appRequest !== NULL && !$this->isSignatureOk($appRequest)) { - return NULL; + if ($appRequest !== NULL && $this->isSignatureOk($appRequest)) { + $appRequest->setFlag(self::SIGNED); } return $appRequest; diff --git a/src/deprecated/SecuredLinksControlTrait.php b/src/deprecated/SecuredLinksControlTrait.php new file mode 100644 index 0000000..b20c415 --- /dev/null +++ b/src/deprecated/SecuredLinksControlTrait.php @@ -0,0 +1,42 @@ +formatSignalMethod($signal); + if (method_exists($this, $methodName)) { + $methodRef = new Nette\Reflection\Method($this, $methodName); + if ($methodRef->hasAnnotation('secured') && !$this->request->hasFlag(SecuredRouter::SIGNED)) { + $who = $this instanceof Presenter ? 'Presenter' : 'Control'; + throw new \LogicException( + "$who received request to secured signal which was not properly signed." . + "This indicate a bug in your installation of Nextras Secured Links." . + "Please consult documentation on how to properly migrate to Nextras Secured Links 2.0" + ); + } + } + + parent::signalReceived($signal); + } +} diff --git a/src/deprecated/SecuredLinksPresenterTrait.php b/src/deprecated/SecuredLinksPresenterTrait.php new file mode 100644 index 0000000..080ea21 --- /dev/null +++ b/src/deprecated/SecuredLinksPresenterTrait.php @@ -0,0 +1,119 @@ +getLastCreatedRequest(); + + do { + if ($lastRequest === NULL) { + break; + } + + $params = $lastRequest->getParameters(); + if (!isset($params[Nette\Application\UI\Presenter::SIGNAL_KEY])) { + break; + } + + if (($pos = strpos($destination, '#')) !== FALSE) { + $destination = substr($destination, 0, $pos); + } + + $a = strpos($destination, '//'); + if ($a !== FALSE) { + $destination = substr($destination, $a + 2); + } + + $signal = strtr(rtrim($destination, '!'), ':', '-'); + $a = strrpos($signal, '-'); + if ($a !== FALSE) { + if ($component instanceof Nette\Application\UI\Presenter && substr($destination, -1) !== '!') { + break; + } + + $component = $component->getComponent(substr($signal, 0, $a)); + $signal = (string) substr($signal, $a + 1); + } + + if ($signal == NULL) { // intentionally == + throw new Nette\Application\UI\InvalidLinkException('Signal must be non-empty string.'); + } + + // only PresenterComponent + if (!$component instanceof PresenterComponent) { + break; + } + + $reflection = $component->getReflection(); + $method = $component->formatSignalMethod($signal); + $signalReflection = $reflection->getMethod($method); + + if (!$signalReflection->hasAnnotation('secured')) { + break; + } + + $origParams = $lastRequest->getParameters(); + $protectedParams = array($component->getUniqueId()); + foreach ($signalReflection->getParameters() as $param) { + if ($param->isOptional()) { + continue; + } + if (isset($origParams[$component->getParameterId($param->name)])) { + $protectedParams[$param->name] = $origParams[$component->getParameterId($param->name)]; + } + } + + $protectedParam = $this->getCsrfToken(get_class($component), $method, $protectedParams); + + if (($pos = strpos($link, '#')) === FALSE) { + $fragment = ''; + } else { + $fragment = substr($link, $pos); + $link = substr($link, 0, $pos); + } + + $link .= (strpos($link, '?') !== FALSE ? '&' : '?') . $component->getParameterId('_sec') . '=' . $protectedParam . $fragment; + } while (FALSE); + + return $link; + } + + + /** + * @deprecated + */ + public function getCsrfToken($control, $method, $params) + { + $session = $this->getSession('Nextras.Application.UI.SecuredLinksPresenterTrait'); + if (!isset($session->token)) { + $session->token = Nette\Utils\Random::generate(); + } + + $params = Nette\Utils\Arrays::flatten($params); + $params = implode('|', array_keys($params)) . '|' . implode('|', array_values($params)); + return substr(md5($control . $method . $params . $session->token . $this->getSession()->getId()), 0, 8); + } +} diff --git a/tests/cases/SecuredLinksExtensionTest.phpt b/tests/cases/SecuredLinksExtensionTest.phpt index ffd6da3..8a59082 100644 --- a/tests/cases/SecuredLinksExtensionTest.phpt +++ b/tests/cases/SecuredLinksExtensionTest.phpt @@ -16,7 +16,7 @@ use Nette\Application\Routers\Route; use Nette\Bridges\ApplicationDI\ApplicationExtension; use Nette\Bridges\HttpDI\HttpExtension; use Nette\Bridges\HttpDI\SessionExtension; -use Nextras\SecuredLinks\SecuredLinksExtension; +use Nextras\SecuredLinks\Bridges\NetteDI\SecuredLinksExtension; use Nextras\SecuredLinks\SecuredRouter; use Tester; use Tester\Assert; @@ -96,6 +96,7 @@ class SecuredLinksExtensionTest extends Tester\TestCase $dic = new \SecuredLinksExtensionContainer(); $dic->removeService('nette.http.sessions.session'); $dic->addService('nette.http.sessions.session', $session); + return $dic; } } diff --git a/tests/fixtures/TestPresenter.php b/tests/fixtures/TestPresenter.php index f8399be..d03bb6c 100644 --- a/tests/fixtures/TestPresenter.php +++ b/tests/fixtures/TestPresenter.php @@ -12,9 +12,25 @@ public function handlePay($amount = 0) } } +interface TestControlFactory +{ + + /** + * @return TestControl + */ + public function create(); +} + class TestPresenter extends Presenter { + /** @var TestControl */ + public $testControl; + + /** @var TestControlFactory */ + public $testControlFactory; + + public function renderDefault() { $this->terminate(); @@ -51,8 +67,34 @@ public function actionDelete() /** * @return TestControl */ - protected function createComponentMyControl() + protected function createComponentMyControlA() + { + + } + + + protected function createComponentMyControlB() { return new TestControl(); } + + + protected function createComponentMyControlC() + { + $tmp = new TestControl(); + $control = $tmp; + return $control; + } + + + protected function createComponentMyControlD() + { + return clone $this->testControl; + } + + + protected function createComponentMyControlE() + { + return $this->testControlFactory->create(); + } }