Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File type validator not checking the type of :tempfile #1841

Closed
Nyangawa opened this issue Dec 12, 2018 · 5 comments
Closed

File type validator not checking the type of :tempfile #1841

Nyangawa opened this issue Dec 12, 2018 · 5 comments
Labels

Comments

@Nyangawa
Copy link

Consider following parameter declaration

      params do
        requires :file, type: File, desc: 'The file to be uploaded'
      end

Currently, the File validator is only validating the existence of key :tempfile

# lib/grape/validations/types/file.rb
        def value_coerced?(value)
          # Rack::Request creates a Hash with filename,
          # content type and an IO object. Do a bit of basic
          # duck-typing.
          value.is_a?(::Hash) && value.key?(:tempfile)
        end

Rack will set params[:file][:tempfile] to a TempFile object and params[:file][:filename] to a String if the end user really uploads a file.

However, the value of params[:file][:tempfile] is easily controllable by sending a normal POST request

curl -F "file[tempfile]=/etc/passwd" -F "file[filename]=myfile" http://host:port

Here the request will pass the check value.key?(:tempfile) while it's not a valid file uploading request in fact.

I think this issue is a risk and has caused some security issues (https://nvd.nist.gov/vuln/detail/CVE-2018-18649 for example). So I suggest to patch this validator to validate the type of :tempfile. Here is a great example

@dblock
Copy link
Member

dblock commented Dec 13, 2018

This makes sense, appreciate a PR.

@Nyangawa
Copy link
Author

Sure, PR submitted

@dm1try
Copy link
Member

dm1try commented Dec 13, 2018

PR totally makes sense

but the code below is 😢

File.read(attrs[:file][:tempfile]) # instead of attrs[:file][:tempfile].read

File.read(Tempfile.new) is even undocumented

@dblock
Copy link
Member

dblock commented Dec 14, 2018

Closed via #1844.

@dblock dblock closed this as completed Dec 14, 2018
@dblock
Copy link
Member

dblock commented Dec 14, 2018

@dm1try I think the whole point is that it can be a String which would cause you to read a file on the system like /etc/passwd and better safe than sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants