Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
squirrelmail quick fix for file disclosure vuln presented at Troopers 2018 (#TR18)
--- 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)) {
@jult

This comment has been minimized.

Copy link

jult commented Mar 15, 2018

Why do they not post alll of squirrelmail's code on github and let people fork it, so others can maintain SquirrelMail the way it should, by communicating..
Sure @dertuxmalwieder, I know github is not the only one, but you just can't deny the facts that it is by far the easiest one to use, and has a much larger user-base. Furthermore, my main complaint on the SM project as it stands is the total lack of communication. Looking at their website makes you think it's dead, while it isn't.

@dertuxmalwieder

This comment has been minimized.

Copy link

dertuxmalwieder commented Mar 15, 2018

Github is not the only VCS platform which allows forks and contributions. There is rarely a good reason to change your VCS every few years just because a different one is "the thing" now.

@leitmedium

This comment has been minimized.

Copy link

leitmedium commented Mar 15, 2018

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

@My1

This comment has been minimized.

Copy link

My1 commented Mar 15, 2018

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

or cant it just say "skip this file"?

@BernhardPosselt

This comment has been minimized.

Copy link

BernhardPosselt commented Mar 15, 2018

ctype_alnum is the wrong thing to use here

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

@ooxi

This comment has been minimized.

Copy link

ooxi commented Mar 15, 2018

@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.

@mnaef

This comment has been minimized.

Copy link

mnaef commented Mar 15, 2018

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

@dagbrown

This comment has been minimized.

Copy link

dagbrown commented Mar 16, 2018

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.

@jult

This comment has been minimized.

Copy link

jult commented Apr 7, 2018

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/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.