Fixed vulnerabilities #1569
Fixed vulnerabilities #1569
Conversation
ryanfox1985
commented
Dec 16, 2022
ryanfox1985
commented
- unsafe reflection
- sql injection
| @@ -22,7 +22,8 @@ def get_case_insensitive_field_from_resource_params(field) | |||
| def find_resource(field, value) | |||
| @resource = if database_adapter&.include?('mysql') | |||
| # fix for mysql default case insensitivity | |||
| resource_class.where("BINARY #{field} = ? AND provider= ?", value, provider).first | |||
| field_sanitized = resource_class.connection.quote_column_name(field) | |||
| resource_class.where("BINARY #{field_sanitized} = ? AND provider= ?", value, provider).first | |||
There was a problem hiding this comment.
Confidence: Weak
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: resource_class.where("BINARY #{field} = ? AND provider= ?", value, provider)
File: gems/devise_token_auth/app/controllers/devise_token_auth/concerns/resource_finder.rb
Line: 25
| @resource_class = ObjectSpace.each_object(Class).detect { |cls| cls.name == constant_name } | ||
| raise 'No resource_class found' if @resource_class.nil? | ||
|
|
||
| @resource_class |
There was a problem hiding this comment.
Confidence: High
Category: Remote Code Execution
Check: UnsafeReflection
Message: Unsafe reflection method `constantize` called on parameter value
Code: params["resource_class"].constantize
File: gems/devise_token_auth/app/controllers/devise_token_auth/omniauth_callbacks_controller.rb
Line: 135
There was a problem hiding this comment.
@ryanfox1985 I had to rollback this because broke the build (it seems at the time that the CI's status wasn't showing correctly), if you know a different way to fix that avoids this unsafe execution, let me know.
There was a problem hiding this comment.
no worries, this PR includes some refactors besides the vulnerability fixes let me reopen in three different PRs.
There was a problem hiding this comment.
could you clarify what was broken in the build?
There was a problem hiding this comment.
oh I see what happens, it's loaded test models with the same class name in the tests...
There was a problem hiding this comment.
I found a work around to this, https://github.com/lynndylanhurley/devise_token_auth/pull/1587/files#diff-e297b1cbec8129624683adca1209383ebe6cbc3bfeebae01e590530e597dc062R138
basically the problem is in the tests some generated classes matches with name User for instance, in theory this patch covers that however, this only works for models under ActiveRecord... I don't think is possible, but, do you know if it's possible to have User without using ActiveRecord?
There was a problem hiding this comment.
oh no, there is Mongoid...
There was a problem hiding this comment.
sorry, I missed these messages, glad you found the problem
| @@ -190,7 +190,7 @@ def build_auth_header(token, client = 'default') | |||
| DeviseTokenAuth.headers_names[:"expiry"] => expiry.to_s, | |||
| DeviseTokenAuth.headers_names[:"uid"] => uid | |||
| } | |||
| headers.merge!(build_bearer_token(headers)) | |||
| headers.merge(build_bearer_token(headers)) | |||
| @@ -176,10 +176,10 @@ def create_new_auth_token(client = nil) | |||
| updated_at: now | |||
| ) | |||
|
|
|||
| update_auth_header(token.token, token.client) | |||
| update_auth_headers(token.token, token.client) | |||
There was a problem hiding this comment.
I decided to pluralize the method.
* Run `bundle update --patch` * Update usage of renamed method in `devise_token_auth` gem lynndylanhurley/devise_token_auth#1569
