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

JWT::ExpiredSignature raised for non-JSON payloads #350

Closed
volmer opened this issue Apr 13, 2020 · 6 comments
Closed

JWT::ExpiredSignature raised for non-JSON payloads #350

volmer opened this issue Apr 13, 2020 · 6 comments

Comments

@volmer
Copy link

volmer commented Apr 13, 2020

Hello!

I'm working with tokens produced in JWS to sign payloads that are not necessarily JSON, as allowed by the JWS spec.

I noticed however that when trying to decode a JWS that has a String payload that includes the "exp" substring, the JWT gem raises a JWT::ExpiredSignature error:

irb(main):007:0> rsa_private = OpenSSL::PKey::RSA.generate(2048)
irb(main):008:0> rsa_public = rsa_private.public_key
irb(main):009:0> token = JWT.encode('string payload with exp substring', rsa_private, 'RS256')
irb(main):010:0> JWT.decode(token, rsa_public, true, algorithm: 'RS256')
Traceback (most recent call last):
       12: from /Users/volmer/.rubies/ruby-2.7.1/bin/irb:23:in `<main>'
       11: from /Users/volmer/.rubies/ruby-2.7.1/bin/irb:23:in `load'
       10: from /Users/volmer/.rubies/ruby-2.7.1/lib/ruby/gems/2.7.0/gems/irb-1.2.3/exe/irb:11:in `<top (required)>'
        9: from (irb):10
        8: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt.rb:28:in `decode'
        7: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt/decode.rb:27:in `decode_segments'
        6: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt/decode.rb:64:in `verify_claims'
        5: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt/verify.rb:20:in `verify_claims'
        4: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt/verify.rb:20:in `each'
        3: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt/verify.rb:22:in `block in verify_claims'
        2: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt/verify.rb:15:in `block (2 levels) in singleton class'
        1: from /Users/volmer/.gem/ruby/2.7.1/gems/jwt-2.2.1/lib/jwt/verify.rb:41:in `verify_expiration'
JWT::ExpiredSignature (Signature has expired)

Apparently the gem is assuming the payload is always a JSON, which is not always true. What would be the best fix for this case?

Thank you in advance.

@mooreds
Copy link

mooreds commented May 18, 2020

Did you turn off the token expires validation check? You can do that by passing options to decode. Here are the defaults: https://github.com/jwt/ruby-jwt/blob/master/lib/jwt/default_options.rb

@volmer
Copy link
Author

volmer commented May 21, 2020

Indeed by setting verify_expiry: false the issue doesn't happen. While it is a quick fix for non-JSON payloads for now, I think it would be better if this gem could be a bit more agnostic about the payload format. It could simply skip all these checks if the payload is not a JSON. Wdyt?

@danleyden
Copy link
Contributor

@volmer the JWT spec (https://tools.ietf.org/html/rfc7519) specifies that the payload is a JSON object. If the payload is not parseable to a JSON object, then it is not a valid JWT....

The specific method you're using - JWT.decode - is designed to take a JWT string, parse it and validate the signature AND the claims per the JWT spec (which it does). What you are looking for is not parsing and validating a JWT, but solely verifying a signature - which goes against the JWT spec.

I would agree that the specific error raised may be incorrect, but would suggest that the JWT class is correct to require the payload to be JSON. Skipping the validation checks in the case that the payload is not JSON (i.e. is not syntactically valid) would very likely cause security issues for users who expect that only valid tokens would be successfully decoded.

There is a good case for splitting out a JWS class that would verify just the signature, regardless of the payload and having the JWT class use that as part of its decode method, prior to asserting the claims.

@volmer
Copy link
Author

volmer commented May 21, 2020

Very good point @danleyden. I think in the end what I'm really looking for is a pure JWS utility. Probably not in the scope of this gem. I agree that if the assumption is to always have a JWT passed to JWT.decode then it doesn't make sense to verify the payload and skip checks. Thanks for the discussion!

@volmer volmer closed this as completed May 21, 2020
@danleyden
Copy link
Contributor

@volmer - something to consider (in case you don't find what you're looking for)...

With some (probably not too much) effort https://github.com/jwt/ruby-jwt/blob/master/lib/jwt/decode.rb could be refactored. I think the key thing that you would need to do is to split that in to two classes... something like below (not tested)

class JWSDecode
  # everything in the current decode class except:
  def payload
    @payload ||= JWT::Base64.url_decode(@segments[1])
  end

  # no verify_claims method

  # this method is very similar to the original... only one line different
  def decode_segments
    validate_segment_count!
    if @verify
      decode_crypto
      verify_signature
      # verify_claims is not needed for JWS
    end
    raise(JWT::DecodeError, 'Not enough or too many segments') unless header && payload
    [payload, header]
  end
end

class Decode << JWSDecode
  # the only thing this needs to do extra is to verify the claims and ensure that the payload
  # is valid
  def decode_segments
    payload, header = *super
    verify_claims if @verify
    [payload, header]
  end

  # this is the original method
  def verify_claims
    Verify.verify_claims(payload, @options)
  end

  # in this class, the payload must be decoded as a token
  def payload
    @payload ||= parse_and_decode @segments[1]
  end
end

@NewAlexandria
Copy link

For anyone getting here by searching for the error in the title, here is the 2021-09 current configurations you can pass.

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

No branches or pull requests

4 participants