Security: Fix XSS attack against new comment form

WARNING: If you're one of the first people testing this commit, please
use a backup database.

How to reproduce: Create a new comment, and set all fields to
<script>alert("Pwned")</script>.  Submit it.  You will see a JavaScript
alert dialog, which is bad.

What's happening: Untrusted fields in Comment objects are sanitized
immediately before they're written to the database for the first time.
But if validation fails, it leaves the application with an unsanitized
comment object.  When the "can't submit comment" error is displayed,
this unsanitized comment object can be passed straight throught to
Liquid, which assumes that all HTML tags have been escaped.

(This may look like "self XSS" attack only, but hostile pages can
trigger it by tricking you into submitting a comment form back to your
own site, preloaded with malicious data.)

How we fix it: We make HTML escaping the responsibility of CommentDrop,
not the Comment model.  This means that we need to unescape several
existing fields in the database.

Possible issues: This means that we're storing dangerous, untrusted data
in our database, and that we need to rely on the proper use of 'h' and
'CGI.escapeHTML'.  In the case of 'h', we're already using SafeERB, so
insecure admin templates will be caught automatically, and dangerous
data should never be sent to the user.  In the case of Liquid, we need
to carefully examine our CommentDrop class to make sure that we're not
passing any unescaped data through to the Liquid templates.  But this is
a pretty manageable "proof obligation"--and remember that the old
"sanitize on create" code actually suffered from XSS attacks, because it
was too easy to do the sanitization in the wrong place.

Author: emk

app/drops/comment_drop.rb

@@ -2,16 +2,25 @@ class CommentDrop < BaseDrop
   include Mephisto::Liquid::UrlMethods
 
   timezone_dates :published_at, :created_at
-  liquid_attributes.push(*[:author, :author_email, :author_ip, :title])
+  liquid_attributes.push(:title) # Not sure who uses this.
 
   def initialize(source)
     super
-    @liquid.update 'is_approved' => @source.approved?, 'body' => ActionView::Base.white_list_sanitizer.sanitize(@source.body_html)
+    @liquid.update('is_approved' => @source.approved?,
+                   'body' => ActionView::Base.white_list_sanitizer.sanitize(@source.body_html))
+
+    # We used to escape these fields when we saved them to the database.
+    # Now we've unescaped everything in the database, but we still need to
+    # preserve backwards compatibility with old themes, which expect these
+    # values to be escaped.  So we escape these fields manually here.
+    [:author, :author_email, :author_ip].each do |a|
+      @liquid.update(a.to_s => CGI.escapeHTML(@source.send(a) || ''))
+    end
   end
-  
+
   def author_url
     return nil if source.author_url.blank?
-    @source.author_url =~ /^https?:\/\// ? @source.author_url : "http://" + @source.author_url
+    CGI.escapeHTML(@source.author_url =~ /^https?:\/\// ? @source.author_url : "http://" + @source.author_url)
   end
 
   def url
@@ -23,7 +32,7 @@ def new_record
   end
 
   def author_link
-    @source.author_url.blank? ? "<span>#{@source.author}</span>" : %Q{<a href="#{author_url}">#{@source.author}</a>}
+    @source.author_url.blank? ? "<span>#{@liquid['author']}</span>" : %Q{<a href="#{author_url}">#{@liquid['author']}</a>}
   end
 
   def presentation_class

app/models/comment.rb

@@ -7,7 +7,6 @@ class Comment < Content
   before_validation :clean_up_author_url
   after_validation_on_create  :snag_article_attributes
   before_create  :check_comment_expiration
-  before_create  :sanitize_attributes
   before_save    :update_counter_cache
   before_destroy :decrement_counter_cache
   belongs_to :article
@@ -78,12 +77,6 @@ def mark_as_ham(site, request)
   end
 
   protected
-    def sanitize_attributes
-      [:author, :author_url, :author_email, :author_ip, :user_agent, :referrer].each do |a|
-        self.send("#{a}=", CGI::escapeHTML(self.send(a).to_s))
-      end
-    end
-
     def snag_article_attributes
       self.filter ||= article.site.filter
       [:site, :title, :published_at, :permalink].each { |a| self.send("#{a}=", article.send(a)) }

db/migrate/20081219130711_unescape_comment_fields.rb

@@ -0,0 +1,35 @@
+# We were storing these fields in the database "pre-escaped", which (oddly
+# enough) actually increased the number of security problems in our
+# application, because we didn't escape the fields until after the record
+# was validated, so error pages tended to vulnerable to XSS attacks.  So
+# let's just rely on SafeERB and our CommentDrop to make sure we escape on
+# output.
+class UnescapeCommentFields < ActiveRecord::Migration
+  class Content < ActiveRecord::Base
+  end
+
+  class Comment < Content
+  end
+
+  # Taken from the old sanitize_attributes method in Content.
+  ATTRIBUTES =
+    [:author, :author_url, :author_email, :author_ip, :user_agent, :referrer]
+
+  def self.up
+    Comment.find(:all).each do |c|
+      ATTRIBUTES.each do |a|
+        c.send("#{a}=", CGI::unescapeHTML(c.send(a).to_s)) if c.send(a)
+      end
+      c.save!
+    end
+  end
+
+  def self.down
+    Comment.find(:all).each do |c|
+      ATTRIBUTES.each do |a|
+        c.send("#{a}=", CGI::escapeHTML(c.send(a).to_s)) if c.send(a)
+      end
+      c.save!
+    end
+  end
+end

db/schema.rb

@@ -1,5 +1,5 @@
 # This file is auto-generated from the current state of the database. Instead of editing this file, 
-# please use the migrations feature of ActiveRecord to incrementally modify your database, and
+# please use the migrations feature of Active Record to incrementally modify your database, and
 # then regenerate this schema definition.
 #
 # Note that this schema.rb definition is the authoritative source for your database schema. If you need
@@ -9,7 +9,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 76) do
+ActiveRecord::Schema.define(:version => 20081219130711) do
 
   create_table "assets", :force => true do |t|
     t.string   "content_type"
@@ -110,8 +110,8 @@
     t.integer  "assets_count",                  :default => 0
   end
 
-  add_index "contents", ["published_at"], :name => "idx_articles_published"
   add_index "contents", ["article_id", "approved", "type"], :name => "idx_comments"
+  add_index "contents", ["published_at"], :name => "idx_articles_published"
 
   create_table "events", :force => true do |t|
     t.string   "mode"

test/unit/comment_drop_test.rb

@@ -5,7 +5,7 @@ class CommentDropTest < Test::Unit::TestCase
 
   def setup
     @comment = contents(:welcome_comment).to_liquid
-    @mock_comment = [:published_at, :created_at, :author, :author_email, :author_ip, :title, :approved?].inject({:body_html => 'foo'}) { |h, i| h.update i => true }
+    @mock_comment = [:published_at, :created_at, :title, :approved?].inject({:body_html => 'foo', :author => 'Bob', :author_email => 'bob@example.com', :author_ip => '127.0.0.1' }) { |h, i| h.update i => true }
   end
 
   def test_should_convert_comment_to_drop
@@ -36,8 +36,7 @@ def test_should_return_correct_author_link
     assert_equal %Q{<a href="https://abc">rico</a>}, @comment.author_link
     @comment.source.author     = '<strong>rico</strong>'
     @comment.source.author_url = '<strong>https://abc</strong>'
-    @comment.source.send(:sanitize_attributes)
-    assert_equal %Q{<a href="http://&lt;strong&gt;https://abc&lt;/strong&gt;">&lt;strong&gt;rico&lt;/strong&gt;</a>}, @comment.author_link
+    assert_equal %Q{<a href="http://&lt;strong&gt;https://abc&lt;/strong&gt;">&lt;strong&gt;rico&lt;/strong&gt;</a>}, @comment.source.to_liquid.author_link
   end
 
   def test_should_show_filtered_text
@@ -74,4 +73,4 @@ def create_comment_stub(options)
         def stub.id() 55; end
       end
     end
-end
+end