New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XSS Vulnerability #3948

Closed
ColdHeat opened this Issue Feb 1, 2013 · 12 comments

Comments

Projects
None yet
7 participants
@ColdHeat

ColdHeat commented Feb 1, 2013

Diaspora is vulnerable to an XSS attack through the name field of a user's profile page. Names are rendered into the page un-encoded in the name field of the Mentions.options.prefillMention variable on a user's public profile page.

<script> 
//<![CDATA[
Mentions.options.prefillMention = Mentions._contactToMention({
    "id": 127044,
    "guid": "ad6c6e92c1c71d9e",
    "name": "</script><script> alert(0)</script>",
    "avatar": "/assets/user/default.png",
    "handle": "superduper@diasp.org",
    "url": "/people/ad6c6e92c1c71d9e"
});
//]]>
 </script>

This executes because the </script> tag closes out the original script and then
begins it's own to allow the alert(0) to trigger.

This is done by setting the a user's first name to:

</script><script>

and setting the last name to:

alert(0)</script>

With an SSL webserver (gist.github would work) to host our script and a URL shortener (goo.gl would work) we can
run more more arbitrary JavaScript.

In addition to the profile page, the alert would trigger on auto-completed searches done by the search bar on the top of a user's session.

For example, if a user is searching around and a user whose name is </script><script> alert(0)</script> shows up, the script would be rendered un-encoded into the DOM resulting in execution. So searching for the username of a user who already has this set as their name would have been hit with the XSS.

@desyncr

This comment has been minimized.

Show comment
Hide comment
@desyncr

desyncr Feb 1, 2013

Contributor

Reproed non-persistent XSS vuln on search auto-complete and persistent XSS on profile name.

Contributor

desyncr commented Feb 1, 2013

Reproed non-persistent XSS vuln on search auto-complete and persistent XSS on profile name.

@desyncr

This comment has been minimized.

Show comment
Hide comment
@desyncr

desyncr Feb 1, 2013

Contributor

Updating validates_format_of could fix the persistent XSS.

  validates_format_of :first_name, :with => /\A[^;]+\z/, :allow_blank => true
  validates_format_of :last_name, :with => /\A[^;]+\z/, :allow_blank => true
\A[\w\s]{0,35}\z
Contributor

desyncr commented Feb 1, 2013

Updating validates_format_of could fix the persistent XSS.

  validates_format_of :first_name, :with => /\A[^;]+\z/, :allow_blank => true
  validates_format_of :last_name, :with => /\A[^;]+\z/, :allow_blank => true
\A[\w\s]{0,35}\z
@jhass

This comment has been minimized.

Show comment
Hide comment
@jhass

jhass Feb 1, 2013

Member

Hmm, I wasn't able to pass the existing validation already and could only reproduce with update_attribute. Anyway, my proposed patch would be:

diff --git a/app/helpers/layout_helper.rb b/app/helpers/layout_helper.rb
index c226aff..d29b124 100644
--- a/app/helpers/layout_helper.rb
+++ b/app/helpers/layout_helper.rb
@@ -43,7 +43,7 @@ module LayoutHelper
     user = UserPresenter.new(current_user, a_ids).to_json
     content_tag(:script) do
       <<-JS.html_safe
-        window.current_user_attributes = #{user}
+        window.current_user_attributes = #{j user}
       JS
     end
   end
diff --git a/app/views/people/show.html.haml b/app/views/people/show.html.haml
index 9dd05ad..a2b18ca 100644
--- a/app/views/people/show.html.haml
+++ b/app/views/people/show.html.haml
@@ -7,7 +7,7 @@
   = javascript_include_tag :people
   - if user_signed_in? && @person != current_user.person
     :javascript
-      Mentions.options.prefillMention = Mentions._contactToMention(#{@person.to_json});
+      Mentions.options.prefillMention = Mentions._contactToMention(#{j @person.to_json});

 - content_for :page_title do
   = @person.name
diff --git a/config/initializers/json_escape.rb b/config/initializers/json_escape.rb
new file mode 100644
index 0000000..a27bf2d
--- /dev/null
+++ b/config/initializers/json_escape.rb
@@ -0,0 +1,11 @@
+# From http://jfire.io/blog/2012/04/30/how-to-securely-bootstrap-json-in-a-rails-view/
+# Review on Rails 4 update, might be built in by then!
+
+class ActionView::Base
+  def json_escape(s)
+    result = s.to_s.gsub('/', '\/')
+    s.html_safe? ? result.html_safe : result
+  end
+
+  alias j json_escape
+end
diff --git a/spec/controllers/people_controller_spec.rb b/spec/controllers/people_controller_spec.rb
index 2225090..c38ed51 100644
--- a/spec/controllers/people_controller_spec.rb
+++ b/spec/controllers/people_controller_spec.rb
@@ -201,11 +201,10 @@ describe PeopleController do
     it 'does not allow xss attacks' do
       user2 = bob
       profile = user2.profile
-      profile.first_name = "<script> alert('xss attack');</script>"
-      profile.save
+      profile.update_attribute(:first_name, "</script><script> alert('xss attack');</script>")
       get :show, :id => user2.person.to_param
       response.should be_success
-      response.body.match(profile.first_name).should be_false
+      response.body.should_not include(profile.first_name)
     end


diff --git a/spec/helpers/layout_helper_spec.rb b/spec/helpers/layout_helper_spec.rb
index f63da63..dd11e36 100644
--- a/spec/helpers/layout_helper_spec.rb
+++ b/spec/helpers/layout_helper_spec.rb
@@ -5,6 +5,18 @@
 require 'spec_helper'

 describe LayoutHelper do
+  describe "#set_current_user_in_javascript" do
+    it "doesn't allow xss" do
+      user = FactoryGirl.create :user
+      profile = user.profile
+      profile.update_attribute(:first_name, "</script><script>alert(0);</script>");
+      stub!(:user_signed_in?).and_return true
+      stub!(:current_user).and_return user
+      set_current_user_in_javascript.should_not be_empty
+      set_current_user_in_javascript.should_not include(profile.first_name)
+    end
+  end
+
   describe "#page_title" do
     context "passed blank text" do
       it "returns Diaspora*" do

Did I miss any instance? @asphxia could you provide steps for the auto complete XSS?

Member

jhass commented Feb 1, 2013

Hmm, I wasn't able to pass the existing validation already and could only reproduce with update_attribute. Anyway, my proposed patch would be:

diff --git a/app/helpers/layout_helper.rb b/app/helpers/layout_helper.rb
index c226aff..d29b124 100644
--- a/app/helpers/layout_helper.rb
+++ b/app/helpers/layout_helper.rb
@@ -43,7 +43,7 @@ module LayoutHelper
     user = UserPresenter.new(current_user, a_ids).to_json
     content_tag(:script) do
       <<-JS.html_safe
-        window.current_user_attributes = #{user}
+        window.current_user_attributes = #{j user}
       JS
     end
   end
diff --git a/app/views/people/show.html.haml b/app/views/people/show.html.haml
index 9dd05ad..a2b18ca 100644
--- a/app/views/people/show.html.haml
+++ b/app/views/people/show.html.haml
@@ -7,7 +7,7 @@
   = javascript_include_tag :people
   - if user_signed_in? && @person != current_user.person
     :javascript
-      Mentions.options.prefillMention = Mentions._contactToMention(#{@person.to_json});
+      Mentions.options.prefillMention = Mentions._contactToMention(#{j @person.to_json});

 - content_for :page_title do
   = @person.name
diff --git a/config/initializers/json_escape.rb b/config/initializers/json_escape.rb
new file mode 100644
index 0000000..a27bf2d
--- /dev/null
+++ b/config/initializers/json_escape.rb
@@ -0,0 +1,11 @@
+# From http://jfire.io/blog/2012/04/30/how-to-securely-bootstrap-json-in-a-rails-view/
+# Review on Rails 4 update, might be built in by then!
+
+class ActionView::Base
+  def json_escape(s)
+    result = s.to_s.gsub('/', '\/')
+    s.html_safe? ? result.html_safe : result
+  end
+
+  alias j json_escape
+end
diff --git a/spec/controllers/people_controller_spec.rb b/spec/controllers/people_controller_spec.rb
index 2225090..c38ed51 100644
--- a/spec/controllers/people_controller_spec.rb
+++ b/spec/controllers/people_controller_spec.rb
@@ -201,11 +201,10 @@ describe PeopleController do
     it 'does not allow xss attacks' do
       user2 = bob
       profile = user2.profile
-      profile.first_name = "<script> alert('xss attack');</script>"
-      profile.save
+      profile.update_attribute(:first_name, "</script><script> alert('xss attack');</script>")
       get :show, :id => user2.person.to_param
       response.should be_success
-      response.body.match(profile.first_name).should be_false
+      response.body.should_not include(profile.first_name)
     end


diff --git a/spec/helpers/layout_helper_spec.rb b/spec/helpers/layout_helper_spec.rb
index f63da63..dd11e36 100644
--- a/spec/helpers/layout_helper_spec.rb
+++ b/spec/helpers/layout_helper_spec.rb
@@ -5,6 +5,18 @@
 require 'spec_helper'

 describe LayoutHelper do
+  describe "#set_current_user_in_javascript" do
+    it "doesn't allow xss" do
+      user = FactoryGirl.create :user
+      profile = user.profile
+      profile.update_attribute(:first_name, "</script><script>alert(0);</script>");
+      stub!(:user_signed_in?).and_return true
+      stub!(:current_user).and_return user
+      set_current_user_in_javascript.should_not be_empty
+      set_current_user_in_javascript.should_not include(profile.first_name)
+    end
+  end
+
   describe "#page_title" do
     context "passed blank text" do
       it "returns Diaspora*" do

Did I miss any instance? @asphxia could you provide steps for the auto complete XSS?

@desyncr

This comment has been minimized.

Show comment
Hide comment
@desyncr

desyncr Feb 1, 2013

Contributor

The XSS is executed when the auto-complete dropdown is displayed.

Untitled

Contributor

desyncr commented Feb 1, 2013

The XSS is executed when the auto-complete dropdown is displayed.

Untitled

@jhass

This comment has been minimized.

Show comment
Hide comment
@jhass

jhass Feb 1, 2013

Member

Next iteration of the patch, please review:

diff --git a/app/assets/javascripts/widgets/search.js b/app/assets/javascripts/widgets/search.js
index 4648cdf..675b6f8 100644
--- a/app/assets/javascripts/widgets/search.js
+++ b/app/assets/javascripts/widgets/search.js
@@ -36,11 +36,12 @@
     };

     this.formatResult = function(row) {
-      return row.name;
+      return Handlebars.Utils.escapeExpression(row.name);
     };

     this.parse = function(data) {
       var results =  data.map(function(person){
+        person['name'] = Handlebars.Utils.escapeExpression(person['name']);
         return {data : person, value : person['name']}
       });

diff --git a/app/helpers/layout_helper.rb b/app/helpers/layout_helper.rb
index c226aff..d29b124 100644
--- a/app/helpers/layout_helper.rb
+++ b/app/helpers/layout_helper.rb
@@ -43,7 +43,7 @@ module LayoutHelper
     user = UserPresenter.new(current_user, a_ids).to_json
     content_tag(:script) do
       <<-JS.html_safe
-        window.current_user_attributes = #{user}
+        window.current_user_attributes = #{j user}
       JS
     end
   end
diff --git a/app/views/people/show.html.haml b/app/views/people/show.html.haml
index 9dd05ad..a2b18ca 100644
--- a/app/views/people/show.html.haml
+++ b/app/views/people/show.html.haml
@@ -7,7 +7,7 @@
   = javascript_include_tag :people
   - if user_signed_in? && @person != current_user.person
     :javascript
-      Mentions.options.prefillMention = Mentions._contactToMention(#{@person.to_json});
+      Mentions.options.prefillMention = Mentions._contactToMention(#{j @person.to_json});

 - content_for :page_title do
   = @person.name
diff --git a/config/initializers/json_escape.rb b/config/initializers/json_escape.rb
new file mode 100644
index 0000000..a27bf2d
--- /dev/null
+++ b/config/initializers/json_escape.rb
@@ -0,0 +1,11 @@
+# From http://jfire.io/blog/2012/04/30/how-to-securely-bootstrap-json-in-a-rails-view/
+# Review on Rails 4 update, might be built in by then!
+
+class ActionView::Base
+  def json_escape(s)
+    result = s.to_s.gsub('/', '\/')
+    s.html_safe? ? result.html_safe : result
+  end
+
+  alias j json_escape
+end
diff --git a/spec/controllers/people_controller_spec.rb b/spec/controllers/people_controller_spec.rb
index 2225090..c38ed51 100644
--- a/spec/controllers/people_controller_spec.rb
+++ b/spec/controllers/people_controller_spec.rb
@@ -201,11 +201,10 @@ describe PeopleController do
     it 'does not allow xss attacks' do
       user2 = bob
       profile = user2.profile
-      profile.first_name = "<script> alert('xss attack');</script>"
-      profile.save
+      profile.update_attribute(:first_name, "</script><script> alert('xss attack');</script>")
       get :show, :id => user2.person.to_param
       response.should be_success
-      response.body.match(profile.first_name).should be_false
+      response.body.should_not include(profile.first_name)
     end


diff --git a/spec/helpers/layout_helper_spec.rb b/spec/helpers/layout_helper_spec.rb
index f63da63..dd11e36 100644
--- a/spec/helpers/layout_helper_spec.rb
+++ b/spec/helpers/layout_helper_spec.rb
@@ -5,6 +5,18 @@
 require 'spec_helper'

 describe LayoutHelper do
+  describe "#set_current_user_in_javascript" do
+    it "doesn't allow xss" do
+      user = FactoryGirl.create :user
+      profile = user.profile
+      profile.update_attribute(:first_name, "</script><script>alert(0);</script>");
+      stub!(:user_signed_in?).and_return true
+      stub!(:current_user).and_return user
+      set_current_user_in_javascript.should_not be_empty
+      set_current_user_in_javascript.should_not include(profile.first_name)
+    end
+  end
+
   describe "#page_title" do
     context "passed blank text" do
       it "returns Diaspora*" do
diff --git a/spec/javascripts/widgets/search-spec.js b/spec/javascripts/widgets/search-spec.js
new file mode 100644
index 0000000..0e06516
--- /dev/null
+++ b/spec/javascripts/widgets/search-spec.js
@@ -0,0 +1,12 @@
+describe("Diaspora.Widgets.Search", function() {
+    describe("parse", function() {
+        it("escapes a persons name", function() {
+            $("#jasmine_content").html('<form action="#" id="searchForm"></form>');
+
+            var search = Diaspora.BaseWidget.instantiate("Search", $("#jasmine_content > #searchForm"));
+            var person = {"name": "</script><script>alert('xss');</script"};
+            result = search.parse([$.extend({}, person)]);
+            expect(result[0].data.name).toNotEqual(person.name);
+        });
+    });
+});
Member

jhass commented Feb 1, 2013

Next iteration of the patch, please review:

diff --git a/app/assets/javascripts/widgets/search.js b/app/assets/javascripts/widgets/search.js
index 4648cdf..675b6f8 100644
--- a/app/assets/javascripts/widgets/search.js
+++ b/app/assets/javascripts/widgets/search.js
@@ -36,11 +36,12 @@
     };

     this.formatResult = function(row) {
-      return row.name;
+      return Handlebars.Utils.escapeExpression(row.name);
     };

     this.parse = function(data) {
       var results =  data.map(function(person){
+        person['name'] = Handlebars.Utils.escapeExpression(person['name']);
         return {data : person, value : person['name']}
       });

diff --git a/app/helpers/layout_helper.rb b/app/helpers/layout_helper.rb
index c226aff..d29b124 100644
--- a/app/helpers/layout_helper.rb
+++ b/app/helpers/layout_helper.rb
@@ -43,7 +43,7 @@ module LayoutHelper
     user = UserPresenter.new(current_user, a_ids).to_json
     content_tag(:script) do
       <<-JS.html_safe
-        window.current_user_attributes = #{user}
+        window.current_user_attributes = #{j user}
       JS
     end
   end
diff --git a/app/views/people/show.html.haml b/app/views/people/show.html.haml
index 9dd05ad..a2b18ca 100644
--- a/app/views/people/show.html.haml
+++ b/app/views/people/show.html.haml
@@ -7,7 +7,7 @@
   = javascript_include_tag :people
   - if user_signed_in? && @person != current_user.person
     :javascript
-      Mentions.options.prefillMention = Mentions._contactToMention(#{@person.to_json});
+      Mentions.options.prefillMention = Mentions._contactToMention(#{j @person.to_json});

 - content_for :page_title do
   = @person.name
diff --git a/config/initializers/json_escape.rb b/config/initializers/json_escape.rb
new file mode 100644
index 0000000..a27bf2d
--- /dev/null
+++ b/config/initializers/json_escape.rb
@@ -0,0 +1,11 @@
+# From http://jfire.io/blog/2012/04/30/how-to-securely-bootstrap-json-in-a-rails-view/
+# Review on Rails 4 update, might be built in by then!
+
+class ActionView::Base
+  def json_escape(s)
+    result = s.to_s.gsub('/', '\/')
+    s.html_safe? ? result.html_safe : result
+  end
+
+  alias j json_escape
+end
diff --git a/spec/controllers/people_controller_spec.rb b/spec/controllers/people_controller_spec.rb
index 2225090..c38ed51 100644
--- a/spec/controllers/people_controller_spec.rb
+++ b/spec/controllers/people_controller_spec.rb
@@ -201,11 +201,10 @@ describe PeopleController do
     it 'does not allow xss attacks' do
       user2 = bob
       profile = user2.profile
-      profile.first_name = "<script> alert('xss attack');</script>"
-      profile.save
+      profile.update_attribute(:first_name, "</script><script> alert('xss attack');</script>")
       get :show, :id => user2.person.to_param
       response.should be_success
-      response.body.match(profile.first_name).should be_false
+      response.body.should_not include(profile.first_name)
     end


diff --git a/spec/helpers/layout_helper_spec.rb b/spec/helpers/layout_helper_spec.rb
index f63da63..dd11e36 100644
--- a/spec/helpers/layout_helper_spec.rb
+++ b/spec/helpers/layout_helper_spec.rb
@@ -5,6 +5,18 @@
 require 'spec_helper'

 describe LayoutHelper do
+  describe "#set_current_user_in_javascript" do
+    it "doesn't allow xss" do
+      user = FactoryGirl.create :user
+      profile = user.profile
+      profile.update_attribute(:first_name, "</script><script>alert(0);</script>");
+      stub!(:user_signed_in?).and_return true
+      stub!(:current_user).and_return user
+      set_current_user_in_javascript.should_not be_empty
+      set_current_user_in_javascript.should_not include(profile.first_name)
+    end
+  end
+
   describe "#page_title" do
     context "passed blank text" do
       it "returns Diaspora*" do
diff --git a/spec/javascripts/widgets/search-spec.js b/spec/javascripts/widgets/search-spec.js
new file mode 100644
index 0000000..0e06516
--- /dev/null
+++ b/spec/javascripts/widgets/search-spec.js
@@ -0,0 +1,12 @@
+describe("Diaspora.Widgets.Search", function() {
+    describe("parse", function() {
+        it("escapes a persons name", function() {
+            $("#jasmine_content").html('<form action="#" id="searchForm"></form>');
+
+            var search = Diaspora.BaseWidget.instantiate("Search", $("#jasmine_content > #searchForm"));
+            var person = {"name": "</script><script>alert('xss');</script"};
+            result = search.parse([$.extend({}, person)]);
+            expect(result[0].data.name).toNotEqual(person.name);
+        });
+    });
+});
@desyncr

This comment has been minimized.

Show comment
Hide comment
@desyncr

desyncr Feb 1, 2013

Contributor

Looks good to me. Maybe a rake task to update existing vuln records.

Contributor

desyncr commented Feb 1, 2013

Looks good to me. Maybe a rake task to update existing vuln records.

@jhass

This comment has been minimized.

Show comment
Hide comment
@jhass

jhass Feb 1, 2013

Member

I prefer to store what a user enters and render it in a safe manner, that's why I didn't even touch the validations.

Member

jhass commented Feb 1, 2013

I prefer to store what a user enters and render it in a safe manner, that's why I didn't even touch the validations.

@denschub

This comment has been minimized.

Show comment
Hide comment
@denschub

denschub Feb 1, 2013

Member

@MrZYX :shipit:

Member

denschub commented Feb 1, 2013

@MrZYX :shipit:

@Raven24

This comment has been minimized.

Show comment
Hide comment
@Raven24

Raven24 Feb 1, 2013

Member

looking good, I'd say hotfix it

Member

Raven24 commented Feb 1, 2013

looking good, I'd say hotfix it

jhass added a commit that referenced this issue Feb 1, 2013

Fix XSS vulnerabilities caused by not escaping a users name fields wh…
…en loading it from JSON. #3948

From a quick look at the for us available databases this was not actually used in the wild.
@DeadSuperHero

This comment has been minimized.

Show comment
Hide comment
@DeadSuperHero

DeadSuperHero Feb 1, 2013

Member

Let me know when we hot fix this, I'll help get the word out to podmins. :)

On Friday, February 1, 2013, Florian Staudacher wrote:

looking good, I'd say hotfix it


Reply to this email directly or view it on GitHubhttps://github.com/diaspora/diaspora/issues/3948#issuecomment-13014933.

Member

DeadSuperHero commented Feb 1, 2013

Let me know when we hot fix this, I'll help get the word out to podmins. :)

On Friday, February 1, 2013, Florian Staudacher wrote:

looking good, I'd say hotfix it


Reply to this email directly or view it on GitHubhttps://github.com/diaspora/diaspora/issues/3948#issuecomment-13014933.

@jhass

This comment has been minimized.

Show comment
Hide comment
@jhass

jhass Feb 1, 2013

Member

Fixed with the release of 0.0.2.4.

@CodeKevin Thank you for pointing this out, however we would welcome a more responsible disclosure, via any available non public channel, very much in the future.

Member

jhass commented Feb 1, 2013

Fixed with the release of 0.0.2.4.

@CodeKevin Thank you for pointing this out, however we would welcome a more responsible disclosure, via any available non public channel, very much in the future.

@jhass jhass closed this Feb 1, 2013

@Flaburgan

This comment has been minimized.

Show comment
Hide comment
@Flaburgan

Flaburgan Feb 2, 2013

Member

PoC and the patch released during the same day, congratulation guys !
(Oracle and Adobe should look at how you are working :p)

Member

Flaburgan commented Feb 2, 2013

PoC and the patch released during the same day, congratulation guys !
(Oracle and Adobe should look at how you are working :p)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment