presidentbeef / brakeman

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

Unsafe Deserialization False Negative - File access is not treated as tainted #1294

Open bannable opened 5 years ago

bannable commented 5 years ago

Background

I'm unsure if this is a functional limitation of Brakeman or a true false negative, but it seems like brakeman is not recognizing data read from a file descriptor as tainted user controlled data. If an application reads from a fd and then calls Marshal.load or similar on it, you're deserializing unsafely.

Brakeman version: 4.4.0 (pro) Rails version: 5.2 Ruby version: trunk

False Positive

Full warning from Brakeman:

== Warning Types ==

Remote Code Execution: 1

== Warnings ==

Confidence: High
Category: Remote Code Execution
Check: Deserialize
Description: `Marshal.load` called with parameter value
`Marshal` allows instantiation of arbitrary objects. Since some classes also execute custom code when deserialized, this can lead to remote code execution.
Code: Marshal.load(params[:contents])
File: lib/foo.rb
Line: 8

Relevant code:

class Foo
  def should_warn_but_does_not
    contents = File.read('foo')
    Marshal.load(contents)
  end

  def warns_correctly
    Marshal.load(params[:contents])
  end
end
presidentbeef commented 5 years ago

Hi Joe,

You are correct that Brakeman does not currently treat the file system as tainted. That would be something I'd have to look into.