presidentbeef / brakeman

A static analysis security vulnerability scanner for Ruby on Rails applications
https://brakemanscanner.org/
Other
7.01k stars 734 forks source link

xss can be avoided only by using h, raw/html_safe wont get identified #516

Closed ff-apujari closed 10 years ago

presidentbeef commented 10 years ago

I'm sorry, can you elaborate a little bit?

ff-apujari commented 10 years ago

this is what I am getting with latest version of brakeman on rails 4, ruby 2

<%= @resource.attr %> => xss with high confidence

<%= @resource.attr.to_s.html_safe %> => xss with high confidence

<%= raw(@resource.attr) %> => xss with weak confidence

<%= h(@resource.attr) %> => no error

I think in all above cases raw, html_safe and h it should not raise security err

right?

presidentbeef commented 10 years ago

It looks like Brakeman does not know this is a Rails 4 application.

Does the output include [Notice] Detected Rails 4 application? Does the report show the correct version of Rails?

ff-apujari commented 10 years ago

Rails Version = Unknown

presidentbeef commented 10 years ago

Do you have a Gemfile? Gemfile.lock? Does your app have a script directory?

ff-apujari commented 10 years ago

yes all of that

presidentbeef commented 10 years ago

Hi Amol,

Are there errors about parsing the Gemfile?

ff-apujari commented 10 years ago

no, it does not throw any error neither display in any form ...

lets have skype ..this is taking long ..please

my id is skype.amol

or hangout amolpujari@gmail.com

presidentbeef commented 10 years ago

Hi Amol,

The only theory I have is that there is an issue parsing the Gemfile/Gemfile.lock. I do not know what I would talk about. If you can provide that file, that would be the most helpful thing at this point.

ff-apujari commented 10 years ago
source 'https://gemrepo.secret.net'

gem 'rails', '4.0.5'
gem 'activeresource', '>= 4.0'
gem 'mysql2'

# Gems used only for assets
gem 'sass', '~> 3.2.5'
gem 'sprockets', '2.11.0'
gem 'sprockets-rails', '2.0.1'
gem 'sass-rails'
gem 'coffee-rails'
gem 'uglifier'
gem 'jquery-rails'
gem 'jquery-ui-rails'
gem 'jquery-datatables-rails'
gem 'chosen-rails'
gem 'paperclip'
gem 'multipart-post'
gem 'exception_notification'
gem 'bcdatabase'
gem 'tinymce-rails', '4.0.19'
gem 'crumble', :require => 'breadcrumb'
gem 'cancancan'
gem 'protected_attributes' #REB TODO Please remove at a later time
gem 'geokit-rails'
gem 'geocoder'
gem 'timezone'
gem 'jbuilder'
gem 'holidays'

gem 'simple_form'
gem 'aws-s3'
gem 'nested_form'
gem 'acts_as_list'
gem 'numbers_and_words'
gem 'html_truncator'
gem 'acts-as-taggable-on'
gem 'letsrate'
gem 'uuidtools'
gem 'will_paginate'
gem 'jQuery-Validation-Engine-rails'
gem 'zip' #Used for Veracode packaging (Needs to be all environments or production)

if Gem.win_platform?
  gem 'nokogiri-happymapper', :require => 'happymapper'
  gem 'libxml-ruby' , '2.3.0'
else
  gem 'happymapper'
end

group :production do
  gem 'therubyracer', :platforms => :ruby
end
ff-apujari commented 10 years ago
GEM
  remote: https://gemrepo.secret.net/
  specs:
    actionmailer (4.0.5)
      actionpack (= 4.0.5)
      mail (~> 2.5.4)
    actionpack (4.0.5)
      activesupport (= 4.0.5)
      builder (~> 3.1.0)
      erubis (~> 2.7.0)
      rack (~> 1.5.2)
      rack-test (~> 0.6.2)
    activemodel (4.0.5)
      activesupport (= 4.0.5)
      builder (~> 3.1.0)
    activerecord (4.0.5)
      activemodel (= 4.0.5)
      activerecord-deprecated_finders (~> 1.0.2)
      activesupport (= 4.0.5)
      arel (~> 4.0.0)
    activerecord-deprecated_finders (1.0.3)
    activeresource (4.0.0)
      activemodel (~> 4.0)
      activesupport (~> 4.0)
      rails-observers (~> 0.1.1)
    activesupport (4.0.5)
      i18n (~> 0.6, >= 0.6.9)
      minitest (~> 4.2)
      multi_json (~> 1.3)
      thread_safe (~> 0.1)
      tzinfo (~> 0.3.37)
    acts-as-taggable-on (3.2.6)
      activerecord (>= 3, < 5)
    acts_as_list (0.4.0)
      activerecord (>= 3.0)
    addressable (2.3.6)
    ansi (1.4.3)
    arel (4.0.2)
    arrayfields (4.9.2)
    ast (2.0.0)
    awesome_print (1.2.0)
    aws-s3 (0.6.3)
      builder
      mime-types
      xml-simple
    bcdatabase (1.2.4)
      highline (~> 1.5, < 1.6.9)
      thor (~> 0.14)
    biggs (0.3.3)
      activerecord (>= 3.0)
    binding_of_caller (0.7.2)
      debug_inspector (>= 0.0.1)
    brakeman-min (2.6.0)
      multi_json (~> 1.2)
      ruby2ruby (~> 2.0.5)
      ruby_parser (~> 3.5.0)
    builder (3.1.4)
    cancancan (1.8.2)
    cane (2.6.2)
      parallel
    capybara (2.3.0)
      mime-types (>= 1.16)
      nokogiri (>= 1.3.3)
      rack (>= 1.0.0)
      rack-test (>= 0.5.4)
      xpath (~> 2.0)
    capybara-accessible (0.2.1)
      capybara (~> 2.0)
    celluloid (0.15.2)
      timers (~> 1.1.0)
    childprocess (0.5.3)
      ffi (~> 1.0, >= 1.0.11)
    chosen-rails (1.1.0)
      coffee-rails (>= 3.2)
      compass-rails (>= 1.1.2)
      railties (>= 3.0)
      sass-rails (>= 3.2)
    chronic (0.10.2)
    chunky_png (1.3.1)
    churn (0.0.35)
      chronic (>= 0.2.3)
      hirb
      json_pure
      main
      rest-client (>= 1.6.0)
      ruby_parser (~> 3.0)
      sexp_processor (~> 4.1)
    climate_control (0.0.3)
      activesupport (>= 3.0)
    cliver (0.3.2)
    cocaine (0.5.4)
      climate_control (>= 0.0.3, < 1.0)
    code_analyzer (0.4.5)
      sexp_processor
    code_metrics (0.1.3)
    coderay (1.1.0)
    coffee-rails (4.0.1)
      coffee-script (>= 2.2.0)
      railties (>= 4.0.0, < 5.0)
    coffee-script (2.2.0)
      coffee-script-source
      execjs
    coffee-script-source (1.7.0)
    colored (1.2)
    columnize (0.8.9)
    compass (0.12.6)
      chunky_png (~> 1.2)
      fssm (>= 0.2.7)
      sass (~> 3.2.19)
    compass-rails (1.1.7)
      compass (>= 0.12.2)
      sprockets (<= 2.11.0)
    coolline (0.4.3)
    crack (0.4.2)
      safe_yaml (~> 1.0.0)
    crumble (0.1.3)
    debug_inspector (0.0.2)
    debugger (1.6.8)
      columnize (>= 0.3.1)
      debugger-linecache (~> 1.2.0)
      debugger-ruby_core_source (~> 1.3.5)
    debugger-linecache (1.2.0)
    debugger-ruby_core_source (1.3.5)
    diff-lcs (1.2.5)
    diffy (3.0.4)
    docile (1.1.5)
    erubis (2.7.0)
    exception_notification (4.0.1)
      actionmailer (>= 3.0.4)
      activesupport (>= 3.0.4)
    execjs (2.2.0)
    factory_girl (4.4.0)
      activesupport (>= 3.0.0)
    factory_girl_rails (4.4.1)
      factory_girl (~> 4.4.0)
      railties (>= 3.0.0)
    fattr (2.2.2)
    ffi (1.9.3)
    flay (2.5.0)
      ruby_parser (~> 3.0)
      sexp_processor (~> 4.0)
    flog (4.2.1)
      ruby_parser (~> 3.1, > 3.1.0)
      sexp_processor (~> 4.4)
    formatador (0.2.5)
    fssm (0.2.10)
    geocoder (1.2.2)
    geokit (1.8.5)
      multi_json (>= 1.3.2)
    geokit-rails (2.0.1)
      geokit (~> 1.5)
      rails (>= 3.0)
    grit (2.5.0)
      diff-lcs (~> 1.1)
      mime-types (~> 1.15)
      posix-spawn (~> 0.3.6)
    guard (2.6.1)
      formatador (>= 0.2.4)
      listen (~> 2.7)
      lumberjack (~> 1.0)
      pry (>= 0.9.12)
      thor (>= 0.18.1)
    guard-rspec (4.2.10)
      guard (~> 2.1)
      rspec (>= 2.14, < 4.0)
    happymapper (0.4.1)
      libxml-ruby (~> 2.0)
    highline (1.6.8)
    hike (1.2.3)
    hirb (0.7.2)
    holepicker (0.3.3)
      json (~> 1.8)
      rainbow (~> 2.0)
    holidays (1.0.6)
    html_truncator (0.4.0)
      nokogiri (~> 1.5)
    i18n (0.6.9)
    jQuery-Validation-Engine-rails (0.0.2)
    jazz_hands (0.5.2)
      awesome_print (~> 1.2)
      coolline (>= 0.4.2)
      hirb (~> 0.7.1)
      pry (~> 0.9.12)
      pry-debugger (~> 0.2.2)
      pry-doc (~> 0.4.6)
      pry-git (~> 0.2.3)
      pry-rails (~> 0.3.2)
      pry-remote (>= 0.1.7)
      pry-stack_explorer (~> 0.4.9)
      railties (>= 3.0, < 5.0)
    jbuilder (2.1.1)
      activesupport (>= 3.0.0, < 5)
      multi_json (~> 1.2)
    jquery-datatables-rails (1.12.2)
      jquery-rails
    jquery-rails (3.1.0)
      railties (>= 3.0, < 5.0)
      thor (>= 0.14, < 2.0)
    jquery-ui-rails (4.2.1)
      railties (>= 3.2.16)
    jslint_on_rails (1.1.1)
    json (1.8.1)
    json_pure (1.8.1)
    launchy (2.4.2)
      addressable (~> 2.3)
    letsrate (1.0.9)
    libv8 (3.16.14.3)
    libxml-ruby (2.7.0)
    listen (2.7.8)
      celluloid (>= 0.15.2)
      rb-fsevent (>= 0.9.3)
      rb-inotify (>= 0.9)
    log4r (1.1.10)
    lumberjack (1.0.6)
    mail (2.5.4)
      mime-types (~> 1.16)
      treetop (~> 1.4.8)
    main (6.0.0)
      arrayfields (>= 4.7.4)
      chronic (>= 0.6.2)
      fattr (>= 2.2.0)
      map (>= 5.1.0)
    map (6.5.4)
    method_source (0.8.2)
    metric_fu (4.11.1)
      cane (~> 2.5, >= 2.5.2)
      churn (~> 0.0.35)
      code_metrics (~> 0.1)
      coderay
      flay (~> 2.1, >= 2.0.1)
      flog (~> 4.1, >= 4.1.1)
      launchy (~> 2.0)
      metric_fu-Saikuro (~> 1.1, >= 1.1.3)
      multi_json
      rails_best_practices (~> 1.14, >= 1.14.3)
      redcard
      reek (~> 1.3, >= 1.3.4)
      roodi (~> 3.1)
    metric_fu-Saikuro (1.1.3)
    mime-types (1.25.1)
    mini_portile (0.6.0)
    minitest (4.7.5)
    multi_json (1.10.1)
    multipart-post (2.0.0)
    mysql2 (0.3.16)
    nested_form (0.3.2)
    nokogiri (1.6.2.1)
      mini_portile (= 0.6.0)
    numbers_and_words (0.10.0)
      activesupport
      i18n
    paperclip (4.1.1)
      activemodel (>= 3.0.0)
      activesupport (>= 3.0.0)
      cocaine (~> 0.5.3)
      mime-types
    parallel (1.0.0)
    parser (2.1.9)
      ast (>= 1.1, < 3.0)
      slop (~> 3.4, >= 3.4.5)
    poltergeist (1.5.1)
      capybara (~> 2.1)
      cliver (~> 0.3.1)
      multi_json (~> 1.0)
      websocket-driver (>= 0.2.0)
    polyglot (0.3.5)
    posix-spawn (0.3.8)
    powerpack (0.0.9)
    protected_attributes (1.0.8)
      activemodel (>= 4.0.1, < 5.0)
    pry (0.9.12.6)
      coderay (~> 1.0)
      method_source (~> 0.8)
      slop (~> 3.4)
    pry-debugger (0.2.2)
      debugger (~> 1.3)
      pry (~> 0.9.10)
    pry-doc (0.4.6)
      pry (>= 0.9)
      yard (>= 0.8)
    pry-git (0.2.3)
      diffy
      grit
      pry (>= 0.9.8)
    pry-rails (0.3.2)
      pry (>= 0.9.10)
    pry-remote (0.1.8)
      pry (~> 0.9)
      slop (~> 3.0)
    pry-stack_explorer (0.4.9.1)
      binding_of_caller (>= 0.7)
      pry (>= 0.9.11)
    puma (2.8.2)
      rack (>= 1.1, < 2.0)
    quiet_assets (1.0.3)
      railties (>= 3.1, < 5.0)
    rack (1.5.2)
    rack-test (0.6.2)
      rack (>= 1.0)
    rails (4.0.5)
      actionmailer (= 4.0.5)
      actionpack (= 4.0.5)
      activerecord (= 4.0.5)
      activesupport (= 4.0.5)
      bundler (>= 1.3.0, < 2.0)
      railties (= 4.0.5)
      sprockets-rails (~> 2.0.0)
    rails-observers (0.1.2)
      activemodel (~> 4.0)
    rails_best_practices (1.15.4)
      activesupport
      awesome_print
      code_analyzer (>= 0.4.3)
      colored
      erubis
      i18n
      json
      require_all
      ruby-progressbar
    railties (4.0.5)
      actionpack (= 4.0.5)
      activesupport (= 4.0.5)
      rake (>= 0.8.7)
      thor (>= 0.18.1, < 2.0)
    rainbow (2.0.0)
    rake (10.3.2)
    rb-fsevent (0.9.4)
    rb-inotify (0.9.5)
      ffi (>= 0.5.0)
    rdoc (4.1.1)
      json (~> 1.4)
    redcard (1.1.0)
    reek (1.3.7)
      rainbow
      ruby2ruby (~> 2.0.8)
      ruby_parser (~> 3.3)
      sexp_processor
    ref (1.0.5)
    require_all (1.3.2)
    rest-client (1.6.7)
      mime-types (>= 1.16)
    roodi (3.3.1)
      ruby_parser (~> 3.2, >= 3.2.2)
    rspec (3.0.0)
      rspec-core (~> 3.0.0)
      rspec-expectations (~> 3.0.0)
      rspec-mocks (~> 3.0.0)
    rspec-core (3.0.1)
      rspec-support (~> 3.0.0)
    rspec-expectations (3.0.1)
      diff-lcs (>= 1.2.0, < 2.0)
      rspec-support (~> 3.0.0)
    rspec-mocks (3.0.1)
      rspec-support (~> 3.0.0)
    rspec-rails (3.0.1)
      actionpack (>= 3.0)
      activesupport (>= 3.0)
      railties (>= 3.0)
      rspec-core (~> 3.0.0)
      rspec-expectations (~> 3.0.0)
      rspec-mocks (~> 3.0.0)
      rspec-support (~> 3.0.0)
    rspec-support (3.0.0)
    rspec_junit_formatter (0.2.0)
      builder (< 4)
      rspec (>= 2, < 4)
      rspec-core (!= 2.12.0)
    rubocop (0.23.0)
      json (>= 1.7.7, < 2)
      parser (~> 2.1.9)
      powerpack (~> 0.0.6)
      rainbow (>= 1.99.1, < 3.0)
      ruby-progressbar (~> 1.4)
    ruby-progressbar (1.5.1)
    ruby2ruby (2.0.8)
      ruby_parser (~> 3.1)
      sexp_processor (~> 4.0)
    ruby_parser (3.5.0)
      sexp_processor (~> 4.1)
    rubyzip (1.1.4)
    rufus-scheduler (3.0.8)
      tzinfo
    safe_yaml (1.0.3)
    sass (3.2.19)
    sass-rails (4.0.3)
      railties (>= 4.0.0, < 5.0)
      sass (~> 3.2.0)
      sprockets (~> 2.8, <= 2.11.0)
      sprockets-rails (~> 2.0)
    sdoc (0.4.0)
      json (~> 1.8)
      rdoc (~> 4.0, < 5.0)
    selenium-webdriver (2.42.0)
      childprocess (>= 0.5.0)
      multi_json (~> 1.0)
      rubyzip (~> 1.0)
      websocket (~> 1.0.4)
    sexp_processor (4.4.3)
    shoulda-matchers (2.6.1)
      activesupport (>= 3.0.0)
    simple_form (3.0.2)
      actionpack (~> 4.0)
      activemodel (~> 4.0)
    simplecov (0.8.2)
      docile (~> 1.1.0)
      multi_json
      simplecov-html (~> 0.8.0)
    simplecov-html (0.8.0)
    slop (3.5.0)
    spring (1.1.3)
    spring-commands-rspec (1.0.2)
      spring (>= 0.9.1)
    sprockets (2.11.0)
      hike (~> 1.2)
      multi_json (~> 1.0)
      rack (~> 1.0)
      tilt (~> 1.1, != 1.3.0)
    sprockets-rails (2.0.1)
      actionpack (>= 3.0)
      activesupport (>= 3.0)
      sprockets (~> 2.8)
    therubyracer (0.12.1)
      libv8 (~> 3.16.14.0)
      ref
    thor (0.19.1)
    thread_safe (0.3.4)
    tilt (1.4.1)
    timers (1.1.0)
    timezone (0.3.2)
    tinymce-rails (4.0.19)
      railties (>= 3.1.1)
    treetop (1.4.15)
      polyglot
      polyglot (>= 0.3.1)
    turn (0.9.7)
      ansi
      minitest (~> 4)
    tzinfo (0.3.39)
    uglifier (2.5.1)
      execjs (>= 0.3.0)
      json (>= 1.8.0)
    uuidtools (2.1.4)
    vcr (2.9.2)
    webmock (1.18.0)
      addressable (>= 2.3.6)
      crack (>= 0.3.2)
    websocket (1.0.7)
    websocket-driver (0.3.3)
    will_paginate (3.0.5)
    xml-simple (1.1.3)
    xpath (2.0.0)
      nokogiri (~> 1.3)
    yard (0.8.7.4)
    zip (2.0.2)

PLATFORMS
  ruby

DEPENDENCIES
  activeresource (>= 4.0)
  acts-as-taggable-on
  acts_as_list
  admin_common!
  aws-s3
  bcdatabase
  brakeman-min
  cancancan
  capybara-accessible
  chosen-rails
  coffee-rails
  common!
  crumble
  disaggregation!
  exception_notification
  factory_girl_rails
  ff-logging!
  ff-support!
  ff-util!
  first_screen!
  geocoder
  geokit-rails
  guard-rspec
  happymapper
  holepicker
  holidays
  html_truncator
  ida!
  jQuery-Validation-Engine-rails
  jazz_hands
  jbuilder
  jquery-datatables-rails
  jquery-rails
  jquery-ui-rails
  jslint_on_rails
  letsrate
  metric_fu
  multipart-post
  mysql2
  nested_form
  numbers_and_words
  paperclip
  poltergeist
  protected_attributes
  puma
  quiet_assets
  rails (= 4.0.5)
  rails_best_practices
  rb-fsevent
  rspec-rails
  rspec_junit_formatter
  rubocop
  sass (~> 3.2.5)
  sass-rails
  sdoc
  selenium-webdriver
  shoulda-matchers
  simple_form
  simplecov
  spring-commands-rspec
  sprockets (= 2.11.0)
  sprockets-rails (= 2.0.1)
  therubyracer
  timezone
  tinymce-rails (= 4.0.19)
  turn
  uglifier
  uuidtools
  vcr
  webmock
  will_paginate
  zip
ff-apujari commented 10 years ago

btw, thnx for helping out, let me know if anything else needed, or I could fix it and request a pull

presidentbeef commented 10 years ago

Weird, those work fine for me. Is your application open source?

ff-apujari commented 10 years ago

sorry for one thing

my gemfile has few private engines

I had removed them being secrets ..

I could give you one example of that ...

Gemfile

gem 'private-support', :path => '../../gems/private-support' gem 'private-plane', :path => '../../engines/private-plane'

Gemfile.lock

PATH remote: ../../gems/private-support specs: ff-util (0.0.1) factory_girl_rails guard-rspec jazz_hands jslint_on_rails puma quiet_assets rspec_junit_formatter

PATH remote: ../../engines/private-plane specs: ida (0.0.1) jQuery-Validation-Engine-rails jquery-rails jquery-ui-rails rails

and there are several such

ff-apujari commented 10 years ago

wait let me give you fresh files ...

I will get back yo you on this ..please wait ...

seems I had run the brakeman on an engine repo ..and that gemfile was not having rails entry in gem file...

meanwhile I am curious about why this main issue of h/raw/html_safe has to with rails version ...

presidentbeef commented 10 years ago

Because Rails 2 did not have raw/html_safe and did not escape template output by default, while Rails 3/4 do. The default falls back to Rails 2 currently. You can use the -3 option to force Rails 3 mode as a partial fix. I'll add a -4 option shortly.

ff-apujari commented 10 years ago

I ran brakeman into an app which does not use entire rails, not uses AR but uses AS etc So Gemfile here does not have rails entry is there any other way you could figure out it or better do not check rails version in case of h/raw/html_safe team using rails 2 would have loads of other options to tell them that raw/html_safe is not for them ... make sense ..?

and for now I will continue with -4 in our ci

presidentbeef commented 10 years ago

Sorry, Brakeman is not guaranteed to work on anything but a standard Rails application.

ff-apujari commented 10 years ago

its a great tool, why can not it support such rails apps, where rails is used partially or for engines...?

do you have any branch/tag for the same?

presidentbeef commented 10 years ago

It could support them, but since they do not have the well-defined structure most Rails applications do, there is no way to do it well. It may happen one day, #260 is still open. But it can never work as well as scanning normal Rails apps.

ff-apujari commented 10 years ago

ok..

thanks

jessieay commented 10 years ago

I'm seeing a similar issue with a Rails 4.1.4 app.

We are using = raw object.attribute all over our views, but when we use them in 2 particular views, Brakeman raises. Example:

screen shot 2014-07-16 at 4 40 06 pm

When we change those snippets of embedded ruby to = object.attribute.html_safe, brakeman no longer raises.

Since raw uses html_safe, this should not be happening. It's also odd that this is only happening in some views. Here is the output of where we are using each when the are 0 brakeman errors:

screen shot 2014-07-16 at 4 44 01 pm

Any thoughts on why this might be?

presidentbeef commented 10 years ago

Hi Jessie,

Yes, the issue is clear: I never added checks for html_safe. :flushed:

Please note Brakeman only warns about XSS when unescaped user input is detected in the output.

jessieay commented 10 years ago

@presidentbeef Not 100% sure I understand. Are you saying that all 3 should be raising XSS errors?

In my case, I don't want brakeman to raise on these. The fields are based on user input, but there is embedded HTML I need to render.

If I am understanding you correctly, should I just configure brakeman to ignore these files?

presidentbeef commented 10 years ago

Yes, I mean the inconsistency you are seeing is probably caused by Brakeman not currently warning about html_safe. I can't tell if they should warn, because I don't know what those values are.

By default, Brakeman warns about any model attributes that are output unescaped. You can ignore any model output in XSS checks with --ignore-model-output. Otherwise, I recommend using the -I option to ignore specific warnings (not whole files).

ff-apujari commented 10 years ago

am happy with -3 for now..and waiting for -4

great work guys, let me know if I could code and help you out, would love to...