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

public_key:pkix_path_validation/3 allows duplicate root certificate #6394

Closed
tanguilp opened this issue Oct 26, 2022 · 4 comments
Closed

public_key:pkix_path_validation/3 allows duplicate root certificate #6394

tanguilp opened this issue Oct 26, 2022 · 4 comments
Assignees
Labels
bug Issue is reported as a bug in progress priority:low team:PS Assigned to OTP team PS
Milestone

Comments

@tanguilp
Copy link

tanguilp commented Oct 26, 2022

Describe the bug

public_key:pkix_path_validation/3 returns {ok, _} when the second argument contains duplicate root certificates, but should return an error according to RFC5280 - 6.1. Basic Path Validation:

To meet this goal, the path validation process verifies, among other
things, that a prospective certification path (a sequence of n
certificates) satisfies the following conditions:

  (a)  for all x in {1, ..., n-1}, the subject of certificate x is
       the issuer of certificate x+1;

  (b)  certificate 1 is issued by the trust anchor;

To Reproduce

Using for instance the Elixir X509 library:

> ca_key = X509.PrivateKey.new_ec(:secp256r1)
> ca = X509.Certificate.self_signed(ca_key, "/C=US/ST=CA/L=San Francisco/O=Acme/CN=ECDSA Root CA", template: :root_ca)
> server_key = X509.PrivateKey.new_ec(:secp256r1)
> server_cert = server_key |> X509.PublicKey.derive() |> X509.Certificate.new("/C=US/ST=CA/L=San Francisco/O=Acme/CN=Sample", ca, ca_key, extensions: [subject_alt_name: X509.Certificate.Extension.subject_alt_name(["example.org", "www.example.org"])])
> :public_key.pkix_path_validation(ca, [ca, ca, server_cert], [])
{:ok,                                        
 {{{1, 2, 840, 10045, 2, 1},
   {:ECPoint,
    <<4, 43, 203, 52, 9, 168, 79, 54, 69, 0, 250, 93, 45, 205, 127, 127, 16, 11,
      32, 110, 84, 170, 237, 245, 108, 19, 60, 213, 91, 7, 158, 162, 231, 37,
      211, 126, 141, 24, 56, 203, 104, 170, 238, ...>>},
   {:namedCurve, {1, 2, 840, 10045, 3, 1, 7}}},
  {:policy_tree_node, {2, 5, 29, 32, 0}, [], false, [{2, 5, 29, 32, 0}]}}}

Expected behavior

An {error, _} error should be returned.

Affected versions

OTP25.1.2 at least

Additional context

Not sure if this is a bug. The WebAuthn/FIDO2 test suite has tests that fail with OTP because of this.

@tanguilp tanguilp added the bug Issue is reported as a bug label Oct 26, 2022
@u3s u3s added the team:PS Assigned to OTP team PS label Oct 28, 2022
@IngelaAndin
Copy link
Contributor

IngelaAndin commented Nov 14, 2022

6.1. Basic Path Validation (of that same RFC) you can also find the following.

 A certificate is self-issued if the same DN appears in the subject
   and issuer fields (the two DNs are the same if they match according
   to the rules specified in Section 7.1).  In general, the issuer and
   subject of the certificates that make up a path are different for
   each certificate.  However, a CA may issue a certificate to itself to
   support key rollover or changes in certificate policies.  These
   self-issued certificates are not counted when evaluating path length
   or name constraints.

Now, it might not make sense to just duplicate the root cert but because of the above, I think it will not be detected. The question is if this causes some actual problem?!

@IngelaAndin IngelaAndin self-assigned this Nov 14, 2022
@IngelaAndin
Copy link
Contributor

@tanguilp ?

@IngelaAndin IngelaAndin added the waiting waiting for changes/input from author label Nov 22, 2022
@tanguilp
Copy link
Author

My bad I quoted the wrong part of the RFC, the relevant part is just below:

A certificate MUST NOT appear more than once in a prospective
certification path.

Therefore the current OTP implementation unfortunately is not perfectly conformant with RFC5280. I came upon this issue while implementing WebAuthn (see this issue related to the WebAuthn test suite). I had to write code to bypass this issue, which could be error prone and could have negative security consequences.

However I don't know why this restriction was set in the first place and if that's a security issue (except for people bypassing it incorrectly).

@IngelaAndin IngelaAndin removed the waiting waiting for changes/input from author label Dec 5, 2022
@IngelaAndin
Copy link
Contributor

Well the ROOT-CA should not be part of the path to be validated at all. But maybe this could happen in the middle of the chain too?! I can only see that it is a problem for self-signed certs, we can look into making public_key verify this property.

@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Feb 15, 2023
IngelaAndin added a commit to IngelaAndin/otp that referenced this issue Aug 16, 2023
@IngelaAndin IngelaAndin removed the stalled waiting for input by the Erlang/OTP team label Aug 16, 2023
@IngelaAndin IngelaAndin added this to the OTP-26.1 milestone Aug 16, 2023
IngelaAndin added a commit that referenced this issue Aug 17, 2023
…_certs/GH-6394/OTP-18723

public_key: Add check for duplicate certificates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug in progress priority:low team:PS Assigned to OTP team PS
Projects
None yet
Development

No branches or pull requests

4 participants