Skip to content

Commit

Permalink
explicitly specifying number of elements when splitting JWE string
Browse files Browse the repository at this point in the history 
ref.) CVE-2019-18848
  • Loading branch information
@nov
nov committed Nov 11, 2019
1 parent 5a8106a commit ada16e7
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
2 changes: 1 addition & 1 deletion lib/json/jwe.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ def decode_compact_serialized(input, private_key_or_secret, algorithms = nil, en
raise InvalidFormat.new("Invalid JWE Format. JWE should include #{NUM_OF_SEGMENTS} segments.")
end
jwe = new
_header_json_, jwe.jwe_encrypted_key, jwe.iv, jwe.cipher_text, jwe.authentication_tag = input.split('.').collect do |segment|
_header_json_, jwe.jwe_encrypted_key, jwe.iv, jwe.cipher_text, jwe.authentication_tag = input.split('.', NUM_OF_SEGMENTS).collect do |segment|
begin
Base64.urlsafe_decode64 segment
rescue ArgumentError
Expand Down
4 changes: 2 additions & 2 deletions lib/json/jws.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ def decode_compact_serialized(input, public_key_or_secret, algorithms = nil, all
unless input.count('.') + 1 == NUM_OF_SEGMENTS
raise InvalidFormat.new("Invalid JWS Format. JWS should include #{NUM_OF_SEGMENTS} segments.")
end
header, claims, signature = input.split('.', JWS::NUM_OF_SEGMENTS).collect do |segment|
header, claims, signature = input.split('.', NUM_OF_SEGMENTS).collect do |segment|
Base64.urlsafe_decode64 segment.to_s
end
header = JSON.parse(header).with_indifferent_access
Expand All @@ -191,7 +191,7 @@ def decode_compact_serialized(input, public_key_or_secret, algorithms = nil, all
jws = new claims
jws.header = header
jws.signature = signature
jws.signature_base_string = input.split('.')[0, JWS::NUM_OF_SEGMENTS - 1].join('.')
jws.signature_base_string = input.split('.')[0, NUM_OF_SEGMENTS - 1].join('.')
jws.verify! public_key_or_secret, algorithms unless public_key_or_secret == :skip_verification
jws
end
Expand Down

3 comments on commit ada16e7

@bcaller
Copy link

@bcaller bcaller commented on ada16e7 Feb 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to work out why this has a CVE classed as high severity.

From what I can see, previously you could end (or start) the jwt with a '.', thereby setting jwe.authentication_tag to nil, or end with '..' to set both jwe.cipher_text and jwe.authentication_tag to nil etc. With this change, we now set the values to "" instead of nil.

Processing the jwt would have always resulted in errors. The only issue I can see is that the errors would have been a mixture of TypeError and NoMethodError and maybe others rather than DecryptionFailed. Am I missing something?

@denisenkom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a demonstration of the bug:

require 'json/jwt'
require 'json/jwe'

describe JSON::JWE do
  let :aes_key do
    SecureRandom.random_bytes 16
  end

  let :hmac_key do
    SecureRandom.random_bytes 16
  end

  let :jwe_key do
    hmac_key + aes_key
  end

  let :valid_iv do
    SecureRandom.random_bytes 16
  end

  let :plaintext do
    'plaintext'
  end

  let :valid_encrypted_block do
    aes = OpenSSL::Cipher::Cipher.new('AES-128-CBC')
    aes.encrypt
    aes.key = aes_key
    aes.iv = valid_iv
    aes.update(plaintext) + aes.final
  end

  describe '.decrypt!' do
    it 'should fail if signature field is empty' do
      hdr = {'alg' => 'dir', 'enc' => 'A128CBC-HS256', 'kid' => '1'}
      iv = Base64.urlsafe_encode64(valid_iv)
      cipher_text = Base64.urlsafe_encode64(valid_encrypted_block)
      # generate compact JWE with empty signature field
      jwe_str_no_hmac = "#{Base64.urlsafe_encode64(JSON.generate(hdr))}..#{iv}.#{cipher_text}."
      # vulnerability here, does not raise exception
      expect {
        described_class.decode_compact_serialized(jwe_str_no_hmac, jwe_key)
      }.to raise_error(JSON::JWE::DecryptionFailed)
    end
  end
end

@bcaller
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now. authentication_tag was set to nil by input.split, but what I missed was that the getter contains a default @authentication_tag ||= case.... This made verify_cbc_authentication_tag! always succeed. Thank you for the reply.

Please sign in to comment.