ssl: with verify_none, accept critical extensions

When establishing a TLS connection with {verify, verify_none}, if the
server has a certificate with a critical extension, for example a
"Netscape Cert Type" extension, certificate verification would fail,
which is surprising given that the name of the option suggests that no
verification would be performed.

With this change, certificate extensions marked as critical are
ignored when using verify_none.

3 files changed, 131 insertions, 2 deletions

diff --git a/lib/ssl/doc/src/ssl.xml b/lib/ssl/doc/src/ssl.xml
index bf87644116..ca5d2afc24 100644
--- a/lib/ssl/doc/src/ssl.xml
+++ b/lib/ssl/doc/src/ssl.xml
@@ -269,7 +269,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>
@@ -291,6 +295,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 6551308935..401ee63908 100644
--- a/lib/ssl/src/ssl.erl
+++ b/lib/ssl/src/ssl.erl
@@ -1280,6 +1280,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/test/ssl_certificate_verify_SUITE.erl b/lib/ssl/test/ssl_certificate_verify_SUITE.erl
index 5940a86a7f..123a7f4cbf 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,
@@ -794,6 +796,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."}].