No xss in filters sidebar by deivid-rodriguez · Pull Request #5275 · activeadmin/activeadmin

deivid-rodriguez · 2017-12-05T11:10:12Z

Fixes #5198.
Supersedes #5265.

deivid-rodriguez · 2017-12-05T11:18:42Z

spec/unit/pretty_format_spec.rb

  end

+  it "normalizes Arbre elements" do
+    expect(pretty_format(Arbre::Element.new.br)).to eq("<br>\n")


This is an unexpected change in output from this patch. Before the ouput would be  , now it's  . Seems ok?

https://stackoverflow.com/questions/1946426/html-5-is-it-br-br-or-br#1946446

For more information.

Its fine. No need to be self closing in HTML5.

Only issue would be user searching for literal   on a text field but current filter will show  ... Could be confusing somehow but seems reeeeally edge...

And an improvement anyways because previously nothing would be displayed, just some whitespace.

zorab47 · 2017-12-05T14:56:18Z

Should it only be sanitizee if the string isn't already considered .html_safe?? Perhaps users would optionally like to output HTML? Just a thought.

deivid-rodriguez · 2017-12-05T15:03:54Z

Mmm, I think that's what sanitize does? It cleans up some backlisted tags but allows the ones considered harmless. That's why   passes through, but <script> disappears in the pretty_format unit tests.

markstory · 2017-12-08T20:44:00Z

There is still an issue in the else case when an ActiveRecord::Base instance is provided.
The else case uses ends up in auto_link or in display_name

activeadmin/lib/active_admin/view_helpers/display_helper.rb

Lines 72 to 79 in a473cb6

    
           else 
        
             if defined?(::ActiveRecord) && object.is_a?(ActiveRecord::Base) || 
        
                defined?(::Mongoid)      && object.class.include?(Mongoid::Document) 
        
               auto_link object 
        
             else 
        
               display_name object 
        
             end 
        
           end

The auto_link method uses display_name as a default value,

activeadmin/lib/active_admin/view_helpers/auto_link_helper.rb

Line 14 in 8b15ff5

def auto_link(resource, content = display_name(resource))

The current code for display_name is missing escaping

activeadmin/lib/active_admin/view_helpers/display_helper.rb

Lines 17 to 36 in a473cb6

    
           def display_name(resource) 
        
             render_in_context resource, display_name_method_for(resource) unless resource.nil? 
        
           end 
        
           # Looks up and caches the first available display name method. 
        
           # To prevent conflicts, we exclude any methods that happen to be associations. 
        
           # If no methods are available and we're about to use the Kernel's `to_s`, provide our own. 
        
           def display_name_method_for(resource) 
        
             @@display_name_methods_cache ||= {} 
        
             @@display_name_methods_cache[resource.class] ||= begin 
        
               methods = active_admin_application.display_name_methods - association_methods_for(resource) 
        
               method  = methods.detect{ |method| resource.respond_to? method } 
        
               if method != :to_s || resource.method(method).source_location 
        
                 method 
        
               else 
        
                 DISPLAY_NAME_FALLBACK 
        
               end 
        
             end 
        
           end

This an acute problem when the ActiveRecord model implements one of the name methods defined in

activeadmin/lib/active_admin/application_settings.rb

Lines 27 to 34 in 26b7c84

    
           register :display_name_methods, [ :display_name, 
        
                                             :full_name, 
        
                                             :name, 
        
                                             :username, 
        
                                             :login, 
        
                                             :title, 
        
                                             :email, 
        
                                             :to_s ]

The end result is XSS from user supplied data stored in active record models.

deivid-rodriguez · 2017-12-08T22:53:33Z

Thanks so much @markstory! Can you send us a patch fixing the other issues?

markstory · 2017-12-09T02:38:47Z

@deivid-rodriguez Sure, would you like a pull request to this branch or to master?

deivid-rodriguez · 2017-12-09T13:57:15Z

I think master is fine, it should be pretty independent from the code in this PR, right? Thanks!

The display_name helper can end up invoking methods on ActiveRecord models or other arbitrary ruby objects that can contain unsafe user supplied data resulting in XSS holes. Refs #5198 Refs #5265 Refs #5275

rdlugosz · 2017-12-21T19:42:13Z

Yes, this seems heavy handed. In our project, this strips the target= attribute out of links, so you cannot have certain links open in new tabs. This even happens when a string is marked as html_safe, (for example, if you wrap the link_to in a call to raw like: raw(link_to "foo", "bar")).

I'm all for sanitizing the strings, but perhaps the step should be ignored when strings are declared to be safe?

edit: fwiw, we're seeing this in columns in our index. I mention it since this patch was to fix an XSS problem in the sidebar, but (perhaps not obviously?) it applies much more broadly.

deivid-rodriguez · 2017-12-21T20:05:22Z

Sorry about that, fixes welcome 🙏!

Or if the old situation is preferred, we can always revert my patch and release a new patch level version. I'm sorry I don't have time to contribute again now... 😞

Remove unnecessary parameter

d691fed

deivid-rodriguez commented Dec 5, 2017

View reviewed changes

deivid-rodriguez force-pushed the fix/no_xss_in_filters_sidebar branch from 744b8d1 to aa32957 Compare December 5, 2017 14:53

Fix XSS issue in filters sidebar

0456e2d

deivid-rodriguez force-pushed the fix/no_xss_in_filters_sidebar branch from aa32957 to 0456e2d Compare December 5, 2017 17:16

markstory mentioned this pull request Dec 11, 2017

HTML encode display_name output. #5284

Merged

varyonic approved these changes Dec 15, 2017

View reviewed changes

deivid-rodriguez merged commit c872533 into master Dec 15, 2017

deivid-rodriguez deleted the fix/no_xss_in_filters_sidebar branch December 15, 2017 18:24

deivid-rodriguez mentioned this pull request Dec 15, 2017

Remove html safe to escape potential xss #5265

Closed

Uh oh!

Conversation

deivid-rodriguez commented Dec 5, 2017

Uh oh!

deivid-rodriguez Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

zorab47 Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

javierjulio Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

zorab47 commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Dec 5, 2017

Uh oh!

markstory commented Dec 8, 2017

Uh oh!

deivid-rodriguez commented Dec 8, 2017

Uh oh!

markstory commented Dec 9, 2017

Uh oh!

deivid-rodriguez commented Dec 9, 2017

Uh oh!

rdlugosz commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Dec 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

deivid-rodriguez Dec 5, 2017 •

edited

Loading

zorab47 commented Dec 5, 2017 •

edited

Loading

rdlugosz commented Dec 21, 2017 •

edited

Loading