presidentbeef / brakeman

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

Flag Cross-Site Scripting via loop in template #1272

Open drewmnoel opened 6 years ago

drewmnoel commented 6 years ago

Is your feature request related to a problem? Please describe. It appears that Brakeman may not pick up on Cross-Site Scripting (XSS) issues when they occurs as a result of a loop. A simple loop which marks html_safe from a param is not detected (see example at bottom). I made a repository demoing this here. Endpoints which trigger the XSS are: http://localhost:3000/unsafe?xss=%3Cscript%3Ealert(1)%3C/script%3E and http://localhost:3000/unsafe_erb?xss=%3Cscript%3Ealert(1)%3C/script%3E

Describe the solution you'd like It'd be nice if Brakeman could detect XSS that occurs as a result of a loop variable.

Describe alternatives you've considered I'm new to Brakeman, so this might not be something that can be done with the current parser. In that case, flagging a low confidence for loop variables being html_safe/raw would still be an improvement.

Additional context Consider the following minimal route and view:

Rails.application.routes.draw do
  get '/unsafe', to: 'unsafe#index'
end
- [params[:xss]].each do |xss|
  = xss.html_safe

Accessing the route such as: /unsafe?xss=<script>alert(1)</script> triggers the XSS, but Brakeman does not detect it:

== Brakeman Report ==

Application Path: /home/dnoel/src/rails-xss
Rails Version: 5.2.1
Brakeman Version: 4.3.0
Scan Date: 2018-10-11 09:11:31 -0500
Duration: 0.27845972 seconds
Checks Run: BasicAuth, BasicAuthTimingAttack, ContentTag, CreateWith, CrossSiteScripting, DefaultRoutes, Deserialize, DetailedExceptions, DigestDoS, DynamicFinders, EscapeFunction, Evaluation, Execute, FileAccess, FileDisclosure, FilterSkipping, ForgerySetting, HeaderDoS, I18nXSS, JRubyXML, JSONEncoding, JSONParsing, LinkTo, LinkToHref, MailTo, MassAssignment, MimeTypeDoS, ModelAttrAccessible, ModelAttributes, ModelSerialize, NestedAttributes, NestedAttributesBypass, NumberToCurrency, PermitAttributes, QuoteTableName, Redirect, RegexDoS, Render, RenderDoS, RenderInline, ResponseSplitting, RouteDoS, SQL, SQLCVEs, SSLVerify, SafeBufferManipulation, SanitizeMethods, SelectTag, SelectVulnerability, Send, SendFile, SessionManipulation, SessionSettings, SimpleFormat, SingleQuotes, SkipBeforeFilter, StripTags, SymbolDoSCVE, TranslateBug, UnsafeReflection, ValidationRegex, WithoutProtection, XMLDoS, YAMLParsing

== Overview ==

Controllers: 2
Models: 1
Templates: 3
Errors: 0
Security Warnings: 0

== Warning Types ==

No warnings found

Note that while my example was HAML, the same issue happens with ERB:

<% [params[:xss]].each do |xss| %>
<%= xss.html_safe %>
<% end %>
presidentbeef commented 6 years ago

Hi Drew,

Thank you for pointing this out.

I agree the values should be propagated to the block variable when possible/interesting.