search

jruby / jruby-openssl

JRuby's OpenSSL gem
http://www.jruby.org
Other
45 stars 79 forks source link

EC key initialization failures: investigation and workaround (PKey, PKeyEC, PKeyEC.Group) #289

Closed mattys101 closed 4 months ago

mattys101 commented 6 months ago

I have been trying to get the ruby-jwt library working with jruby-openssl and have made some headway in identifying some issues. The main issues are around EC keys and PSS keys, the latter is completely missing #288, while I have somewhat of a workaround for the former.

The key problem with the EC implementation (pun intended šŸ˜‰) is twofold:

This generally results in a NullPointerException as there is no curve name set in one or both of the places.

(A third issues is the inconsistent signing/verification interface with the standard OpenSSL library which is identified in #241 and #255)

Inconsistent initialization

The behaviour of OpenSSL::PKey.read and OpenSSL::Pkey::EC.new is not the same:

Fixing the curve name information would partly address #189, #256, and #257, but private keys still do not produce the correct output from to_pem, although it does appear to correct public keys.

Curve Name access

The curve name is stored in both PKeyEC and PKeyEC.Group and there are several accessors, some as ruby methods, some as private java methods, some that do additional processing, and others that access the value raw. This is a recipe for disaster and leads to inconsistent usage of curve name, ultimately leading to unexpected NullPointerExceptions in various cases when things are not setup properly.

Fix: refactor where and how the key information, including curve name, is captured and retrieved in the PKeyEC and PKeyEC.Group classes. Ideally, there should be only one location where the information is stored, likely on the group (or possibly retrieved from the underlying ECNamedCurve object), and the objects should delegate as appropriate. Moreover, the ruby and plain Java methods should be consistent and used consistently.

A similar treatment should be make across the PKeyEC class and its nested classes. As the Group and Point classes are nested classes, there is no reason they cannot leverage shared code. This would allow, for example, an easy avenue for implementing instantiation of PKey::EC::Group objects from PEM/DER strings consistently as for PKey::EC objects, and making the interfaces more compatible with the standard Ruby OpenSSL library.

Workaround

I have a temporary workaround at the ruby level for the moment. Note that it only works if you retrieve the curve_name from the group before using the key as is done in ruby-jwt to verify the algorithm. This will ensure the curve name is consistent in both the key and group objects, avoiding any unexpected NullPointerExceptions.

if defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby'
  class Java::OrgJrubyExtOpenssl::PKeyEC
    field_accessor :curveName
  end

  # Make the paramSpec private field available to ruby from the Java class
  class Java::OrgJrubyExtOpenssl::PKeyEC::Group
    field_accessor :paramSpec, :curve_name, :key
  end

  class OpenSSL::PKey::EC::Group
    # Save the original curve_name implementation
    alias_method :old_curve_name, :curve_name

    # Override the curve_name implementation to catch try an alternative
    # means of retreiving the curve name when the private field is null
    # Basically, just get it from the curve parameters if it is a named curve.
    def curve_name
      old_curve_name
    rescue java.lang.NullPointerException
      # Get access to the internal fields
      internal_group = self.to_java
      if internal_group.paramSpec.kind_of?(Java::OrgBouncycastleJceSpec::ECNamedCurveSpec)
        name = internal_group.paramSpec.name
        internal_group.curve_name = name
        internal_group.key.to_java(org.jruby.ext.openssl.PKeyEC).curveName = name
        return name
      end
      nil
    end
  end

  class OpenSSL::PKey::EC
    # Support generation as a class method (may not be perfectly equivalent)
    def self.generate(ec_group)
      key = self.new(ec_group)
      key.generate_key
      key
    end
  end

  # Only include the following if working with ruby-jwt
  module JWT

    def self.openssl_3_hmac_empty_key_regression?
      # assuming Bouncy Castle does not have this regression.
      false
    end

    module Algos
      module Ecdsa
        module_function

        def verify(algorithm, public_key, signing_input, signature)
          curve_definition = curve_by_name(public_key.group.curve_name)
          key_algorithm = curve_definition[:algorithm]
          if algorithm != key_algorithm
            raise IncorrectAlgorithm, "payload algorithm is #{algorithm} but #{key_algorithm} verification key was provided"
          end

          digest = OpenSSL::Digest.new(curve_definition[:digest])
          # JRuby OpenSSL does not implement dsa_verify_asn1 and difficult to just
          # add it as an extension method as the PKey.verify takes args that would be lost.
          #public_key.dsa_verify_asn1(digest.digest(signing_input), raw_to_asn1(signature, public_key))
          public_key.verify(digest, raw_to_asn1(signature, public_key), signing_input)
        rescue OpenSSL::PKey::PKeyError
          raise JWT::VerificationError, 'Signature verification raised'
        end
      end
    end
  end
end
kares commented 4 months ago

Thanks Matt, for sharing the details.

The EC class as is was unfinished, written for a specific use-case some time ago and the internals where very ... internal!

My "employer" actually uses ruby-jwt on JRuby but there was a patch due the missing dsa_verify_asn1 + it's an older version the app is currently locked into.

Please do open new tickets if you find more issues... :pray:

mattys101 commented 4 months ago

No worries. Thanks for cleaning up the implementation and making it compatible. Will let you know in a new ticket if we run into any trouble when we switch over to the updated version.