Merge pull request #165 from BaranekD/scripts_improvements

fix: Security improvements in script calls

Author: vyskocilpavel

CHANGELOG.md

@@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file.
 #### Changed
 - Improve WAYF searching by localized name and domain
 - Implemented filter EnsureVoMember
+- Security improvements in script calls
 
 #### Fixed
 - Detailed endpoint format when spaced in EndpointMapToArray 

config-templates/challenges_config.php

@@ -0,0 +1,28 @@
+<?php
+
+$config = array(
+    /**
+     * Path to public key
+     */
+    'pubKey' => 'Path_to_public_key',
+
+    /**
+     * Path to private key
+     */
+    'privKey' => 'Path_to_private_key',
+
+    /**
+     * Hash algorithm
+     */
+    'hashAlg' => 'sha256',
+
+    /**
+     * Signature algorith
+     */
+    'sigAlg' => 'RS512',
+
+    /**
+     * Challenge length
+     */
+    'challengeLength' => 64,
+);

lib/Auth/Process/UpdateUserExtSource.php

@@ -2,15 +2,10 @@
 
 namespace SimpleSAML\Module\perun\Auth\Process;
 
-use Jose\Component\KeyManagement\JWKFactory;
-use Jose\Component\Core\AlgorithmManager;
-use Jose\Component\Signature\JWSBuilder;
-use Jose\Component\Signature\Serializer\CompactSerializer;
-use Jose\Component\Signature\Algorithm\RS512;
 use SimpleSAML\Auth\ProcessingFilter;
 use SimpleSAML\Error\Exception;
-use SimpleSAML\Logger;
 use SimpleSAML\Module;
+use SimpleSAML\Module\perun\ChallengeManager;
 use SimpleSAML\Module\perun\UpdateUESThread;
 
 /**
@@ -25,7 +20,6 @@ class UpdateUserExtSource extends ProcessingFilter
 {
     private $attrMap;
     private $attrsToConversion;
-    private $pathToKey;
 
     const SCRIPT_NAME = 'updateUes';
 
@@ -41,73 +35,27 @@ public function __construct($config, $reserved)
             );
         }
 
-        if (!isset($config['pathToKey'])) {
-            throw new Exception(
-                'perun:UpdateUserExtSource: missing mandatory configuration option \'pathToKey\'.'
-            );
-        }
-
         if (isset($config['arrayToStringConversion'])) {
             $this->attrsToConversion = (array)$config['arrayToStringConversion'];
         } else {
             $this->attrsToConversion = [];
         }
 
         $this->attrMap = (array)$config['attrMap'];
-        $this->pathToKey = $config['pathToKey'];
     }
 
     public function process(&$request)
     {
         $id = uniqid("", true);
 
-        $dataChallenge = [
-            'id' => $id,
-            'scriptName' => self::SCRIPT_NAME
-        ];
-
-        $json = json_encode($dataChallenge);
-
-        $curlChallenge = curl_init();
-        curl_setopt($curlChallenge, CURLOPT_POSTFIELDS, $json);
-        curl_setopt($curlChallenge, CURLOPT_URL, Module::getModuleURL('perun/getChallenge.php'));
-        curl_setopt($curlChallenge, CURLOPT_RETURNTRANSFER, true);
-
-        $challenge = curl_exec($curlChallenge);
-
-        if (empty($challenge)) {
-            Logger::error('Retrieving the challenge was not successful.');
-            return;
-        }
-
-        $jwk = JWKFactory::createFromKeyFile($this->pathToKey);
-        $algorithmManager = new AlgorithmManager([new RS512()]);
-        $jwsBuilder = new JWSBuilder($algorithmManager);
-
+        $challengeManager = new ChallengeManager();
         $data = [
             'attributes' => $request['Attributes'],
             'attrMap' => $this->attrMap,
             'attrsToConversion' => $this->attrsToConversion,
             'perunUserId' => $request['perun']['user']->getId()
         ];
-
-        $payload = json_encode([
-            'iat' => time(),
-            'nbf' => time(),
-            'exp' => time() + 300,
-            'challenge' => $challenge,
-            'id' => $id,
-            'data' => $data
-        ]);
-
-        $jws = $jwsBuilder
-            ->create()
-            ->withPayload($payload)
-            ->addSignature($jwk, ['alg' => 'RS512'])
-            ->build();
-
-        $serializer = new CompactSerializer();
-        $token = $serializer->serialize($jws, 0);
+        $token = $challengeManager->generateToken($id, self::SCRIPT_NAME, $data);
 
         $cmd = 'curl -X POST -H "Content-Type: application/json" -d ' . escapeshellarg($token) . ' ' .
             escapeshellarg(Module::getModuleURL('perun/updateUes.php')) . ' > /dev/null &';

lib/ChallengeManager.php

@@ -2,34 +2,166 @@
 
 namespace SimpleSAML\Module\perun;
 
+use SimpleSAML\Configuration;
 use SimpleSAML\Logger;
 use SimpleSAML\Module\perun\databaseCommand\ChallengesDbCmd;
+use Jose\Component\KeyManagement\JWKFactory;
+use Jose\Component\Core\AlgorithmManager;
+use Jose\Component\Signature\JWSBuilder;
+use Jose\Component\Signature\Serializer\CompactSerializer;
+use Jose\Component\Checker\AlgorithmChecker;
+use Jose\Component\Checker\ClaimCheckerManager;
+use Jose\Component\Checker\HeaderCheckerManager;
+use Jose\Component\Signature\JWSTokenSupport;
+use Jose\Component\Signature\JWSVerifier;
+use Jose\Component\Signature\Serializer\JWSSerializerManager;
+use Jose\Component\Checker;
 
 class ChallengeManager
 {
     const LOG_PREFIX = 'Perun:ChallengeManager: ';
+
+    const CONFIG_FILE_NAME = 'challenges_config.php';
+    const HASH_ALG = 'hashAlg';
+    const SIG_ALG = 'sigAlg';
+    const CHALLENGE_LENGTH = 'challengeLength';
+    const PUB_KEY = 'pubKey';
+    const PRIV_KEY = 'privKey';
+
     private $challengeDbCmd;
 
+    private $hashAlg;
+    private $challengeLength;
+
+    private $privKey;
+    private $pubKey;
+
+
     public function __construct()
     {
         $this->challengeDbCmd = new ChallengesDbCmd();
+        $config = Configuration::getConfig(self::CONFIG_FILE_NAME);
+        $this->hashAlg = $config->getString(self::HASH_ALG, 'sha512');
+        $this->sighAlg = $config->getString(self::SIG_ALG, 'sha512');
+        $this->challengeLength = $config->getInteger(self::CHALLENGE_LENGTH, 32);
+        $this->pubKey = $config->getString(self::PUB_KEY);
+        $this->privKey = $config->getString(self::PRIV_KEY);
     }
 
-    public function insertChallenge($challenge, $id, $scriptName): bool
+    public function generateChallenge($id, $scriptName): string
     {
-        if (empty($challenge) ||
-            empty($id) ||
+        try {
+            $challenge = hash($this->hashAlg, random_bytes($this->challengeLength));
+        } catch (Exception $ex) {
+            http_response_code(500);
+            throw new Exception('ChallengeManager.generateChallenge: Error while generating a challenge');
+        }
+
+        if (empty($id) ||
             empty($scriptName) ||
             !$this->challengeDbCmd->insertChallenge($challenge, $id, $scriptName)) {
             Logger::error(self::LOG_PREFIX . 'Error while creating a challenge');
             http_response_code(500);
-            return false;
+            throw new Exception('ChallengeManager.generateChallenge: Error while generating a challenge');
+        }
+        return $challenge;
+    }
+
+    public function generateToken($id, $scriptName, $data)
+    {
+
+        $challenge = $this->generateChallenge($id, $scriptName);
+
+        if (empty($challenge)) {
+            Logger::error('Retrieving the challenge was not successful.');
+            return;
         }
 
-        return true;
+        $jwk = JWKFactory::createFromKeyFile($this->privKey);
+        $algorithmManager = new AlgorithmManager(
+            [
+                self::getAlgorithm('Signature\\Algorithm', $this->sighAlg)
+            ]
+        );
+        $jwsBuilder = new JWSBuilder($algorithmManager);
+        $payload = json_encode([
+            'iat' => time(),
+            'nbf' => time(),
+            'exp' => time() + 300,
+            'challenge' => $challenge,
+            'id' => $id,
+            'data' => $data
+        ]);
+
+        $jws = $jwsBuilder
+            ->create()
+            ->withPayload($payload)
+            ->addSignature($jwk, ['alg' => $this->sighAlg])
+            ->build();
+
+        $serializer = new CompactSerializer();
+        $token = $serializer->serialize($jws, 0);
+
+        return $token;
     }
 
-    public function readChallengeFromDb($id)
+    public function decodeToken($token)
+    {
+        $algorithmManager = new AlgorithmManager(
+            [
+                self::getAlgorithm('Signature\\Algorithm', $this->sighAlg)
+            ]
+        );
+        $jwsVerifier = new JWSVerifier($algorithmManager);
+        $jwk = JWKFactory::createFromKeyFile($this->pubKey);
+
+        $serializerManager = new JWSSerializerManager([new CompactSerializer()]);
+        $jws = $serializerManager->unserialize($token);
+
+        $headerCheckerManager = new HeaderCheckerManager(
+            [new AlgorithmChecker([$this->sighAlg])],
+            [new JWSTokenSupport()]
+        );
+
+        $headerCheckerManager->check($jws, 0);
+
+        $isVerified = $jwsVerifier->verifyWithKey($jws, $jwk, 0);
+
+        if (!$isVerified) {
+            Logger::error('Perun.updateUes: The token signature is invalid!');
+            http_response_code(401);
+            exit;
+        }
+
+        $claimCheckerManager = new ClaimCheckerManager(
+            [
+                new Checker\IssuedAtChecker(),
+                new Checker\NotBeforeChecker(),
+                new Checker\ExpirationTimeChecker(),
+            ]
+        );
+        $claims = json_decode($jws->getPayload(), true);
+
+        $claimCheckerManager->check($claims);
+
+        $challenge = $claims['challenge'];
+        $id = $claims['id'];
+
+        $challengeManager = new ChallengeManager();
+
+        $challengeDb = $challengeManager->readChallengeFromDb($id);
+
+        $checkAccessSucceeded = self::checkAccess($challenge, $challengeDb);
+        $challengeSuccessfullyDeleted = $challengeManager->deleteChallengeFromDb($id);
+
+        if (!$checkAccessSucceeded || !$challengeSuccessfullyDeleted) {
+            exit;
+        }
+
+        return $claims;
+    }
+
+    private function readChallengeFromDb($id)
     {
         if (empty($id)) {
             http_response_code(400);
@@ -45,7 +177,7 @@ public function readChallengeFromDb($id)
         return $result;
     }
 
-    public static function checkAccess($challenge, $challengeDb): bool
+    private static function checkAccess($challenge, $challengeDb): bool
     {
         if (empty($challenge) || empty($challengeDb)) {
             http_response_code(400);
@@ -61,7 +193,7 @@ public static function checkAccess($challenge, $challengeDb): bool
         return true;
     }
 
-    public function deleteChallengeFromDb($id): bool
+    private function deleteChallengeFromDb($id): bool
     {
         if (empty($id)) {
             http_response_code(400);
@@ -76,4 +208,13 @@ public function deleteChallengeFromDb($id): bool
 
         return true;
     }
+
+    private static function getAlgorithm($path, $className)
+    {
+        $classPath = sprintf('Jose\\Component\\%s\\%s', $path, $className);
+        if (! class_exists($classPath)) {
+            throw new \Exception('Invalid algorithm specified: ' . $classPath);
+        }
+        return new $classPath();
+    }
 }