SECURITY VULNERABILITY - request without token, or empty token is granted access

[cs3b]

I've added knock to JSONAPI::ResourceControllerMetal and I was surprised that all my controller specs were still working (without any token). Whenever I've pass the wrong token the action was stopped.

Digging dipper give me hint that Knock::AuthToken.new(token: nil) will generate new token and Authentication is executed at all.

This is my first project where I'm using Knock - still it looks for me as serious vulnerability. I know that in User finder method I can raise an exception, still authenticate_user should never pass through request without valid token (in this case without token).

So far I've solved the problem by adding another before_action to my ApplicationController

class ApplicationController < JSONAPI::ResourceControllerMetal
  include Knock::Authenticable

  before_action :ensure_token_is_given
  before_action :authenticate_user

  # it's for knock only, and this is a very naive implementation of head method
  def head(type, options = {})
    render json: {error: type}, status: 401
  end

  def ensure_token_is_given
    head :unauthorized if token.blank?
  end
end

The most reasonable is to add this in token_from_request_headers
I'm willing to prepare pull request, still would like to have any feedback from the previous contributors.

[cs3b]

I also wonder why the constructor new in Knock::AuthToken have more then one job.

    def initialize payload: {}, token: nil, verify_options: {}
      if token.present?
        @payload, _ = JWT.decode token, decode_key, true, options.merge(verify_options)
        @token = token
      else
        @payload = claims.merge(payload)
        @token = JWT.encode @payload,
          secret_key,
          Knock.token_signature_algorithm
      end
end

is there a reason why it isn't a two separate constructors (or two separate classes sharing some behaviour through modules)?

[as I mention, I new here, so I might not see the reason behind]

[AldrichMascarenhas]

This checks out. I was wondering as well.
Using knock with Auth0, and rails is in API Mode.
I am able to make all requests IF i pass no token.

[ghost]

@AldrichMascarenhas , can you show us some examples of requests with no token using for example Postman?

[AldrichMascarenhas]

@johnunclesam
Repo : https://github.com/AldrichMascarenhas/Knock-Issue142-Example

Also image : https://github.com/AldrichMascarenhas/Knock-Issue142-Example/blob/master/Screenshot%20from%202017-02-07%2007-54-24.png

books_controller has a

before_action :authenticate_user set but it still gets bypassed.

I added the fix that @cs3b mentioned in his post to my application_controller and it successfully works.

[ehannes]

I encountered the same vulnerability when testing my application that uses knock. How does the test coverage for this gem look like?

Update

It appears as if my problem concerned namespaced models...

[cs3b]

From what I see there is no test case for context: authorization header not provided:

https://github.com/nsarno/knock/blob/master/test/dummy/test/controllers/admin_protected_controller_test.rb#L18

Can anyone confirm?

[nsarno]

@cs3b thank you for reporting this.

@AldrichMascarenhas thank you for providing example code.

Knock responsibility is to assert the existence of a correctly signed token. I agree that in the case where a token isn't provided, the execution should be stopped as soon as possible by Knock and it should not try to instantiate a user. This need to be fixed (I'm happy to do it myself or review someone else's PR).

What this issue is not:
I don't believe you can take advantage of this issue to get access to another user's data.

What this issue is:
Knock should not put the developer in a position where they have to find or create a user from an empty payload. Although doing so should not result in a user being granted access as basic validation should be performed on the payload data.

In the case of the example code provided, this would mean actually checking that the sub key is present (using fetch for example) and that the value contained is not nil.

--

I also wonder why the constructor new in Knock::AuthToken have more then one job.

No good reason. It's one of the things I've been willing to refactor but haven't taken the time yet.

From what I see there is no test case for context: authorization header not provided:

Isn't the test at line 20 what you're looking for?

Sorry if the test description isn't explicit. Although I don't believe knock lacks test coverage, they could definitely be improved in term of readability.

[nsarno]

#143 should address the issue.

@cs3b (and others) Could you please review it and let me know if I'm missing something?

Ultimately, I believe a deeper refactoring of the Authenticable module and the AuthToken class would be needed. SRP and readability have been compromised in multiple places which goes against what I think this gem should be.

Thank you everyone for the precious feedback.

[cs3b]

@nsarno yes it looks good - thank you 👍