ERL-686: SSL certfile stored in db when it does not validate

[OTP-Maintainer]

Original reporter: dcorbacho
Affected version: OTP-21.0
Fixed in version: OTP-21.1
Component: ssl
Migrated from: https://bugs.erlang.org/browse/ERL-686

---

I'm using OTP/master to verify that https://bugs.erlang.org/browse/ERL-664 solves the issued detected with RabbitMQ. The testcase provides by essen works now, but there are two outstanding issues:

1. Reported in https://bugs.erlang.org/browse/ERL-685

2. While investigating the first bug, I noticed that after the first connection attempt with an empty certificate the connection will proceed a bit further with the handshake and then close (as expected).  Debugging it found that the first time the verification of the certificates fails:

{code}
(rabbit@mars)2> (<0.256.0>) call ssl_pkix_db:add_trusted_certs(<0.585.0>,<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,[#Ref<0.3478279164.1433010178.229479>,
 {#Ref<0.3478279164.1433010178.229480>,#Ref<0.3478279164.1433010178.229481>},
 ssl_pem_cache,
 {#Ref<0.3478279164.1433010178.229482>,#Ref<0.3478279164.1433010178.229483>}])
(<0.256.0>) call ssl_pkix_db:lookup(<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,#Ref<0.3478279164.1433010178.229481>)
(<0.256.0>) returned from ssl_pkix_db:lookup/2 -> undefined
(<0.256.0>) call ssl_pkix_db:new_trusted_cert_entry(<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,[#Ref<0.3478279164.1433010178.229479>,
 {#Ref<0.3478279164.1433010178.229480>,#Ref<0.3478279164.1433010178.229481>},
 ssl_pem_cache,
 {#Ref<0.3478279164.1433010178.229482>,#Ref<0.3478279164.1433010178.229483>}])
(<0.256.0>) call ssl_pkix_db:init_ref_db(#Ref<0.3478279164.1432879106.231103>,<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,{#Ref<0.3478279164.1433010178.229480>,#Ref<0.3478279164.1433010178.229481>})
(<0.256.0>) returned from ssl_pkix_db:init_ref_db/3 -> true
(<0.256.0>) exception_from {ssl_pkix_db,new_trusted_cert_entry,2} {error,{badmatch,{error,enoent}}}
(<0.256.0>) exception_from {ssl_pkix_db,add_trusted_certs,3} {error,{badmatch,{error,enoent}}}
{code}
but succeeds for any later call:
{code}
(rabbit@mars)2> (<0.256.0>) call ssl_pkix_db:add_trusted_certs(<0.586.0>,<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,[#Ref<0.34
78279164.1433010178.229479>,
 {#Ref<0.3478279164.1433010178.229480>,#Ref<0.3478279164.1433010178.229481>},
 ssl_pem_cache,
 {#Ref<0.3478279164.1433010178.229482>,#Ref<0.3478279164.1433010178.229483>}])
(<0.256.0>) call ssl_pkix_db:lookup(<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,#Ref<0.3478279164.1433010178.229481>)
(<0.256.0>) call ssl_pkix_db:'-lookup/2-lc$^1/1-0-'([{<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,
  #Ref<0.3478279164.1432879106.231103>}],#Fun<ssl_pkix_db.1.62886622>)
(<0.256.0>) call ssl_pkix_db:'-lookup/2-fun-0-'({<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/testca/cacert.pem">>,
 #Ref<0.3478279164.1432879106.231103>})
(<0.256.0>) returned from ssl_pkix_db:'-lookup/2-fun-0-'/1 -> #Ref<0.3478279164.1432879106.231103>
(<0.256.0>) call ssl_pkix_db:'-lookup/2-lc$^1/1-0-'([],#Fun<ssl_pkix_db.1.62886622>)
(<0.256.0>) returned from ssl_pkix_db:'-lookup/2-lc$^1/1-0-'/2 -> []
(<0.256.0>) returned from ssl_pkix_db:'-lookup/2-lc$^1/1-0-'/2 -> [#Ref<0.3478279164.1432879106.231103>]
(<0.256.0>) returned from ssl_pkix_db:lookup/2 -> [#Ref<0.3478279164.1432879106.231103>]
(<0.256.0>) call ssl_pkix_db:ref_count(#Ref<0.3478279164.1432879106.231103>,#Ref<0.3478279164.1433010178.229480>,1)
(<0.256.0>) returned from ssl_pkix_db:ref_count/3 -> 2
(<0.256.0>) returned from ssl_pkix_db:add_trusted_certs/3 -> {ok,
                                                              #Ref<0.3478279164.1432879106.231103>}
(<0.586.0>) call ssl_pkix_db:lookup(<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/server/cert.pem">>,ssl_pem_cache)
(<0.586.0>) returned from ssl_pkix_db:lookup/2 -> undefined
(<0.586.0>) call ssl_pkix_db:lookup(<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/server/key.pem">>,ssl_pem_cache)
(<0.586.0>) returned from ssl_pkix_db:lookup/2 -> undefined
(<0.255.0>) call ssl_pkix_db:insert(<<"/Users/dparracorbacho/dev/umbrella-master/deps/rabbitmq_server_release/server/cert.pem">>,[{'Certificate',<<48,130,2,221,48,130,1,197,160,3
{code}

The first time _ssl_pem_cache:insert_ returns {error, enoent} https://github.com/erlang/otp/blob/master/lib/ssl/src/ssl_pkix_db.erl#L322, but the cert has already been stored. So the second time that _ssl_pkix_db:add_trusted_certs/3_ is called with the same invalid certificate, the lookup https://github.com/erlang/otp/blob/master/lib/ssl/src/ssl_pkix_db.erl#L139 will succeed.

[OTP-Maintainer]

ingela said:

I propose the following patch:

{code}
diff --git a/lib/ssl/src/ssl_manager.erl b/lib/ssl/src/ssl_manager.erl
index f44fe6a..52aa164 100644
--- a/lib/ssl/src/ssl_manager.erl
+++ b/lib/ssl/src/ssl_manager.erl
@@ -127,7 +127,13 @@ cache_pem_file(File, DbHandle) ->
 	[Content]  ->
 	    {ok, Content};
 	undefined ->
-            ssl_pem_cache:insert(File)
+            case ssl_pkix_db:decode_pem_file(File) of
+                {ok, Content} ->
+                    ssl_pem_cache:insert(File, Content),
+                    {ok, Content};
+                Error ->
+                    Error
+            end
     end.
 
 %%--------------------------------------------------------------------
diff --git a/lib/ssl/src/ssl_pem_cache.erl b/lib/ssl/src/ssl_pem_cache.erl
index 115ab44..a952e20 100644
--- a/lib/ssl/src/ssl_pem_cache.erl
+++ b/lib/ssl/src/ssl_pem_cache.erl
@@ -29,7 +29,7 @@
 -export([start_link/1, 
 	 start_link_dist/1,
 	 name/1,
-	 insert/1,
+	 insert/2,
 	 clear/0]).
 
 % Spawn export
@@ -90,19 +90,17 @@ start_link_dist(_) ->
 
 
 %%--------------------------------------------------------------------
--spec insert(binary()) -> {ok, term()} | {error, reason()}.
+-spec insert(binary(), term()) -> ok | {error, reason()}.
 %%		    
 %% Description: Cache a pem file and return its content.
 %%--------------------------------------------------------------------
-insert(File) ->    
-    {ok, PemBin} = file:read_file(File),
-    Content = public_key:pem_decode(PemBin),
+insert(File, Content) ->    
     case bypass_cache() of
 	true ->
-	    {ok, Content};
+	    ok;
 	false ->
 	    cast({cache_pem, File, Content}),
-	    {ok, Content}
+            ok
     end.
 
 %%--------------------------------------------------------------------
diff --git a/lib/ssl/src/ssl_pkix_db.erl b/lib/ssl/src/ssl_pkix_db.erl
index 8828c3a..56efd93 100644
--- a/lib/ssl/src/ssl_pkix_db.erl
+++ b/lib/ssl/src/ssl_pkix_db.erl
@@ -316,11 +316,16 @@ decode_certs(Ref, Cert) ->
     end.
 
 new_trusted_cert_entry(File, [CertsDb, RefsDb, _ | _]) ->
-    Ref = make_ref(),
-    init_ref_db(Ref, File, RefsDb),
-    {ok, Content} = ssl_pem_cache:insert(File),
-    add_certs_from_pem(Content, Ref, CertsDb),
-    {ok, Ref}.
+    case decode_pem_file(File) of
+        {ok, Content} ->
+            Ref = make_ref(),
+            init_ref_db(Ref, File, RefsDb),
+            ok = ssl_pem_cache:insert(File, Content),
+            add_certs_from_pem(Content, Ref, CertsDb),
+            {ok, Ref};
+        Error ->
+            Error
+    end.
 
 add_crls([_,_,_, {_, Mapping} | _], ?NO_DIST_POINT, CRLs) ->
     [add_crls(CRL, Mapping) || CRL <- CRLs];

{code}

[OTP-Maintainer]

dcorbacho said:

Thanks Ingela. I tested it and confirm that it works.

[OTP-Maintainer]

ingela said:

Thank you for testing. I will merge it tomorrow when my own test case has passed through our builds.