CVE-2023-37306: Generation of Error Message Containing Sensitive Information (CWE-209)
value: MISP 2.4.172 mishandles different certificate file extensions in server sync. An attacker can obtain sensitive information because of the nature of the error messages.
CWE description: The product generates an error message that includes sensitive information about its environment, users, or associated data.
CVSS v3 base score: 7.5
Published: 2023-06-30T17:15Z
Repository: https://github.com/MISP/MISP
Language: PHP
Files changed: app/Controller/ServersController.php, app/Lib/Tools/SyncTool.php
Commit date: 2023-06-28 09:42:12 +0200
Commit message: fix: properly handle different cert file extensions in server sync. #9084

Diff: diff --git a/app/Controller/ServersController.php b/app/Controller/ServersController.php
index 16dff24a431..67699cbf4ad 100644
--- a/app/Controller/ServersController.php
+++ b/app/Controller/ServersController.php
@@ -913,34 +913,56 @@ class ServersController extends AppController
         if (!$delete) {
             $ext = '';
             App::uses('File', 'Utility');
             App::uses('Folder', 'Utility');
             App::uses('FileAccessTool', 'Tools');
+            App::uses('SyncTool', 'Tools');
             if (isset($server['Server'][$subm]['name'])) {
                 if ($this->request->data['Server'][$subm]['size'] != 0) {
                     if (!$this->Server->checkFilename($server['Server'][$subm]['name'])) {
                         throw new Exception(__('Filename not allowed'));
                     }
-                    $file = new File($server['Server'][$subm]['name']);
-                    $ext = $file->ext();
+
+                    if (!is_uploaded_file($server['Server'][$subm]['tmp_name'])) {
+                        throw new Exception(__('File not uploaded correctly'));
+                    }
+
+                    $ext = pathinfo($server['Server'][$subm]['name'], PATHINFO_EXTENSION);
+                    if (!in_array($ext, SyncTool::ALLOWED_CERT_FILE_EXTENSIONS)) {
+                        $this->Flash->error(__('Invalid extension.'));
+                        $this->redirect(array('action' => 'index'));
+                    }
+
                     if (!$server['Server'][$subm]['size'] > 0) {
                         $this->Flash->error(__('Incorrect extension or empty file.'));
                         $this->redirect(array('action' => 'index'));
                     }
 
-                    // read pem file data
-                    $pemData = FileAccessTool::readFromFile($server['Server'][$subm]['tmp_name'], $server['Server'][$subm]['size']);
+                    // read certificate file data
+                    $certData = FileAccessTool::readFromFile($server['Server'][$subm]['tmp_name'], $server['Server'][$subm]['size']);
                 } else {
                     return true;
                 }
             } else {
-                $pemData = base64_decode($server['Server'][$subm]);
+                $ext = 'pem';
+                $certData = base64_decode($server['Server'][$subm]);
             }
+
+            // check if the file is a valid x509 certificate
+            try {
+                $cert = openssl_x509_parse($certData);
+                if (!$cert) {
+                    throw new Exception(__('Invalid certificate.'));
+                }
+            } catch (Exception $e) {
+                $this->Flash->error(__('Invalid certificate.'));
+                $this->redirect(array('action' => 'index'));
+            }
+
             $destpath = APP . "files" . DS . "certs" . DS;
-            $dir = new Folder(APP . "files" . DS . "certs", true);
             $pemfile = new File($destpath . $id . $ins . '.' . $ext);
-            $result = $pemfile->write($pemData);
+            $result = $pemfile->write($certData);
             $s = $this->Server->read(null, $id);
             $s['Server'][$attr] = $s['Server']['id'] . $ins . '.' . $ext;
             if ($result) {
                 $this->Server->save($s);
             }
diff --git a/app/Lib/Tools/SyncTool.php b/app/Lib/Tools/SyncTool.php
index aa621879bd3..157123d21bd 100644
--- a/app/Lib/Tools/SyncTool.php
+++ b/app/Lib/Tools/SyncTool.php
@@ -1,9 +1,12 @@
 <?php
 
 class SyncTool
 {
+
+    const ALLOWED_CERT_FILE_EXTENSIONS = ['pem', 'crt'];
+
     /**
      * Take a server as parameter and return a HttpSocket object using the ssl options defined in the server settings
      * @param array|null $server
      * @param false $timeout
      * @param string $model
@@ -13,14 +16,14 @@ class SyncTool
     public function setupHttpSocket($server = null, $timeout = false, $model = 'Server')
     {
         $params = ['compress' => true];
         if (!empty($server)) {
             if (!empty($server[$model]['cert_file'])) {
-                $params['ssl_cafile'] = APP . "files" . DS . "certs" . DS . $server[$model]['id'] . '.pem';
+                $params['ssl_cafile'] = APP . "files" . DS . "certs" . DS . $server[$model]['cert_file'];
             }
             if (!empty($server[$model]['client_cert_file'])) {
-                $params['ssl_local_cert'] = APP . "files" . DS . "certs" . DS . $server[$model]['id'] . '_client.pem';
+                $params['ssl_local_cert'] = APP . "files" . DS . "certs" . DS . $server[$model]['client_cert_file'];
             }
             if (!empty($server[$model]['self_signed'])) {
                 $params['ssl_allow_self_signed'] = true;
                 $params['ssl_verify_peer_name'] = false;
                 if (!isset($server[$model]['cert_file'])) {

Vulnerable code:                     $file = new File($server['Server'][$subm]['name']);
                    $ext = $file->ext();
                    // read pem file data
                    $pemData = FileAccessTool::readFromFile($server['Server'][$subm]['tmp_name'], $server['Server'][$subm]['size']);
                $pemData = base64_decode($server['Server'][$subm]);
            $dir = new Folder(APP . "files" . DS . "certs", true);
            $result = $pemfile->write($pemData);
                $params['ssl_cafile'] = APP . "files" . DS . "certs" . DS . $server[$model]['id'] . '.pem';
                $params['ssl_local_cert'] = APP . "files" . DS . "certs" . DS . $server[$model]['id'] . '_client.pem';
Fixed code:             App::uses('SyncTool', 'Tools');

                    if (!is_uploaded_file($server['Server'][$subm]['tmp_name'])) {
                        throw new Exception(__('File not uploaded correctly'));
                    }

                    $ext = pathinfo($server['Server'][$subm]['name'], PATHINFO_EXTENSION);
                    if (!in_array($ext, SyncTool::ALLOWED_CERT_FILE_EXTENSIONS)) {
                        $this->Flash->error(__('Invalid extension.'));
                        $this->redirect(array('action' => 'index'));
                    }

                    // read certificate file data
                    $certData = FileAccessTool::readFromFile($server['Server'][$subm]['tmp_name'], $server['Server'][$subm]['size']);
                $ext = 'pem';
                $certData = base64_decode($server['Server'][$subm]);

            // check if the file is a valid x509 certificate
            try {
                $cert = openssl_x509_parse($certData);
                if (!$cert) {
                    throw new Exception(__('Invalid certificate.'));
                }
            } catch (Exception $e) {
                $this->Flash->error(__('Invalid certificate.'));
                $this->redirect(array('action' => 'index'));
            }

            $result = $pemfile->write($certData);

    const ALLOWED_CERT_FILE_EXTENSIONS = ['pem', 'crt'];

                $params['ssl_cafile'] = APP . "files" . DS . "certs" . DS . $server[$model]['cert_file'];
                $params['ssl_local_cert'] = APP . "files" . DS . "certs" . DS . $server[$model]['client_cert_file'];