Skip to content
Permalink
Browse files Browse the repository at this point in the history
security: shellwords escape attachment file names to prevent remote c…
…ode injection

Merge peristent temp files and fix quotation of shellwords escaped string

s|returnded|returned|

Conflicts:
	doc/Hooks.txt
	lib/sup/message_chunks.rb
  • Loading branch information
gauteh committed Oct 28, 2013
1 parent 7eaf96e commit 8b46cdb
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 4 deletions.
3 changes: 2 additions & 1 deletion doc/Hooks.txt
Expand Up @@ -50,10 +50,11 @@ before-poll:
mime-decode:
## turn text/html attachments into plain text, unless they are part
## of a multipart/alternative pair
require 'shellwords'
unless sibling_types.member? "text/plain"
case content_type
when "text/html"
`/usr/bin/w3m -dump -T #{content_type} '#{filename}'`
`/usr/bin/w3m -dump -T #{content_type} #{Shellwords.escape filename}`
end
end

Expand Down
21 changes: 18 additions & 3 deletions lib/sup/message_chunks.rb
Expand Up @@ -60,6 +60,8 @@ def make_tmpname(prefix_suffix, n)
module Redwood
module Chunk
class Attachment
## please see note in write_to_disk on important usage
## of quotes to avoid remote command injection.
HookManager.register "mime-decode", <<EOS
Decodes a MIME attachment into text form. The text will be displayed
directly in Sup. For attachments that you wish to use a separate program
Expand All @@ -76,6 +78,9 @@ class Attachment
The decoded text of the attachment, or nil if not decoded.
EOS


## please see note in write_to_disk on important usage
## of quotes to avoid remote command injection.
HookManager.register "mime-view", <<EOS
Views a non-text MIME attachment. This hook allows you to run
third-party programs for attachments that require such a thing (e.g.
Expand Down Expand Up @@ -122,6 +127,8 @@ def initialize content_type, filename, encoded_content, sibling_types
when /^text\/plain\b/
@raw_content
else
## please see note in write_to_disk on important usage
## of quotes to avoid remote command injection.
HookManager.run "mime-decode", :content_type => content_type,
:filename => lambda { write_to_disk },
:charset => encoded_content.charset,
Expand Down Expand Up @@ -153,18 +160,22 @@ def expandable?; !viewable? end
def initial_state; :open end
def viewable?; @lines.nil? end
def view_default! path
## please see note in write_to_disk on important usage
## of quotes to avoid remote command injection.
case RbConfig::CONFIG['arch']
when /darwin/
cmd = "open '#{path}'"
cmd = "open #{path}"
else
cmd = "/usr/bin/run-mailcap --action=view '#{@content_type}:#{path}'"
cmd = "/usr/bin/run-mailcap --action=view #{@content_type}:#{path}"
end
debug "running: #{cmd.inspect}"
BufferManager.shell_out(cmd)
$? == 0
end

def view!
## please see note in write_to_disk on important usage
## of quotes to avoid remote command injection.
write_to_disk do |file|

@@view_tempfiles.push file # make sure the tempfile is not garbage collected before sup stops
Expand All @@ -175,6 +186,10 @@ def view!
end
end

## note that the path returned from write_to_disk is
## Shellwords.escaped and is intended to be used without single
## or double quotes. the use of either opens sup up for remote
## code injection in the file name.
def write_to_disk
begin
file = Tempfile.new(["sup", Shellwords.escape(@filename.gsub("/", "_")) || "sup-attachment"])
Expand Down Expand Up @@ -243,7 +258,7 @@ def color; :sig_color end
class EnclosedMessage
attr_reader :lines
def initialize from, to, cc, date, subj
@from = from ? "unknown sender" : from.full_adress
@from = from ? "unknown sender" : from.full_address
@to = to ? "" : to.map { |p| p.full_address }.join(", ")
@cc = cc ? "" : cc.map { |p| p.full_address }.join(", ")
if date
Expand Down

0 comments on commit 8b46cdb

Please sign in to comment.