From b3eb5817abc9c23d44ad4a55697e123a035e1e7c Mon Sep 17 00:00:00 2001
From: Ad Schellevis <ad@opnsense.org>
Date: Tue, 24 Sep 2024 11:02:09 +0200
Subject: [PATCH] System: Trust: Certificates - add proper validation when
 certs are being imported for CSR's.

If we don't know the issuer, according to security standards. we should prevent the new certificate being imported.
While here, wrap a recurring pattern for proc_open() in our Store implementation and keep the CSR for reuse after import.
---
 .../OPNsense/Trust/Api/CertController.php     | 15 ++--
 .../mvc/app/library/OPNsense/Trust/Store.php  | 71 ++++++++-----------
 2 files changed, 41 insertions(+), 45 deletions(-)

diff --git a/src/opnsense/mvc/app/controllers/OPNsense/Trust/Api/CertController.php b/src/opnsense/mvc/app/controllers/OPNsense/Trust/Api/CertController.php
index c34662dd7..93e20eb9b 100644
--- a/src/opnsense/mvc/app/controllers/OPNsense/Trust/Api/CertController.php
+++ b/src/opnsense/mvc/app/controllers/OPNsense/Trust/Api/CertController.php
@@ -114,11 +114,18 @@ class CertController extends ApiMutableModelControllerBase
                 $this->getModel()->linkCaRefs($node->refid);
                 break;
             case 'import_csr':
-                if (CertStore::parseX509((string)$node->crt_payload) === false) {
-                    $error = gettext('Invalid X509 certificate provided');
-                } else {
+                /* certificate should be signed by something we trust */
+                $tmp = CertStore::verify((string)$node->crt_payload);
+                if ($tmp['exit_status'] === 0) {
                     $node->crt = base64_encode((string)$node->crt_payload);
-                    $node->csr = null;
+                } else {
+                    /* try to grab some useful feedback from stderr to append to the message */
+                    $msg = '';
+                    $parts = explode("\n", $tmp['stderr']);
+                    if (count($parts) > 2) {
+                        $msg = $parts[1];
+                    }
+                    $error = sprintf(gettext('Invalid X509 certificate provided : %s'), $msg);
                 }
                 break;
             case 'sign_csr':
diff --git a/src/opnsense/mvc/app/library/OPNsense/Trust/Store.php b/src/opnsense/mvc/app/library/OPNsense/Trust/Store.php
index bd0e41637..1d3c790a1 100644
--- a/src/opnsense/mvc/app/library/OPNsense/Trust/Store.php
+++ b/src/opnsense/mvc/app/library/OPNsense/Trust/Store.php
@@ -512,51 +512,57 @@ class Store
     }
 
     /**
-     * @param string $cert certificate
-     * @return array [stdout|stderr]
+     * wrapper around proc_open()
+     * @param string $cmd command to execute
+     * @param string $stdin data to push into <stdin>
+     * @return array [stdout|stderr|exit_status]
      */
-    public static function dumpX509($cert)
+    private static function proc_open(string $cmd, string $stdin)
     {
-        $result = [];
+        $result = ['exit_status' => -1, 'stderr' => '', 'stdout' => ''];
         $process = proc_open(
-            '/usr/local/bin/openssl x509 -fingerprint -sha256 -text',
+            $cmd,
             [["pipe", "r"], ["pipe", "w"], ["pipe", "w"]],
             $pipes
         );
         if (is_resource($process)) {
-            fwrite($pipes[0], $cert);
+            fwrite($pipes[0], $stdin);
             fclose($pipes[0]);
             $result['stdout'] = stream_get_contents($pipes[1]);
             fclose($pipes[1]);
             $result['stderr'] = stream_get_contents($pipes[2]);
             fclose($pipes[2]);
-            proc_close($process);
+            $result['exit_status'] = proc_close($process);
         }
         return $result;
     }
 
+    /**
+     * verify offered cert agains local trust store
+     * @param string $cert certificate
+     * @return array [stdout|stderr|exit_status]
+     */
+    public static function verify($cert)
+    {
+        return static::proc_open('/usr/local/bin/openssl verify', $cert);
+    }
+
+    /**
+     * @param string $cert certificate
+     * @return array [stdout|stderr]
+     */
+    public static function dumpX509($cert)
+    {
+        return static::proc_open('/usr/local/bin/openssl x509 -fingerprint -sha256 -text', $cert);
+    }
+
     /**
      * @param string $csr CSR
      * @return array [stdout|stderr]
      */
     public static function dumpCSR($csr)
     {
-        $result = [];
-        $process = proc_open(
-            '/usr/local/bin/openssl req -text -noout',
-            [["pipe", "r"], ["pipe", "w"], ["pipe", "w"]],
-            $pipes
-        );
-        if (is_resource($process)) {
-            fwrite($pipes[0], $csr);
-            fclose($pipes[0]);
-            $result['stdout'] = stream_get_contents($pipes[1]);
-            fclose($pipes[1]);
-            $result['stderr'] = stream_get_contents($pipes[2]);
-            fclose($pipes[2]);
-            proc_close($process);
-        }
-        return $result;
+        return static::proc_open('/usr/local/bin/openssl req -text -noout', $csr);
     }
 
     /**
@@ -565,26 +571,9 @@ class Store
      */
     public static function dumpCRL($cert)
     {
-        $result = [];
-        $process = proc_open(
-            '/usr/local/bin/openssl crl -fingerprint -sha256 -text',
-            [["pipe", "r"], ["pipe", "w"], ["pipe", "w"]],
-            $pipes
-        );
-        if (is_resource($process)) {
-            fwrite($pipes[0], $cert);
-            fclose($pipes[0]);
-            $result['stdout'] = stream_get_contents($pipes[1]);
-            fclose($pipes[1]);
-            $result['stderr'] = stream_get_contents($pipes[2]);
-            fclose($pipes[2]);
-            proc_close($process);
-        }
-        return $result;
+        return static::proc_open('/usr/local/bin/openssl crl -fingerprint -sha256 -text', $cert);
     }
 
-
-
     /**
      * Extract certificate chain
      * @param string $caref reference number