From 443889e7bcee9ac5ef06afe9e8c6b25814b78b72 Mon Sep 17 00:00:00 2001
From: David Anderson <davea@ssl.berkeley.edu>
Date: Fri, 20 Dec 2024 19:19:05 -0800
Subject: [PATCH 1/2] web: fix various XSS vulnerabilities

Most of these involve putting user text in error messages.
Use htmlspecialchars() for this.

filenames: require POSIX portable names
---
 html/inc/util_basic.inc          | 28 +++++++++++++++++++++++++---
 html/user/get_output.php         | 10 +++++-----
 html/user/get_output3.php        |  3 ++-
 html/user/job_file.php           | 14 ++++++++++++--
 html/user/openid_login.php       | 10 ++--------
 html/user/prefs_edit.php         |  2 +-
 html/user/submit_rpc_handler.php |  8 ++++----
 7 files changed, 51 insertions(+), 24 deletions(-)

diff --git a/html/inc/util_basic.inc b/html/inc/util_basic.inc
index 754de0954d4..1cd39a34979 100644
--- a/html/inc/util_basic.inc
+++ b/html/inc/util_basic.inc
@@ -197,12 +197,34 @@ function dtime() {
     return microtime(true);
 }
 
+// security vulnerabilities and user-supplied strings:
+// sources:
+// GET and POST arguments
+//      including XML documents passed as args to RPC handlers
+// cookies
+//
+// when used as SQL query args:
+//      use BoincDb::escape_string() to prevent SQL injection
+// when shown as HTML output
+//      (e.g. 'not found' error pages, user names, forum posts)
+//      use htmlspecialchars() to prevent XSS
+// when used as file or dir name
+//      use is_valid_filename()
+
 // is $x a valid file (or dir) name?
+// we want to avoid
+//      FS traversal, e.g. "../../foo" or "/usr/lib/..."
+//      shell command injection, e.g. "foo; rm*"
+//      XSS stuff
+// let's be conservative and allow only 'POSIX fully portable filenames',
+// which can have only A-Z a-z 0-9 . - _
+// In some cases filenames are used on volunteer hosts,
+// whose OSs may have such restrictions.
 //
 function is_valid_filename($x) {
-    if (htmlspecialchars($x) != $x) return false;
-    if (strstr($x, '/')) return false;
-    return true;
+    if (strlen($x)>255) return false;
+    // \w means A-Za-z0-9_
+    return preg_match('/^[\w\-.]+$/', $x);
 }
 
 ?>
diff --git a/html/user/get_output.php b/html/user/get_output.php
index 1c5e69420f4..914ac8ac86d 100644
--- a/html/user/get_output.php
+++ b/html/user/get_output.php
@@ -33,7 +33,7 @@ function return_error($str) {
 function get_output_file($instance_name, $file_num, $auth_str) {
     $result = BoincResult::lookup_name(BoincDb::escape_string($instance_name));
     if (!$result) {
-        return_error("no job instance $instance_name");
+        return_error("no job instance ".htmlspecialchars($instance_name));
     }
     $workunit = BoincWorkunit::lookup_id($result->workunitid);
     if (!$workunit) {
@@ -124,7 +124,7 @@ function get_wu_output_file($wu_name, $file_num, $auth_str) {
     $wu_name = BoincDb::escape_string($wu_name);
     $wu = BoincWorkunit::lookup("name='$wu_name'");
     if (!$wu) {
-        return_error("no workunit $wu_name");
+        return_error("no workunit ".htmlspecialchars($wu_name));
     }
     $batch = BoincBatch::lookup_id($wu->batch);
     if (!$batch) {
@@ -140,7 +140,7 @@ function get_wu_output_file($wu_name, $file_num, $auth_str) {
     $fanout = parse_config(get_config(), "<uldl_dir_fanout>");
     $upload_dir = parse_config(get_config(), "<upload_dir>");
     if (!$wu->canonical_resultid) {
-        return_error("no canonical result for wu $wu->name");
+        return_error("no canonical result for wu ".htmlspecialchars($wu->name));
     }
     $result = BoincResult::lookup_id($wu->canonical_resultid);
     $names = get_outfile_phys_names($result);
@@ -148,7 +148,7 @@ function get_wu_output_file($wu_name, $file_num, $auth_str) {
     if (file_exists($path)) {
         do_download($path);
     } else {
-        return_error("no such file: $path");
+        return_error("no such file: ".htmlspecialchars($path));
     }
 }
 
@@ -178,7 +178,7 @@ function get_wu_output_files($wu_id, $auth_str) {
     $upload_dir = parse_config(get_config(), "<upload_dir>");
 
     if (!$wu->canonical_resultid) {
-        return_error("no canonical result for wu $wu->name");
+        return_error("no canonical result for wu ".htmlspecialchars($wu->name));
     }
     $result = BoincResult::lookup_id($wu->canonical_resultid);
     $names = get_outfile_phys_names($result);
diff --git a/html/user/get_output3.php b/html/user/get_output3.php
index b8f8fb7385b..b3fa5767d21 100644
--- a/html/user/get_output3.php
+++ b/html/user/get_output3.php
@@ -44,8 +44,9 @@ function get_file() {
 // download a zip of the given directory
 //
 function get_batch() {
-    $batch_id = get_str('batch_id');
+    $batch_id = get_int('batch_id');
     $dir = "../../results/$batch_id";
+    if (!is_dir($dir)) die('no batch dir');
     $name = "batch_$batch_id.zip";
     $cmd = "cd $dir; rm -f $name; zip -q $name *";
     system($cmd);
diff --git a/html/user/job_file.php b/html/user/job_file.php
index 01501381e00..0b7ea9ba696 100644
--- a/html/user/job_file.php
+++ b/html/user/job_file.php
@@ -17,7 +17,7 @@
 // along with BOINC.  If not, see <http://www.gnu.org/licenses/>.
 
 // Web RPCs for managing input files for remote job submission
-// These support systems where user - possibly lots of them -
+// These support systems where users - possibly lots of them -
 // process jobs without logging on to the BOINC server.
 //
 // Issues:
@@ -111,6 +111,9 @@ function query_files($r) {
     }
     $i = 0;
     foreach($phys_names as $fname) {
+        if (!is_valid_filename($fname)) {
+            xml_error(-1, 'bad filename');
+        }
         $path = dir_hier_path($fname, project_dir() . "/download", $fanout);
 
         // if the job_file record is there,
@@ -194,11 +197,18 @@ function upload_files($r) {
 
     $phys_names = array();
     foreach ($r->phys_name as $cs) {
-        $phys_names[] = (string)$cs;
+        $fname = (string)$cs;
+        if (!is_valid_filename($fname)) {
+            xml_error(-1, 'bad filename');
+        }
+        $phys_names[] = $fname;
     }
 
     foreach ($_FILES as $f) {
         $name = $f['name'];
+        if (!is_valid_filename($fname)) {
+            xml_error(-1, 'bad FILES filename');
+        }
         $tmp_name = $f['tmp_name'];
 
         if ($f['error'] != UPLOAD_ERR_OK) {
diff --git a/html/user/openid_login.php b/html/user/openid_login.php
index 407216ab6fa..befe87a4818 100644
--- a/html/user/openid_login.php
+++ b/html/user/openid_login.php
@@ -16,6 +16,8 @@
 // You should have received a copy of the GNU Lesser General Public License
 // along with BOINC.  If not, see <http://www.gnu.org/licenses/>.
 
+NOT FINISHED.   DON'T USE
+
 require 'openid.php';
 include_once("../inc/boinc_db.inc");
 include_once("../inc/util.inc");
@@ -25,8 +27,6 @@
 function show_error($str) {
     page_head("Can't create account");
     echo "$str<br>\n";
-    echo BoincDb::error();
-    echo "<p>Click your browser's <b>Back</b> button to try again.\n<p>\n";
     page_tail();
     exit();
 }
@@ -34,7 +34,6 @@ function show_error($str) {
 try {
     $openid = new LightOpenID;
     echo "<pre>";
-    print_r($openid); exit;
     if(!$openid->mode) {
         if(isset($_POST['openid_identifier'])) {
             $openid->identity = $_POST['openid_identifier'];
@@ -52,7 +51,6 @@ function show_error($str) {
         echo 'User has canceled authentication!';
     } else {
         echo 'User ' . ($openid->validate() ? $openid->identity . ' has ' : 'has not ') . 'logged in.';
-        //print_r($openid->getAttributes());
         // Create the user in the DB
         $data = $openid->getAttributes();
         $email_addr = $data['contact/email'];
@@ -67,7 +65,6 @@ function show_error($str) {
             error_page("Account creation is disabled");
         }
 
-
         // see whether the new account should be pre-enrolled in a team,
         // and initialized with its founder's project prefs
         //
@@ -95,9 +92,6 @@ function show_error($str) {
         //    }
         //}
 
-        print_r($data);
-        exit();
-
         $new_name = $data['namePerson/friendly'];
         if (!is_valid_user_name($new_name, $reason)) {
             show_error($reason);
diff --git a/html/user/prefs_edit.php b/html/user/prefs_edit.php
index db0b9299744..e16f026b2ec 100644
--- a/html/user/prefs_edit.php
+++ b/html/user/prefs_edit.php
@@ -26,7 +26,7 @@
 $action = sanitize_tags(get_str("action", true));
 $subset = sanitize_tags(get_str("subset"));
 $venue = sanitize_tags(get_str("venue", true));
-$columns = get_str("cols", true);
+$columns = get_int("cols", true);
 $c = $columns?"&cols=$columns":"";
 check_subset($subset);
 if ($action) {
diff --git a/html/user/submit_rpc_handler.php b/html/user/submit_rpc_handler.php
index b9c3f9f34e6..404e91706e6 100644
--- a/html/user/submit_rpc_handler.php
+++ b/html/user/submit_rpc_handler.php
@@ -34,7 +34,7 @@ function get_wu($name) {
     $wu = BoincWorkunit::lookup("name='$name'");
     if (!$wu) {
         log_write("no job named $name was found");
-        xml_error(-1, "no job named $name was found");
+        xml_error(-1, "job not found: ".htmlspecialchars($name));
     }
     return $wu;
 }
@@ -44,7 +44,7 @@ function get_submit_app($name) {
     $app = BoincApp::lookup("name='$name'");
     if (!$app) {
         log_write("no app named $name was found");
-        xml_error(-1, "no app named $name was found");
+        xml_error(-1, "app not found: ".htmlspecialchars($name));
     }
     return $app;
 }
@@ -103,7 +103,7 @@ function read_input_template($app, $r) {
         $x = simplexml_load_file($path);
         if (!$x) {
             log_write("couldn't parse input template file $path");
-            xml_error(-1, "couldn't parse input template file $path");
+            xml_error(-1, "couldn't parse input template file ".htmlspecialchars($path));
         }
         return $x;
     } else {
@@ -1096,7 +1096,7 @@ function ping($r) {
 $r = simplexml_load_string($req);
 if (!$r) {
     log_write("----- RPC request: can't parse request message: $req");
-    xml_error(-1, "can't parse request message: $req");
+    xml_error(-1, "can't parse request message: ".htmlspecialchars($req));
 }
 
 log_write("----- Handling RPC; command ".$r->getName());

From c08cd584439a9a82689b459394312b85a527d9d6 Mon Sep 17 00:00:00 2001
From: David Anderson <davea@ssl.berkeley.edu>
Date: Sat, 21 Dec 2024 14:55:49 -0800
Subject: [PATCH 2/2] fix XSS

---
 html/user/job_file.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/html/user/job_file.php b/html/user/job_file.php
index 0b7ea9ba696..5fb161ef16d 100644
--- a/html/user/job_file.php
+++ b/html/user/job_file.php
@@ -299,7 +299,7 @@ function upload_files($r) {
 $req = $_POST['request'];
 $r = simplexml_load_string($req);
 if (!$r) {
-    xml_error(-1, "can't parse request message: $req", __FILE__, __LINE__);
+    xml_error(-1, "can't parse request message: ".htmlspecialchars($req), __FILE__, __LINE__);
 }
 
 switch($r->getName()) {