sup

commit 2caccbb02f63f1da56a6e7562903ac449c6367bf
parent e791c9e33e510d98bea0c1b505d95c469a8d369c
Author: Matthieu Rakotojaona <matthieu.rakotojaona@gmail.com>
Date:   Wed, 14 Jan 2015 21:32:22 +0100

Escape attachments filenames so they can be safely saved

Diffstat:

3 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/lib/sup/message_chunks.rb b/lib/sup/message_chunks.rb
@@ -159,6 +159,7 @@ EOS
         "Attachment: #{filename} (#{content_type}; #{@raw_content.size.to_human_size})"
       end
     end
+    def safe_filename; Shellwords.escape(@filename).gsub("/", "_") end
 
     ## an attachment is exapndable if we've managed to decode it into
     ## something we can display inline. otherwise, it's viewable.
diff --git a/lib/sup/modes/thread_view_mode.rb b/lib/sup/modes/thread_view_mode.rb
@@ -388,7 +388,7 @@ EOS
     when Chunk::Attachment
       default_dir = $config[:default_attachment_save_dir]
       default_dir = ENV["HOME"] if default_dir.nil? || default_dir.empty?
-      default_fn = File.expand_path File.join(default_dir, chunk.filename)
+      default_fn = File.expand_path File.join(default_dir, chunk.safe_filename)
       fn = BufferManager.ask_for_filename :filename, "Save attachment to file or directory: ", default_fn, true
 
       # if user selects directory use file name from message
@@ -417,7 +417,7 @@ EOS
     num_errors = 0
     m.chunks.each do |chunk|
       next unless chunk.is_a?(Chunk::Attachment)
-      fn = File.join(folder, chunk.filename)
+      fn = File.join(folder, chunk.safe_filename)
       num_errors += 1 unless save_to_file(fn, false) { |f| f.print chunk.raw_content }
       num += 1
     end
diff --git a/test/test_message.rb b/test/test_message.rb
@@ -520,6 +520,82 @@ EOS
 
   end
 
+  def test_malicious_attachment_names
+
+
+    message = <<EOS
+From: Matthieu Rakotojaona <matthieu.rakotojaona@gmail.com>
+To: reply+0007a7cb7174d1d188fcd420fce83e0f68fe03fc7416cdae92cf0000000110ce4efd92a169ce033d18e1 <reply+0007a7cb7174d1d188fcd420fce83e0f68fe03fc7416cdae92cf0000000110ce4efd92a169ce033d18e1@reply.github.com>
+Subject: Re: [sup] Attachment saving and special characters in filenames (#378)
+In-reply-to: <sup-heliotrope/sup/issues/378@github.com>
+References: <sup-heliotrope/sup/issues/378@github.com>
+X-pgp-key: http://otokar.looc2011.eu/static/matthieu.rakotojaona.asc
+Date: Wed, 14 Jan 2015 22:13:37 +0100
+Message-Id: <1421269972-sup-5245@kpad>
+User-Agent: Sup/git
+Content-Transfer-Encoding: 8bit
+MIME-Version: 1.0
+Content-Type: multipart/mixed; boundary="=-1421270017-526778-1064-1628-1-="
+
+
+--=-1421270017-526778-1064-1628-1-=
+Content-Type: text/plain; charset=UTF-8
+Content-Disposition: inline
+
+Excerpts from Felix Kaiser's message of 2015-01-14 16:36:29 +0100:
+> When saving attachments, sup should replace special characters when suggesting a filename to save the attachment to.
+> 
+> I just got an attachment with a name like "foo/2.pdf". sup suggests saving it to /home/fxkr/foo/2.pdf (and fails to save it, of course, if /home/fxkr/foo isn't a directory).
+> 
+> I haven't tested the "Save All" feature, but I hope nothing bad happens when there's an attachment called "../../../../../../../home/fxkr/.bashrc" ;-)
+> 
+> ---
+> Reply to this email directly or view it on GitHub:
+> https://github.com/sup-heliotrope/sup/issues/378
+
+For tests, here's an email with an attachment filename set to
+sup/.travis.yml (really, this time)
+
+-- 
+Matthieu Rakotojaona
+
+--=-1421270017-526778-1064-1628-1-=
+Content-Disposition: attachment; filename="sup/.travis.yml"
+Content-Type: text/x-yaml; name="sup/.travis.yml"
+Content-Transfer-Encoding: 8bit
+
+language: ruby
+
+rvm:
+  - 2.1.1
+  - 2.0.0
+  - 1.9.3
+
+before_install:
+  - sudo apt-get update -qq
+  - sudo apt-get install -qq uuid-dev uuid libncursesw5-dev libncursesw5 gnupg2 pandoc
+  - git submodule update --init --recursive
+
+script: bundle exec rake travis
+
+--=-1421270017-526778-1064-1628-1-=--
+EOS
+
+    source = DummySource.new("sup-test://test_blank_header_lines")
+    source.messages = [ message ]
+    source_info = 0
+
+    sup_message = Message.build_from_source(source, source_info)
+    chunks = sup_message.load_from_source!
+
+    # See if attachment filenames can be safely used for saving.
+    # We do that by verifying that any folder-related character (/ or \)
+    # are not interpreted: the filename must not be interpreted into a
+    # path.
+    fn = chunks[3].safe_filename
+    assert_equal(fn, File.basename(fn))
+
+  end
   # TODO: test different error cases, malformed messages etc.
 
   # TODO: test different quoting styles, see that they are all divided