presidentbeef / brakeman

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

Accept ActiveStorage::Filename#sanitized and to_i as safe #1883

Open bb opened 3 hours ago

bb commented 3 hours ago

Background

This is a follow-up to https://github.com/presidentbeef/brakeman/issues/337.

Brakeman version: 6.2.2 Rails version: 7.1.5 Ruby version: 3.3.4

Link to Rails application code: ?

False Positive

Full warning from Brakeman:

Confidence: Weak
Category: File Access
Check: SendFile
Message: Parameter value used in file name
Code: send_file("public/ogimages/#{ActiveStorage::Filename.new("post-#{MeiliSearchRails.client.index("posts").document(params[:id].to_i)["id"].to_i}").sanitized}.png", :type => "image/png", :disposition => "inline")
File: app/controllers/posts_controller.rb
Line: 64

Relevant code:

    file_local = ActiveStorage::Filename.new("alert-#{post_hash["id"].to_i}").sanitized
    file_name = "public/ogimages/#{file_local}.png"
    send_file file_name, type: "image/png", disposition: "inline"

Why might this be a false positive?

bb commented 3 hours ago

I just found about https://github.com/presidentbeef/brakeman/pull/1375 so I guess this might be because I'm only calling sanitized on the local part and adding a static path outside of the sanitization?!

presidentbeef commented 3 hours ago

Yes, see my comment on that PR 😄