SSL Validation

[benmmurphy]

I'm fairly sure you have the same verification problem that just hit openssl.

hex/lib/hex/api.ex

Lines 59 to 81 in 8794c59

	defp partial_chain(certs) do
	certs = Enum.map(certs, &{&1, :public_key.pkix_decode_cert(&1, :otp)})
	cacerts = Hex.API.Certs.cacerts()
	cacerts = Enum.map(cacerts, &:public_key.pkix_decode_cert(&1, :otp))
	trusted =
	Enum.find_value(certs, fn {der, cert} ->
	trust? = Enum.any?(cacerts, fn cacert ->
	if :public_key.pkix_is_issuer(cert, cacert) do
	key = extract_key(cacert)
	:public_key.pkix_verify(der, key)
	end
	end)
	if trust?, do: der
	end)
	if trusted do
	{:trusted_ca, trusted}
	else
	:unknown_ca
	end
	end

Looks like it returns the first certificate in the chain that is trusted by a certificate in the trust store as trusted_ca. but, this certificate is not necessarily a ca certificate. i think you should be returning the issuer . I suspect a chain like this will lead to accepting bad certificates:

RANDOM CERT SIGNED BY ISSUER NOT IN TRUST STORE
|
V
VALID_CERT_SIGNED_BY_CERT_IN_TRUST_STORE (effectively treated as CA bit set)
|
V
EVIL CERTIFICATE

[ericmj]

Thank you for raising this issue, I have never been confident in the partial_chain implementation. Do you have a proposal implementation?

[ericmj]

I tried this but none of the ca certs seem to match anything in the chain:

defp partial_chain(certs) do
  cacerts = Hex.API.Certs.cacerts()

  trusted_cert =
    Enum.find_value(cacerts, fn cacert ->
      if cacert in certs, do: cacert
    end)

  if trusted_cert do
    {:trusted_ca, trusted_cert}
  else
    :unknown_ca
  end
end

[ericmj]

@benmmurphy Ping? I would very much like this fixed.

[benmmurphy]

i was thinking something like this (not tested. not even sure it compiles) to make sure the trusted_ca cert is one returned from the store. also, it is important to note that this breaks the use case where a client doesn't include the CA certificate which worked before. for example if your valid cert path is: CA->User Cert and you just send in 'User Cert' and no chain then before it would have set trusted_ca to the 'User Cert' and then in the chain rebuilding it would have found 'User Cert' == trusted_ca cert and returned the {TrustedCert, Chain} tuple as {'User Cert', []}. so this would have validated correctly but it would have incorrectly flagged 'User Cert' as being a trusted ca certificate. However, now it will set trusted_ca as the certificate that signed 'User Cert' and when it tries to rebuild the chain it won't be able to find a certificate in the chain which is identical to trusted_ca so it will fail to rebuild the chain with an unknown_ca error.

however, one problem with this implementation is it depends on the equality check when rebuilding the chain. if the cert in the store is not exactly equal with the cert in the chain then it won't work (which i think is maybe the problem you were having). i'm not sure if they can differ.

defp partial_chain(certs) do
  certs = Enum.map(certs, &{&1, :public_key.pkix_decode_cert(&1, :otp)})
  cacerts = Hex.API.Certs.cacerts()
  cacerts = Enum.map(cacerts, &:public_key.pkix_decode_cert(&1, :otp))

  trusted =
    Enum.find_value(certs, fn {der, cert} ->
      Enum.find_value(cacerts, fn cacert ->
        if :public_key.pkix_is_issuer(cert, cacert) do
          key = extract_key(cacert)
          :public_key.pkix_verify(der, key)
          cacert
        end
        nil
      end)
    end)

  if trusted do
    {:trusted_ca, trusted}
  else
    :unknown_ca
  end
end

also, looking at your fix i think the following would be a better solution than my first thoughts assuming no equality problem: (EDIT: just realized it is basically the same code as your proposed solution.)

defp partial_chain(certs) do
  cacerts = Enum.map(Hex.API.Certs, &:public_key.pkix_decode_cert(&1, :otp))
  certs = Enum.map(certs, &{&1, :public_key.pkix_decode_cert(&1, :otp)})

  trusted_cert =
    Enum.find_value(certs, fn cert ->
      if cert in cacerts, do: cert
    end)

  if trusted_cert do
    {:trusted_ca, trusted_cert}
  else
    :unknown_ca
  end
end

you can have a look at how erlang uses the partial chain function here;

https://github.com/erlang/otp/blob/maint/lib/ssl/src/ssl_certificate.erl#L305

i'll update the issue once i have a proper tested solution

it is also interesting the the otp code handles the case where the issuer

[ericmj]

Yes, the issue seems to be this check https://github.com/erlang/otp/blob/maint/lib/ssl/src/ssl_certificate.erl#L315-L320. It only allows you to return a certificate in the given chain.

The equality persists even when comparing OTP records it seems.

[benmmurphy]

i'll try and confirm that this is an actual security issue. if this is an actual issue then it limits the usefulness of the partial chain function a lot.

[ericmj]

Thank you for investing time in this @benmmurphy. I appreciate the help.

[benmmurphy]

yeah. i'm definitely sure this is an issue now. i have a test case here that illustrates it:

master...benmmurphy:invalid_ca_bit

[benmmurphy]

i've updated the code to fix the effective ca bit problem but i may have introduced other security bugs. i believe it is significantly better than the current code tho.

master...benmmurphy:invalid_ca_bit

---

so the previous partial chain code effectively
set the CA bit on any certificate that was signed
by a certificate in the ca trust store.

we fix this by making sure the trusted_ca return
result is an actual ca certificate.

so erlang partial_chain requires the trusted_ca
you return to be identical to a cert already
in the chain. in an ideal world we would just
like to replace the chain and return a cert
that we consider a trusted ca. some servers
like s3.amazonaws.com return a root cert (but used
to be an intermediate cert?) in the chain and
erlang does not like this. so partial_chain is
meant to return this root cert as trusted_ca but
how is it possible to identify this as the root
cert? the cert in the trust store is not the same
as the cert the server gives. because the trust
store cert is self-signed and server cert is
signed by the CA that is no longer in the trust
store. so we just check if the public key is the
same.

it is important to note that this creates
another security problem because a CA can serve up
a root cert with arbitrary extensions. for example
maybe a root cert has naming constraints but
by serving up a root cert without naming
constraints it is possible to subvert the naming
constraint policy. i'm not sure if erlang enforces
naming constraints on root cert so i'm not sure if
this introduces a vulnerability. also, there might
be problems with validity period and other
attributes on the certificate.

[ericmj]

Thanks for the code and the thorough explanation. We should go with this for now since it's a definitive improvement and I will ask the OTP team for guidance about properly implementing partial_chain or if it should be changed to allow changing the chain as you suggested.

I applied your patch but the tests are not running, I needed to change validate_cert_tests.exs to validate_cert_test.exs. But now I get undefined function: :ssl_manager.connection_init/2 errors, it also seems meck is not used? Would you like to send a PR?

[benmmurphy]

i've added a pull here: #111