Add csrf token validation to internal api calls to improve security (…

…thanks @Ccamm)
Cockpit-HQ · Jun 7, 2023 · ef00d03 · ef00d03
1 parent eb14808
commit ef00d03
Show file tree

Hide file tree

Showing 22 changed files with 97 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,10 +8,11 @@
 - Improve role permissions UI
 - Show locale value picker only if multiple locales are available
 - Use `crypto.randomUUID()` only if available
-- Populate only allowed models in content api
-- Fix possible content `models` permission naming collision
-- Fix internal content find/populate api exposing data to users without required rights
+- Populate only allowed models in content api (thanks @Ccamm)
+- Fix possible content `models` permission naming collision (thanks @raffaelj)
+- Fix internal content find/populate api exposing data to users without required rights (thanks @raffaelj)
 - Fix empty settings screen
+- Add csrf token validation to internal api calls to improve security (thanks @Ccamm)
 
 ## 2.5.2 (2023-05-18)
 

diff --git a/modules/App/Controller/Authenticated.php b/modules/App/Controller/Authenticated.php
@@ -33,6 +33,18 @@ protected function isAllowed(string $permission): bool {
         return $this->helper('acl')->isAllowed($permission);
     }
 
+    protected function hasValidCsrfToken(bool $stop = false) {
+
+        $csrf = $this->app->request->headers['X-Csrf-Token'] ?? '';
+        $check = $this->helper('csrf')->isValid('app-csrf', $csrf, true);
+
+        if (!$check && $stop) {
+            return $this->stop(412);
+        }
+
+        return $check;
+    }
+
     protected function checkAndLockResource($resourceId) {
 
         $meta = null;

diff --git a/modules/App/Controller/Utils.php b/modules/App/Controller/Utils.php
@@ -24,6 +24,9 @@ public function csrf($name = null, $generate = false, $expire = null) {
 
     public function search() {
 
+        $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
+
         $findings = new \ArrayObject([]);
         $search = $this->param('search');
 

diff --git a/modules/App/assets/app.bundle.js b/modules/App/assets/app.bundle.js
@@ -3824,6 +3824,7 @@
 
       base_url: baseUrl,
       route_url: routeUrl,
+      csrf: (html.getAttribute("data-csrf") || undefined),
       version: (html.getAttribute("data-version") || '0.0.1'),
 
       _events: {},
@@ -3853,6 +3854,8 @@
           url = this.route(url);
           type = type || 'json';
 
+          let csrf = this.csrf;
+
           return new Promise(function (fulfill, reject) {
 
               let xhr = new XMLHttpRequest();
@@ -3862,6 +3865,8 @@
 
               url += (url.indexOf('?') !== -1 ? '&' : '?') + 'nc=' + Math.random().toString(36).substr(2);
 
+              data = data || {};
+
               if (data) {
 
                   if (typeof (data) === 'object' && data instanceof HTMLFormElement) {
@@ -3873,6 +3878,10 @@
                   }
               }
 
+              if (csrf) {
+                  xhr.setRequestHeader('X-CSRF-TOKEN', csrf);
+              }
+
               xhr.onloadend = function () {
 
                   let resdata = xhr.responseText;

diff --git a/modules/App/assets/js/app.js b/modules/App/assets/js/app.js
@@ -18,6 +18,7 @@ let App = {
 
     base_url: baseUrl,
     route_url: routeUrl,
+    csrf: (html.getAttribute("data-csrf") || undefined),
     version: (html.getAttribute("data-version") || '0.0.1'),
 
     _events: {},
@@ -47,6 +48,8 @@ let App = {
         url = this.route(url);
         type = type || 'json';
 
+        let csrf = this.csrf;
+
         return new Promise(function (fulfill, reject) {
 
             let xhr = new XMLHttpRequest();
@@ -56,6 +59,8 @@ let App = {
 
             url += (url.indexOf('?') !== -1 ? '&' : '?') + 'nc=' + Math.random().toString(36).substr(2);
 
+            data = data || {};
+
             if (data) {
 
                 if (typeof (data) === 'object' && data instanceof HTMLFormElement) {
@@ -69,6 +74,10 @@ let App = {
                 }
             }
 
+            if (csrf) {
+                xhr.setRequestHeader('X-CSRF-TOKEN', csrf);
+            }
+
             xhr.onloadend = function () {
 
                 let resdata = xhr.responseText;

diff --git a/modules/App/layouts/app.php b/modules/App/layouts/app.php
@@ -5,7 +5,7 @@
     $sidePanelContents = $this->block('app-side-panel', ['print' => false]);
 
 ?><!DOCTYPE html>
-<html lang="en" class="<?=$this->helper('theme')->pageClass()?>" data-base="<?=rtrim($this->baseUrl('/'), '/')?>" data-route="<?=rtrim($this->routeUrl('/'), '/')?>" data-version="<?=$this->retrieve('app.version')?>" data-theme="<?=$this->helper('theme')->theme()?>">
+<html lang="en" class="<?=$this->helper('theme')->pageClass()?>" data-base="<?=rtrim($this->baseUrl('/'), '/')?>" data-route="<?=rtrim($this->routeUrl('/'), '/')?>" data-csrf="<?=$this->helper('csrf')->token('app-csrf')?>" data-version="<?=$this->retrieve('app.version')?>" data-theme="<?=$this->helper('theme')->theme()?>">
 <head>
     <meta charset="UTF-8">
     <meta name="viewport" content="width=device-width, initial-scale=1.0">

diff --git a/modules/Assets/Controller/Assets.php b/modules/Assets/Controller/Assets.php
@@ -9,7 +9,6 @@
 
 class Assets extends App {
 
-
     public function index() {
 
         $this->helper('theme')->favicon('assets:icon.svg');
@@ -20,6 +19,7 @@ public function index() {
     public function assets() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         $options = array_merge([
             'sort' => ['_created' => -1]
@@ -110,6 +110,8 @@ public function assets() {
 
     public function asset($id = null) {
 
+        $this->hasValidCsrfToken(true);
+
         if (!$id) {
             return false;
         }
@@ -122,6 +124,7 @@ public function asset($id = null) {
     public function update() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         if (!$this->isAllowed('assets/edit')) {
             return $this->stop(['error' => 'Editing not allowed'], 401);
@@ -137,6 +140,7 @@ public function update() {
     public function upload() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         if (!$this->isAllowed('assets/upload')) {
             return $this->stop(['error' => 'Upload not allowed'], 401);
@@ -150,6 +154,7 @@ public function upload() {
     public function replace() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         if (!$this->isAllowed('assets/upload')) {
             return $this->stop(['error' => 'Upload not allowed'], 401);
@@ -190,6 +195,7 @@ public function replace() {
     public function remove() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         if (!$this->isAllowed('assets/delete')) {
             return $this->stop(['error' => 'Deleting assets not allowed'], 401);
@@ -204,6 +210,8 @@ public function remove() {
 
     public function folders() {
 
+        $this->hasValidCsrfToken(true);
+
         $folders = $this->module('assets')->folders(['sort' => ['name' => 1]]);
         $folders = $this->helper('utils')->buildTreeList($folders, ['parent_id_column_name' => '_p']);
 
@@ -212,6 +220,8 @@ public function folders() {
 
     public function saveFolder() {
 
+        $this->hasValidCsrfToken(true);
+
         $name   = $this->param('name', null);
         $parent = $this->param('parent', '');
 
@@ -240,6 +250,8 @@ public function saveFolder() {
 
     public function removeFolder() {
 
+        $this->hasValidCsrfToken(true);
+
         if (!$this->isAllowed('assets/folders/delete')) {
             return $this->stop(['error' => 'Deleting folders not allowed'], 401);
         }

diff --git a/modules/Assets/assets/dialogs/asset.js b/modules/Assets/assets/dialogs/asset.js
@@ -212,6 +212,9 @@ export default {
                 browserBackButtonClose: false
             }).use(Uppy.XHRUpload, {
                 endpoint: App.route('/assets/replace'),
+                headers: {
+                    'X-CSRF-TOKEN': App.csrf
+                },
                 bundle: true
             }).use(Uppy.Webcam, { target: Uppy.Dashboard, showVideoSourceDropdown: true })
             .use(Uppy.ScreenCapture, { target: Uppy.Dashboard })

diff --git a/modules/Assets/assets/vue-components/assets-manager.js b/modules/Assets/assets/vue-components/assets-manager.js
@@ -17,6 +17,9 @@ function getUppy(meta = {}) {
         browserBackButtonClose: false
     }).use(Uppy.XHRUpload, {
         endpoint: App.route('/assets/upload'),
+        headers: {
+            'X-CSRF-TOKEN': App.csrf
+        },
         bundle: true
     }).use(Uppy.Webcam, { target: Uppy.Dashboard, showVideoSourceDropdown: true })
     .use(Uppy.ScreenCapture, { target: Uppy.Dashboard })

diff --git a/modules/Content/Controller/Collection.php b/modules/Content/Controller/Collection.php
@@ -128,6 +128,7 @@ public function clone($model = null, $id = null) {
     public function find($model = null) {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         if (!$model) {
             return false;
@@ -236,6 +237,7 @@ public function find($model = null) {
     public function remove($model = null) {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         $model = $this->module('content')->model($model);
         $ids = $this->param('ids');
@@ -260,6 +262,7 @@ public function remove($model = null) {
     public function updateState($model = null) {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         $model = $this->module('content')->model($model);
         $ids = $this->param('ids');
@@ -290,6 +293,9 @@ public function updateState($model = null) {
 
     public function batchUpdate($model = null) {
 
+        $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
+
         $model = $this->module('content')->model($model);
         $data = $this->param('data');
         $filter = $this->param('filter');

diff --git a/modules/Content/Controller/Content.php b/modules/Content/Controller/Content.php
@@ -17,6 +17,7 @@ public function index() {
     public function populate() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         $locale = $this->param('locale', 'default');
         $lvl = $this->param('lvl', 10);

diff --git a/modules/Finder/Controller/Buckets.php b/modules/Finder/Controller/Buckets.php
@@ -12,6 +12,7 @@ class Buckets extends App {
     public function api(?string $bucket = null) {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         $cmd = $this->param('cmd', false);
 

diff --git a/modules/Finder/Controller/Finder.php b/modules/Finder/Controller/Finder.php
@@ -26,11 +26,12 @@ public function index() {
     public function api() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         $this->root = $this->app->path('#root:');
         $cmd = $this->param('cmd', false);
 
-        if (file_exists($this->root) && in_array($cmd, get_class_methods($this))){
+        if (file_exists($this->root) && in_array($cmd, get_class_methods($this))) {
 
             $this->app->response->mime = 'json';
             return $this->{$cmd}();

diff --git a/modules/Finder/assets/dialogs/bucket-picker.js b/modules/Finder/assets/dialogs/bucket-picker.js
@@ -251,6 +251,9 @@ export default {
 
             xhr.open('POST', App.route(`/finder/buckets/api/${this.bucket}`));
 
+            xhr.setRequestHeader('X-CSRF-TOKEN', App.csrf);
+            xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
+
             this.uploading = 0;
 
             xhr.upload.addEventListener('progress', ({loaded, total}) => {

diff --git a/modules/Finder/assets/vue-components/finder.js b/modules/Finder/assets/vue-components/finder.js
@@ -273,6 +273,9 @@ export default {
 
             xhr.open('POST', App.route('/finder/api'));
 
+            xhr.setRequestHeader('X-CSRF-TOKEN', App.csrf);
+            xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
+
             this.uploading = 0;
 
             xhr.upload.addEventListener('progress', ({loaded, total}) => {

diff --git a/modules/System/Controller/Api.php b/modules/System/Controller/Api.php
@@ -70,6 +70,8 @@ public function create() {
 
     public function remove() {
 
+        $this->hasValidCsrfToken(true);
+
         $key = $this->param('key');
 
         if (!$key || !isset($key['_id'], $key['key'])) {

diff --git a/modules/System/Controller/Locales.php b/modules/System/Controller/Locales.php
@@ -56,6 +56,8 @@ public function create() {
 
     public function remove() {
 
+        $this->hasValidCsrfToken(true);
+
         $locale = $this->param('locale');
 
         if (!$locale || !isset($locale['_id'], $locale['i18n'])) {
@@ -73,6 +75,8 @@ public function remove() {
 
     public function save() {
 
+        $this->hasValidCsrfToken(true);
+
         $locale = $this->param('locale');
 
         if (!$locale) {
@@ -121,6 +125,7 @@ public function save() {
     public function load() {
 
         $this->helper('session')->close();
+        $this->hasValidCsrfToken(true);
 
         $locales = $this->app->dataStorage->find('system/locales', [
             'sort' => ['name' => 1]
@@ -132,4 +137,4 @@ public function load() {
     protected function cache() {
         $this->helper('locales')->cache();
     }
-}
+}
diff --git a/modules/System/Controller/Spaces.php b/modules/System/Controller/Spaces.php
@@ -28,6 +28,8 @@ public function create() {
 
         if ($space) {
 
+            $this->hasValidCsrfToken(true);
+
             if (!isset($space['name'])) {
                 return $this->stop(404);
             }
@@ -43,7 +45,6 @@ public function create() {
             return ['success' => true, 'space' => $space];
         }
 
-
         return $this->render('system:views/spaces/create.php');
     }
 

diff --git a/modules/System/Controller/Tower.php b/modules/System/Controller/Tower.php
@@ -29,6 +29,8 @@ public function index() {
 
     public function exec() {
 
+        $this->hasValidCsrfToken(true);
+
         $command = trim($this->param('command', ''));
 
         if (!$command) {