Merge branch 'maint'

5 files changed, 201 insertions, 23 deletions

diff --git a/lib/ssl/doc/src/ssl.xml b/lib/ssl/doc/src/ssl.xml
index d3881ad117..a76d46ee9b 100644
--- a/lib/ssl/doc/src/ssl.xml
+++ b/lib/ssl/doc/src/ssl.xml
@@ -271,7 +271,11 @@ atom()}} |
 	  terminate regarding verification failures and the connection is
 	  established.</p></item>
 	  <item><p>If called with an extension unknown to the user application,
-	  return value <c>{unknown, UserState}</c> is to be used.</p></item>
+	  return value <c>{unknown, UserState}</c> is to be used.</p>
+
+	  <p>Note that if the fun returns <c>unknown</c> for an extension marked
+	  as critical, validation will fail.</p>
+	  </item>
 	</list>
 
 	<p>Default option <c>verify_fun</c> in <c>verify_peer mode</c>:</p>
@@ -293,6 +297,8 @@ atom()}} |
        <code>
 {fun(_,{bad_cert, _}, UserState) ->
 	 {valid, UserState};
+    (_,{extension, #'Extension'{critical = true}}, UserState) ->
+	 {valid, UserState};
     (_,{extension, _}, UserState) ->
 	 {unknown, UserState};
     (_, valid, UserState) ->
diff --git a/lib/ssl/src/ssl.erl b/lib/ssl/src/ssl.erl
index c1bc90559e..3afc3a5e87 100644
--- a/lib/ssl/src/ssl.erl
+++ b/lib/ssl/src/ssl.erl
@@ -1296,6 +1296,12 @@ handle_verify_options(Opts, CaCerts) ->
     DefaultVerifyNoneFun =
 	{fun(_,{bad_cert, _}, UserState) ->
 		 {valid, UserState};
+	    (_,{extension, #'Extension'{critical = true}}, UserState) ->
+		 %% This extension is marked as critical, so
+		 %% certificate verification should fail if we don't
+		 %% understand the extension.  However, this is
+		 %% `verify_none', so let's accept it anyway.
+		 {valid, UserState};
 	    (_,{extension, _}, UserState) ->
 		 {unknown, UserState};
 	    (_, valid, UserState) ->
diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl
index e9e140836b..e98073080a 100644
--- a/lib/ssl/src/ssl_handshake.erl
+++ b/lib/ssl/src/ssl_handshake.erl
@@ -2072,12 +2072,9 @@ crl_check(OtpCert, Check, CertDbHandle, CertDbRef, {Callback, CRLDbHandle}, _) -
 	      ],
     case dps_and_crls(OtpCert, Callback, CRLDbHandle, ext) of
 	no_dps ->
-	    case dps_and_crls(OtpCert, Callback, CRLDbHandle, same_issuer) of
-		[] ->
-		    valid; %% No relevant CRL existed
-		DpsAndCRls ->
-		    crl_check_same_issuer(OtpCert, Check, DpsAndCRls, Options)		
-	    end;
+	    crl_check_same_issuer(OtpCert, Check,
+				  dps_and_crls(OtpCert, Callback, CRLDbHandle, same_issuer),
+				  Options);
 	DpsAndCRLs ->  %% This DP list may be empty if relevant CRLs existed 
 	    %% but could not be retrived, will result in {bad_cert, revocation_status_undetermined}
 	    case public_key:pkix_crls_validate(OtpCert, DpsAndCRLs, Options) of
diff --git a/lib/ssl/test/ssl_certificate_verify_SUITE.erl b/lib/ssl/test/ssl_certificate_verify_SUITE.erl
index 968ef30791..d10506cb69 100644
--- a/lib/ssl/test/ssl_certificate_verify_SUITE.erl
+++ b/lib/ssl/test/ssl_certificate_verify_SUITE.erl
@@ -66,7 +66,9 @@ tests() ->
      invalid_signature_client,
      invalid_signature_server,
      extended_key_usage_verify_peer,
-     extended_key_usage_verify_none].
+     extended_key_usage_verify_none,
+     critical_extension_verify_peer,
+     critical_extension_verify_none].
 
 error_handling_tests()->
     [client_with_cert_cipher_suites_handshake,
@@ -795,6 +797,121 @@ extended_key_usage_verify_none(Config) when is_list(Config) ->
     ssl_test_lib:close(Client).
 
 %%--------------------------------------------------------------------
+critical_extension_verify_peer() ->
+    [{doc,"Test cert that has a critical unknown extension in verify_peer mode"}].
+
+critical_extension_verify_peer(Config) when is_list(Config) ->
+    ClientOpts = ?config(client_verification_opts, Config),
+    ServerOpts = ?config(server_verification_opts, Config),
+    PrivDir = ?config(priv_dir, Config),
+    Active = ?config(active, Config),
+    ReceiveFunction =  ?config(receive_function, Config),
+
+    KeyFile = filename:join(PrivDir, "otpCA/private/key.pem"),
+    NewCertName = integer_to_list(erlang:unique_integer()) ++ ".pem",
+
+    ServerCertFile = proplists:get_value(certfile, ServerOpts),
+    NewServerCertFile = filename:join([PrivDir, "server", NewCertName]),
+    add_critical_netscape_cert_type(ServerCertFile, NewServerCertFile, KeyFile),
+    NewServerOpts = [{certfile, NewServerCertFile} | proplists:delete(certfile, ServerOpts)],
+
+    ClientCertFile = proplists:get_value(certfile, ClientOpts),
+    NewClientCertFile = filename:join([PrivDir, "client", NewCertName]),
+    add_critical_netscape_cert_type(ClientCertFile, NewClientCertFile, KeyFile),
+    NewClientOpts = [{certfile, NewClientCertFile} | proplists:delete(certfile, ClientOpts)],
+
+    {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
+
+    Server = ssl_test_lib:start_server_error(
+               [{node, ServerNode}, {port, 0},
+                {from, self()},
+                {mfa, {ssl_test_lib,  ReceiveFunction, []}},
+                {options, [{verify, verify_peer}, {active, Active} | NewServerOpts]}]),
+    Port = ssl_test_lib:inet_port(Server),
+    Client = ssl_test_lib:start_client_error(
+               [{node, ClientNode}, {port, Port},
+                {host, Hostname},
+                {from, self()},
+                {mfa, {ssl_test_lib, ReceiveFunction, []}},
+                {options, [{verify, verify_peer}, {active, Active} | NewClientOpts]}]),
+
+    %% This certificate has a critical extension that we don't
+    %% understand.  Therefore, verification should fail.
+    tcp_delivery_workaround(Server, {error, {tls_alert, "unsupported certificate"}},
+                            Client, {error, {tls_alert, "unsupported certificate"}}),
+
+    ssl_test_lib:close(Server),
+    ok.
+
+%%--------------------------------------------------------------------
+critical_extension_verify_none() ->
+    [{doc,"Test cert that has a critical unknown extension in verify_none mode"}].
+
+critical_extension_verify_none(Config) when is_list(Config) ->
+    ClientOpts = ?config(client_verification_opts, Config),
+    ServerOpts = ?config(server_verification_opts, Config),
+    PrivDir = ?config(priv_dir, Config),
+    Active = ?config(active, Config),
+    ReceiveFunction =  ?config(receive_function, Config),
+
+    KeyFile = filename:join(PrivDir, "otpCA/private/key.pem"),
+    NewCertName = integer_to_list(erlang:unique_integer()) ++ ".pem",
+
+    ServerCertFile = proplists:get_value(certfile, ServerOpts),
+    NewServerCertFile = filename:join([PrivDir, "server", NewCertName]),
+    add_critical_netscape_cert_type(ServerCertFile, NewServerCertFile, KeyFile),
+    NewServerOpts = [{certfile, NewServerCertFile} | proplists:delete(certfile, ServerOpts)],
+
+    ClientCertFile = proplists:get_value(certfile, ClientOpts),
+    NewClientCertFile = filename:join([PrivDir, "client", NewCertName]),
+    add_critical_netscape_cert_type(ClientCertFile, NewClientCertFile, KeyFile),
+    NewClientOpts = [{certfile, NewClientCertFile} | proplists:delete(certfile, ClientOpts)],
+
+    {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
+
+    Server = ssl_test_lib:start_server(
+               [{node, ServerNode}, {port, 0},
+                {from, self()},
+                {mfa, {ssl_test_lib, ReceiveFunction, []}},
+                {options, [{verify, verify_none}, {active, Active} | NewServerOpts]}]),
+    Port = ssl_test_lib:inet_port(Server),
+    Client = ssl_test_lib:start_client(
+               [{node, ClientNode}, {port, Port},
+                {host, Hostname},
+                {from, self()},
+                {mfa, {ssl_test_lib, ReceiveFunction, []}},
+                {options, [{verify, verify_none}, {active, Active} | NewClientOpts]}]),
+
+    %% This certificate has a critical extension that we don't
+    %% understand.  But we're using `verify_none', so verification
+    %% shouldn't fail.
+    ssl_test_lib:check_result(Server, ok, Client, ok),
+
+    ssl_test_lib:close(Server),
+    ssl_test_lib:close(Client),
+    ok.
+
+add_critical_netscape_cert_type(CertFile, NewCertFile, KeyFile) ->
+    [KeyEntry] = ssl_test_lib:pem_to_der(KeyFile),
+    Key = ssl_test_lib:public_key(public_key:pem_entry_decode(KeyEntry)),
+
+    [{'Certificate', DerCert, _}] = ssl_test_lib:pem_to_der(CertFile),
+    OTPCert = public_key:pkix_decode_cert(DerCert, otp),
+    %% This is the "Netscape Cert Type" extension, telling us that the
+    %% certificate can be used for SSL clients and SSL servers.
+    NetscapeCertTypeExt = #'Extension'{
+                             extnID = {2,16,840,1,113730,1,1},
+                             critical = true,
+                             extnValue = <<3,2,6,192>>},
+    OTPTbsCert = OTPCert#'OTPCertificate'.tbsCertificate,
+    Extensions =  OTPTbsCert#'OTPTBSCertificate'.extensions,
+    NewOTPTbsCert = OTPTbsCert#'OTPTBSCertificate'{
+                      extensions = [NetscapeCertTypeExt] ++ Extensions},
+    NewDerCert = public_key:pkix_sign(NewOTPTbsCert, Key),
+    ssl_test_lib:der_to_pem(NewCertFile, [{'Certificate', NewDerCert, not_encrypted}]),
+    ok.
+
+%%--------------------------------------------------------------------
 no_authority_key_identifier() ->
     [{doc, "Test cert that does not have authorityKeyIdentifier extension"
       " but are present in trusted certs db."}].
diff --git a/lib/ssl/test/ssl_crl_SUITE.erl b/lib/ssl/test/ssl_crl_SUITE.erl
index 44580be1ff..5b86027210 100644
--- a/lib/ssl/test/ssl_crl_SUITE.erl
+++ b/lib/ssl/test/ssl_crl_SUITE.erl
@@ -53,7 +53,7 @@ groups() ->
      {idp_crl, [], basic_tests()}].
 
 basic_tests() ->
-    [crl_verify_valid, crl_verify_revoked].
+    [crl_verify_valid, crl_verify_revoked, crl_verify_no_crl].
 
 
 init_per_suite(Config) ->
@@ -186,11 +186,6 @@ crl_verify_revoked(Config)  when is_list(Config) ->
 
     {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
 
-    Server = ssl_test_lib:start_server_error([{node, ServerNode}, {port, 0}, 
-					      {from, self()}, 
-					      {options, ServerOpts}]),
-    Port = ssl_test_lib:inet_port(Server),
-    
     ssl_crl_cache:insert({file, filename:join([PrivDir, "erlangCA", "crl.pem"])}),
     ssl_crl_cache:insert({file, filename:join([PrivDir, "otpCA", "crl.pem"])}),
     
@@ -206,16 +201,55 @@ crl_verify_revoked(Config)  when is_list(Config) ->
 			   {verify, verify_peer}]
 		  end,	
     
-    Client = ssl_test_lib:start_client_error([{node, ClientNode}, {port, Port}, 
-     					      {host, Hostname},
-   					      {from, self()}, 
-     					      {options, ClientOpts}]),
-    receive
-	{Server, AlertOrColse} ->
-	    ct:pal("Server Alert or Close ~p", [AlertOrColse])
-    end,	    
-    ssl_test_lib:check_result(Client, {error, {tls_alert, "certificate revoked"}}).
+    crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts,
+                     "certificate revoked").
 
+crl_verify_no_crl() ->
+    [{doc,"Verify a simple CRL chain when the CRL is missing"}].
+crl_verify_no_crl(Config) when is_list(Config) ->
+    PrivDir = ?config(cert_dir, Config),
+    Check = ?config(crl_check, Config),
+    ServerOpts =  [{keyfile, filename:join([PrivDir, "server", "key.pem"])},
+      		  {certfile, filename:join([PrivDir, "server", "cert.pem"])},
+		   {cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])}],
+    ClientOpts =  case ?config(idp_crl, Config) of 
+		      true ->	       
+			  [{cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])},
+			   {crl_check, Check},
+			   {crl_cache, {ssl_crl_cache, {internal, [{http, 5000}]}}},
+			   {verify, verify_peer}];
+		      false ->
+			  [{cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])},
+			   {crl_check, Check},
+			   {verify, verify_peer}]
+		  end,			  
+    {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
+
+    %% In case we're running an HTTP server that serves CRLs, let's
+    %% rename those files, so the CRL is absent when we try to verify
+    %% it.
+    %%
+    %% If we're not using an HTTP server, we just need to refrain from
+    %% adding the CRLs to the cache manually.
+    rename_crl(filename:join([PrivDir, "erlangCA", "crl.pem"])),
+    rename_crl(filename:join([PrivDir, "otpCA", "crl.pem"])),
+
+    %% The expected outcome when the CRL is missing depends on the
+    %% crl_check setting.
+    case Check of
+        true ->
+            %% The error "revocation status undetermined" gets turned
+            %% into "bad certificate".
+            crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts,
+                             "bad certificate");
+        peer ->
+            crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts,
+                             "bad certificate");
+        best_effort ->
+            %% In "best effort" mode, we consider the certificate not
+            %% to be revoked if we can't find the appropriate CRL.
+            crl_verify_valid(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts)
+    end.
 
 crl_verify_valid(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts) ->
     Server = ssl_test_lib:start_server([{node, ServerNode}, {port, 0}, 
@@ -236,6 +270,22 @@ crl_verify_valid(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts) ->
     ssl_test_lib:close(Server),
     ssl_test_lib:close(Client).
 
+crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts, ExpectedAlert) ->
+    Server = ssl_test_lib:start_server_error([{node, ServerNode}, {port, 0},
+					      {from, self()},
+					      {options, ServerOpts}]),
+    Port = ssl_test_lib:inet_port(Server),
+
+    Client = ssl_test_lib:start_client_error([{node, ClientNode}, {port, Port},
+					      {host, Hostname},
+					      {from, self()},
+					      {options, ClientOpts}]),
+    receive
+	{Server, AlertOrClose} ->
+	    ct:pal("Server Alert or Close ~p", [AlertOrClose])
+    end,
+    ssl_test_lib:check_result(Client, {error, {tls_alert, ExpectedAlert}}).
+
 %%--------------------------------------------------------------------
 %% Internal functions ------------------------------------------------
 %%--------------------------------------------------------------------
@@ -259,3 +309,5 @@ make_dir_path(PathComponents) ->
 		"",
 		PathComponents).
 
+rename_crl(Filename) ->
+    file:rename(Filename, Filename ++ ".notfound").