Merge pull request #20 from Vectorial1024/security

Secure the task runners

Author: Vectorial1024

README.md

@@ -116,6 +116,19 @@ Some tips:
 - If multiple tasks are started with the same task ID, then the task status object will only track the first task that was started
 - Known issue: on Windows, checking task statuses can be slow (about 0.5 - 1 seconds) due to underlying bottlenecks
 
+### Securing the task runners
+The way this library works means that attackers (or other unwanted parties) may simply craft malicious commands that mimic legitimate usage of this library.
+
+To secure the task runners from being started illegitimately, you may configure the `.env` file to contain the following key:
+
+```
+PROCESS_ASYNC_SECRET_KEY=[your secret key here]
+```
+
+You may need to clear your Laravel optimisation cache after changing this value.
+
+The contents of the async tasks will be signed by this secret key, so that this library can know whether the tasks are started by this library itself or someone else.
+
 ## Testing
 PHPUnit via Composer script:
 ```sh

src/AsyncTask.php

@@ -13,7 +13,10 @@
 use loophp\phposinfo\OsInfo;
 use RuntimeException;
 
-use function Opis\Closure\{serialize, unserialize};
+use function Opis\Closure\init;
+use function Opis\Closure\serialize;
+use function Opis\Closure\set_security_provider;
+use function Opis\Closure\unserialize;
 
 /**
  * The common handler of an AsyncTask; this can be a closure (will be wrapped inside AsyncTask) or an interface instance.
@@ -147,6 +150,26 @@ public function __unserialize($data): void
     }
 
     /**
+     * Loads (or reloads) the secret key used by this library.
+     * 
+     * Normally, this function does not need to be called by linrary users.
+     * @return void
+     */
+    public static function loadSecretKey(): void
+    {
+        // read from the env file for the secret key (if exists) to verify our identity
+        $secretKey = env("PROCESS_ASYNC_SECRET_KEY");
+        init(null);
+        if ($secretKey != null && strlen($secretKey) > 0) {
+            // we can set the secret key
+            set_security_provider($secretKey);
+        } else {
+            // no secret key given, clear the security
+            set_security_provider(null);
+        }
+    }
+
+    /*
      * Returns a status object for the started AsyncTask.
      * 
      * If this task does not have an explicit task ID, a new one will be generated on-the-fly.

src/AsyncTaskRunnerCommand.php

@@ -3,6 +3,7 @@
 namespace Vectorial1024\LaravelProcessAsync;
 
 use Illuminate\Console\Command;
+use Opis\Closure\Security\SecurityException;
 
 /**
  * The Artisan command to run AsyncTask. DO NOT USE DIRECTLY!
@@ -23,6 +24,8 @@ class AsyncTaskRunnerCommand extends Command
      */
     protected $description = 'Runs a background async task (DO NOT USE DIRECTLY)';
 
+    protected $hidden = true;
+
     /**
      * Execute the console command.
      *
@@ -33,7 +36,13 @@ public function handle(): int
         // first, unpack the task
         // (Symfony already safeguards the "task" argument to make it required)
         $theTask = $this->argument('task');
-        $theTask = AsyncTask::fromBase64Serial($theTask);
+        try {
+            $theTask = AsyncTask::fromBase64Serial($theTask);
+        } catch (SecurityException $x) {
+            // bad secret key; cannot verify sender identity
+            $this->error("Unrecognized task giver is trying to start AsyncTaskRunner.");
+            return self::FAILURE;
+        }
         if ($theTask === null) {
             // bad underializing; probably bad data
             $this->error("Invalid task details!");

src/AsyncTaskStatus.php

@@ -128,7 +128,6 @@ private function findTaskRunnerProcess(): bool
             // we can assume we are in cmd, but wcim in cmd is deprecated, and the replacement gcim requires powershell
             $results = [];
             $fullCmd = "powershell echo \"\"(gcim Win32_Process -Filter \\\"CommandLine LIKE '%id=\'$encodedTaskID\'%'\\\").ProcessId\"\"";
-            \Illuminate\Support\Facades\Log::info($fullCmd);
             exec("powershell echo \"\"(gcim Win32_Process -Filter \\\"CommandLine LIKE '%id=\'$encodedTaskID\'%'\\\").ProcessId\"\"", $results);
             // will output many lines, each line being a PID
             foreach ($results as $candidatePID) {

src/ProcessAsyncServiceProvider.php

@@ -11,7 +11,8 @@ class ProcessAsyncServiceProvider extends ServiceProvider
      */
     public function register(): void
     {
-        // pass
+        // load/reset our secret key
+        AsyncTask::loadSecretKey();
     }
 
     /**

tests/AsyncTaskTest.php

@@ -4,6 +4,7 @@
 
 use InvalidArgumentException;
 use LogicException;
+use Opis\Closure\Security\SecurityException;
 use RuntimeException;
 use Vectorial1024\LaravelProcessAsync\AsyncTask;
 use Vectorial1024\LaravelProcessAsync\AsyncTaskStatus;
@@ -13,10 +14,18 @@
 use Vectorial1024\LaravelProcessAsync\Tests\Tasks\TestTimeoutErrorTask;
 use Vectorial1024\LaravelProcessAsync\Tests\Tasks\TestTimeoutNoOpTask;
 use Vectorial1024\LaravelProcessAsync\Tests\Tasks\TestTimeoutNormalTask;
+use function Opis\Closure\init;
+use function Opis\Closure\set_security_provider;
 
 class AsyncTaskTest extends BaseTestCase
 {
-    // directly running the runner 
+    public function setUp(): void
+    {
+        // we need to reset the security key in case some of our tests change the secret key
+        AsyncTask::loadSecretKey();
+    }
+
+    // directly running the runner
 
     public function testCanRunClosure()
     {
@@ -74,6 +83,42 @@ public function testConfigNoTimeLimit()
         $this->assertNull($task->getTimeLimit());
     }
 
+    public function testCanVerifySender()
+    {
+        // test that, when given a serialized closure signed by us, we can correctly reproduce the closure
+        $testSecretKey = "LaravelProcAsync";
+        $testClosure = fn () => "Hello there!";
+        // reset the security provider
+        set_security_provider(null);
+        init($testSecretKey);
+        // serialize once
+        $testTask = new AsyncTask($testClosure);
+        $checkingString = $testTask->toBase64Serial();
+        // then unserialize it
+        $reproducedTask = AsyncTask::fromBase64Serial($checkingString);
+        $this->assertTrue($reproducedTask instanceof AsyncTask);
+    }
+
+    public function testCannotVerifySender()
+    {
+        // test that, when given a serialized closure signed by Mallory, we can correctly reject the closure
+        $testSecretKey = "LaravelProcAsync";
+        $testMalloryKey = "HereComesDatMallory";
+        $testClosure = fn () => "Hello there!";
+        // reset the security provider
+        // mallory appears!
+        set_security_provider($testMalloryKey);
+        // serialize once
+        $testTask = new AsyncTask($testClosure);
+        $checkingString = $testTask->toBase64Serial();
+        // then unserialize it
+        set_security_provider($testSecretKey);
+        $this->expectException(SecurityException::class);
+        // should throw
+        $reproducedTask = AsyncTask::fromBase64Serial($checkingString);
+        unset($reproducedTask);
+    }
+
     // ---------
 
     // integration test with the cli artisan via a mocked artisan file, which tests various features of this library