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)) {

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/