squirrelmail quick fix for file disclosure vuln presented at Troopers 2018 (#TR18)

squirrelmail-fix-file-disclosure.diff

--- squirrelmail.stable/squirrelmail/class/deliver/Deliver.class.php	2017-01-27 21:31:33.000000000 +0100
+++ htdocs/class/deliver/Deliver.class.php	2018-03-14 17:21:10.320000000 +0100
@@ -281,6 +281,7 @@
                 global $username, $attachment_dir;
                 $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
                 $filename = $message->att_local_name;
+		if(!ctype_alnum($filename)) die();
 
                 // inspect attached file for lines longer than allowed by RFC,
                 // in which case we'll be using base64 encoding (so we can split
@@ -339,6 +340,7 @@
                 global $username, $attachment_dir;
                 $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
                 $filename = $message->att_local_name;
+		if(!ctype_alnum($filename)) die();
                 $file = fopen ($hashed_attachment_dir . '/' . $filename, 'rb');
                 
                 while ($tmp = fread($file, 570)) {

Thanks! Just hot patched a server with hundreds of users…

@hannob
is it really a good idea to go straight to die()?

or cant it just say "skip this file"?

ctype_alnum is the wrong thing to use here

That code will also prevent you from having files called something.tar.gz

@BernhardPosselt are you sure? As far as I understand, $message->att_local_name contains a temporary file name generated by squirrelmail, not the actual name of the attachment.

I went for

--- class/deliver/Deliver.class.php.orig        2018-03-15 18:08:34.649035050 +0100
+++ class/deliver/Deliver.class.php     2018-03-15 18:33:44.638918479 +0100
@@ -293,7 +293,14 @@
                 $file_has_long_lines = file_has_long_lines($hashed_attachment_dir
                                                            . '/' . $filename, 990);
 
-                $file = fopen ($hashed_attachment_dir . '/' . $filename, 'rb');
+               /*
+               Quickndirty Mitigate https://www.golem.de/news/webmailer-squirrelmail-sicherheitsluecke-bleibt-vorerst-ungefixt-1803-133344.html
+               */
+                if (strpos(realpath($hashed_attachment_dir . '/' . $filename),$hashed_attachment_dir) !== 0 ) die();
+
+               $file = fopen ($hashed_attachment_dir . '/' . $filename, 'rb');
 
                 // long lines were found, need to use base64 encoding
                 //
@@ -339,6 +346,12 @@
                 global $username, $attachment_dir;
                 $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
                 $filename = $message->att_local_name;
+               /*
+               Quickndirty Mitigate https://www.golem.de/news/webmailer-squirrelmail-sicherheitsluecke-bleibt-vorerst-ungefixt-1803-133344.html
+               */
+                if (strpos(realpath($hashed_attachment_dir . '/' . $filename),$hashed_attachment_dir) !== 0 ) die();
                 $file = fopen ($hashed_attachment_dir . '/' . $filename, 'rb');
                 
                 while ($tmp = fread($file, 570)) {

as a quick & dirty fix

~cal

Crashing at the first sign of trouble seems suboptimal.

Here's what I did to fix it:

diff -r -C 3 squirrelmail-webmail-1.4.22.orig/class/deliver/Deliver.class.php squirrelmail-webmail-1.4.22/class/deliver/Deliver.class.php
*** squirrelmail-webmail-1.4.22.orig/class/deliver/Deliver.class.php	Fri Mar 16 14:45:18 2018
--- squirrelmail-webmail-1.4.22/class/deliver/Deliver.class.php	Fri Mar 16 14:46:56 2018
***************
*** 280,286 ****
              } elseif ($message->att_local_name) {
                  global $username, $attachment_dir;
                  $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!                 $filename = $message->att_local_name;
  
                  // inspect attached file for lines longer than allowed by RFC,
                  // in which case we'll be using base64 encoding (so we can split
--- 280,286 ----
              } elseif ($message->att_local_name) {
                  global $username, $attachment_dir;
                  $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!                 $filename = base64_encode($message->att_local_name);
  
                  // inspect attached file for lines longer than allowed by RFC,
                  // in which case we'll be using base64 encoding (so we can split
***************
*** 338,344 ****
              } elseif ($message->att_local_name) {
                  global $username, $attachment_dir;
                  $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!                 $filename = $message->att_local_name;
                  $file = fopen ($hashed_attachment_dir . '/' . $filename, 'rb');
                  
                  while ($tmp = fread($file, 570)) {
--- 338,344 ----
              } elseif ($message->att_local_name) {
                  global $username, $attachment_dir;
                  $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!                 $filename = base64_encode($message->att_local_name);
                  $file = fopen ($hashed_attachment_dir . '/' . $filename, 'rb');
                  
                  while ($tmp = fread($file, 570)) {
***************
*** 502,508 ****
              if (!empty($message->att_local_name)) { // is this redundant? I have no idea
                  global $username, $attachment_dir;
                  $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!                 $filename = $hashed_attachment_dir . '/' . $message->att_local_name;
  
                  // using 990 because someone somewhere is folding lines at
                  // 990 instead of 998 and I'm too lazy to find who it is
--- 502,508 ----
              if (!empty($message->att_local_name)) { // is this redundant? I have no idea
                  global $username, $attachment_dir;
                  $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!                 $filename = $hashed_attachment_dir . '/' . base64_encode($message->att_local_name);
  
                  // using 990 because someone somewhere is folding lines at
                  // 990 instead of 998 and I'm too lazy to find who it is
diff -r -C 3 squirrelmail-webmail-1.4.22.orig/class/mime/Message.class.php squirrelmail-webmail-1.4.22/class/mime/Message.class.php
*** squirrelmail-webmail-1.4.22.orig/class/mime/Message.class.php	Fri Mar 16 14:45:18 2018
--- squirrelmail-webmail-1.4.22/class/mime/Message.class.php	Fri Mar 16 14:48:20 2018
***************
*** 1114,1121 ****
          if ($this->att_local_name) {
              global $username, $attachment_dir;
              $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!             if ( file_exists($hashed_attachment_dir . '/' . $this->att_local_name) ) {
!                 unlink($hashed_attachment_dir . '/' . $this->att_local_name);
              }
          }
          // recursively delete attachments from entities contained in this object
--- 1114,1121 ----
          if ($this->att_local_name) {
              global $username, $attachment_dir;
              $hashed_attachment_dir = getHashedDir($username, $attachment_dir);
!             if ( file_exists($hashed_attachment_dir . '/' . base64_encode($this->att_local_name) ) ) {
!                 unlink($hashed_attachment_dir . '/' . base64_encode($this->att_local_name) );
              }
          }
          // recursively delete attachments from entities contained in this object

You can't have problems with slashes in filenames if there were never slashes in any real filenames to begin with.

The vulnerability [CVE-2018-8741] has been patched in SM Stable's compose.php component, for those wanting to grab and replace their own;
https://github.com/jult/SquirrelMail/tree/master/src/