ERL-1030: SSL failing to handle a 'cRLIssuer' entry in a CRL distribution point extension

[OTP-Maintainer]

Original reporter: tim gleeson Affected version: Not Specified Fixed in version: OTP-20.3 Component: ssl Migrated from: https://bugs.erlang.org/browse/ERL-1030

---

OTP19.3.3

Attempting an SSL connection with a cert containing a CRL distribution point extension like that below:

{noformat}
[{'DistributionPoint',
     {fullName,[{uniformResourceIdentifier,"http://blah.com/BLAH/certdist"}]},
     asn1_NOVALUE,
     [{directoryName,
       {rdnSequence,[[{'AttributeTypeAndValue',{2,5,4,3},<<12,6,65,65,65,65,65,65>>}],[{'AttributeTypeAndValue',{2,5,4,10},<<12,3,65,65,65>>}]]}
      }
     ]
 }
]

{noformat}

i.e. with a non-asn1_NOVALUE cRLIssuer.

It requires a little instrumentation to get the stacktrace for the
exception. The top of it is:

{noformat}
    {public_key,short_name_hash,[[{directoryName,{rdnSequence,[[{'AttributeTypeAndValue',{2,5,4,3},<<12,6,65,65,65,65,65,65>>}],[{'AttributeTypeAndValue',{2,5,4,10},<<12,3,65,65,65>>}]]}}]],
                                       [{file,"public_key.erl"},{line,956}]},
    {ssl_crl_hash_dir,find_crls,2,     [{file,"ssl_crl_hash_dir.erl"},{line,69}]},
    {ssl_crl_hash_dir,select,2,        [{file,"ssl_crl_hash_dir.erl"},{line,49}]},
    {ssl_crl_hash_dir,lookup,3,        [{file,"ssl_crl_hash_dir.erl"},{line,40}]},
    {ssl_handshake,distpoints_lookup,4,[{file,"ssl_handshake.erl"},{line,2276}]},
    {ssl_handshake,dps_and_crls,4,     [{file,"ssl_handshake.erl"},{line,2251}]},
    {ssl_handshake,crl_check,7,        [{file,"ssl_handshake.erl"},{line,2219}]},

{noformat}

The root cause of the problem seems to be in ssl_crl_hash_dir:lookup/3
which effectively has to choose between two similar looking 'Issuer'
entities: 'CertIssuer' is a singleton, but 'CRLIssuer' is potentially
a list, but both are treated as if singletons.

I think this selection is as required by [RFC5280,6.3.3(b)1], roughly:

{noformat}
  if   the DP includes cRLIssuer
  then verify that the issuer field in the complete CRL matches
       cRLIssuer in the DP
       and
       that the complete CRL contains an issuing distribution point
       extension with the indirectCRL boolean asserted
  else verify that the CRL issuer matches the certificate issuer
{noformat}

Perhaps only the 'else' branch has been exercised until now?

Investigatory patch
-------------------

{noformat}
diff --git a/lib/public_key/src/pubkey_crl.erl b/lib/public_key/src/pubkey_crl.erl
index 33bef91..642d109 100644
--- a/lib/public_key/src/pubkey_crl.erl
+++ b/lib/public_key/src/pubkey_crl.erl
@@ -23,7 +23,7 @@
 -include("public_key.hrl").

 -export([validate/7, init_revokation_state/0, fresh_crl/3, verify_crl_signature/4,
-    is_delta_crl/1, combines/2, match_one/2]).
+    is_delta_crl/1, combines/2, match_one/2, dp_crlissuer_to_issuer/1]).

 -record(userstate, {dpcrls,
            idp
@@ -333,6 +333,7 @@ verify_issuer_and_scope(#'OTPCertificate'{tbsCertificate = TBSCert}= Cert,
     end.

 dp_crlissuer_to_issuer(DPCRLIssuer) ->
+     %% Assume the cRLIssuer SEQUENCE is of length exactly 1
      [{directoryName, Issuer}] = pubkey_cert_records:transform(DPCRLIssuer, decode),
      Issuer.

diff --git a/lib/ssl/src/ssl_crl_hash_dir.erl b/lib/ssl/src/ssl_crl_hash_dir.erl
index bb62737..71f689d 100644
--- a/lib/ssl/src/ssl_crl_hash_dir.erl
+++ b/lib/ssl/src/ssl_crl_hash_dir.erl
@@ -33,7 +33,7 @@ lookup(#'DistributionPoint'{cRLIssuer = CRLIssuer} = DP, CertIssuer, CRLDbInfo)
        %% indicate a CRL issuer, use the certificate issuer.
        CertIssuer;
        _ ->
-       CRLIssuer
+       pubkey_crl:dp_crlissuer_to_issuer(CRLIssuer)
    end,
     %% Find all CRLs for this issuer, and return those that match the
     %% given distribution point.
{noformat}

However, this does make the assumption that cRLIssuer is a SEQUENCE of
length exactly 1, or asn1_NOVALUE. But, for better or worse, the
current usage of pubkey_crl:dp_crlissuer_to_issuer() already makes
that assumption.

I'm not steeped well enough in RFC5280 to guess what the implied
semantics of a list of len > 1 would be; the openssl implementation
does seem to handle a list of general length and the semantics appear
to be 'if any dp_issuer matches crl_issuer'.

Repro and test
--------------
I've looked through the OTP SSL tests for somewhere suitable to drop
in a case for this, but it's all rather new territory for me. I'm sure
the following is the wrong place, but it does seem to provoke the
issue.

{noformat}
diff --git a/release/tests/ssl_test/make_certs.erl b/release/tests/ssl_test/make_certs.erl
index 7f27f70..254108d 100644
--- a/release/tests/ssl_test/make_certs.erl
+++ b/release/tests/ssl_test/make_certs.erl
@@ -437,14 +436,20 @@ ca_cnf(Root, C = #config{issuing_distribution_point = true}) ->
      "authorityKeyIdentifier = keyid,issuer:always\n"
      "subjectAltName   = email:copy\n"
      "issuerAltName    = issuer:copy\n"
-     "crlDistributionPoints=@crl_section\n"
+     "crlDistributionPoints=crl_section\n"

      "[crl_section]\n"
      %% intentionally invalid
-     "URI.1=http://localhost/",C#config.commonName,"/crl.pem\n"
-     "URI.2=http://localhost:",integer_to_list(C#config.crl_port),"/",C#config.commonName,"/crl.pem\n"
+     "fullname=URI:http://localhost/",C#config.commonName,"/crl.pem\n"
+     "fullname=URI:http://localhost:",integer_to_list(C#config.crl_port),"/",C#config.commonName,"/crl.pem\n"
+     "CRLissuer=dirName:issuer_sect\n"
      "\n"

+     "[issuer_sect]\n"
+     "C=UK\n"
+     "O=Organisation\n"
+     "CN=DEF123\n"
+
      "[user_cert_digital_signature_only]\n"
      "basicConstraints = CA:false\n"
      "keyUsage         = digitalSignature\n"

{noformat}

I get:

{noformat}
  Testing tests.ssl_test.ssl_crl_SUITE.crl_hash_dir: TEST COMPLETE, 12 ok, 3 failed of 15 test cases
{noformat}

And the backtrace I get (I need to instrument
ssl_handshake.erl:certify catch to see it) is the same as shown at the
top.

Adding my investigatory means I don't get the indicative traceback,
suggesting my patch may be effective. But it still leaves two other
errors. I think I may be turning an 'intentionally invalid'
crl_section into something valid. I think maybe the testing should go
somewhere else, but I'm not sure where would be good.

[OTP-Maintainer]

ingela said:

I think you may be correct that the code path may not have been covered before.
The RFC 5280 is not one of the easiest to understand,  I think. 

The erl_make_certs part of the test frame work is one of the older parts the code,which creates OpenSSL configuration for generation of cert data. A problem with it is that it assumes all test wants more or less the same  test data. So it could be that your addition could brakes some other test.  For most test cases we no longer use this configuration but use our own public_key functions to generate test data. For CRLs however we have not had time to prioritize additions to public_key to be able to do so.  

One way would be to add some config value so that you only use your configuration for the new test case. 

You could also provide an OpenSSL config file in the ssl_crl_SUITE_data directory and then fetch it ( DataDir = proplists:get_value(data_dir, Config),
Conffile = filename:join(DataDir, "OpenSSLConf") )  and create your special input in init_per_tescase(yourtest, Config) ->  ...  it that feels easier. That  is probably how I would do it if I wrote test using OpenSSL to generate data from scratch.

[OTP-Maintainer]

ingela said:

It could be a good idea if you make your changes into a PR and we could cooperate to finish it to something "mergable"!

[OTP-Maintainer]

tim gleeson said:

I hope I'll have a little time next week to look at the testing path you suggested. And then I'll try working up a proper PR.

[OTP-Maintainer]

tim gleeson said:

Here's what I'm thinking of doing for testing. I'd appreciate
your shooting down anything which you think is mad...

When you talk about private_key testing, I assume you are talking
about the private_key module. I guess many (some?) of the
functions there can be 'unit tested' in a straightforward way
because they are fairly straightforward functions with limited
environment interaction. Is that in fact what's happening?

The crash I observed was in CRL file handling. I think I can't do
simple unit testing, because of the need to cover that path which
interacts with the file system. Is there a subset of SSL
functionality which I could test? I don't know the code well
enough, so I'm assuming I have to do a full 'system' test and
establish an SSL connection - as the other ssl_crl_SUITE tests
do.

If that's true, then I might as well use the
crl_verify_valid() (etc.) tests in the ssl_crl_SUITE, being
careful to have set up the environment correctly for them. I
think this means I will need to have pre-created:
    %   server/key.pem, server/cert.pem, server/cacerts.pem
    %   erlangCA/crl.pem
    %   otpCA/crl.pem

make_certs:all(), as you say, is doing too much and is
inflexible. So, I'm thinking of exploding that all() function into
my testcase (or testgroup) and writing a local enduser() function
to take an openssl config file from the datadir.

[OTP-Maintainer]

ingela said:

Well we use the erlang public_key:pkix_test_data/1  to create a lots of our configuration. It will return its data as DER blobs. But the ssl test code also uses other public_key functions to create PEM file format (public_key application does not handle files, see x509-test.erl in ssl test directory) and writes it to files in test case priv_dir  as to test that path of invocation.  However public_key API has no function to create CRLs. 

The problem with pre creating certs is that they might not always have a valid date when you try to run the test. 
We prefer to always create fresh data that shall pass, we do not know the life cycle of the code. I think you should be able to
use OpenSSL to generate all cert data, that is what make_certs does. Pre generated keys are ok. 

The public_key application has some own test of CRL based on  http://csrc.nist.gov/groups/ST/crypto_apps_infra/pki/pkitesting.html
We have had the problem with this suite, that the certs expired, and they made a new one released in a different way.

The ssl application uses the public_key application to do all pkix_path and CRL validation so it could be possible to add a test case to 
public_key instead of ssl but I am not sure it make the problem easier as generating the test data is the hard part.