Avoid errors with a priori inapplicable protocol bounds

The 'MinProtocol' and 'MaxProtocol' configuration commands now silently
ignore TLS protocol version bounds when configurign DTLS-based contexts,
and conversely, silently ignore DTLS protocol version bounds when
configuring TLS-based contexts.  The commands can be repeated to set
bounds of both types.  The same applies with the corresponding
"min_protocol" and "max_protocol" command-line switches, in case some
application uses both TLS and DTLS.

SSL_CTX instances that are created for a fixed protocol version (e.g.
TLSv1_server_method()) also silently ignore version bounds.  Previously
attempts to apply bounds to these protocol versions would result in an
error.  Now only the "version-flexible" SSL_CTX instances are subject to
limits in configuration files in command-line options.

Expected to resolve #12394

Reviewed-by: Paul Dale <paul.dale@oracle.com>
GH: #12472

5 files changed, 69 insertions, 24 deletions

diff --git a/CHANGES.md b/CHANGES.md
index 5ff188c18c..14694739ae 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -23,6 +23,22 @@ OpenSSL 3.0
 
 ### Changes between 1.1.1 and 3.0 [xx XXX xxxx]
 
+ * The 'MinProtocol' and 'MaxProtocol' configuration commands now silently
+   ignore TLS protocol version bounds when configuring DTLS-based contexts, and
+   conversely, silently ignore DTLS protocol version bounds when configuring
+   TLS-based contexts.  The commands can be repeated to set bounds of both
+   types.  The same applies with the corresponding "min_protocol" and
+   "max_protocol" command-line switches, in case some application uses both TLS
+   and DTLS.
+
+   SSL_CTX instances that are created for a fixed protocol version (e.g.
+   TLSv1_server_method()) also silently ignore version bounds.  Previously
+   attempts to apply bounds to these protocol versions would result in an
+   error.  Now only the "version-flexible" SSL_CTX instances are subject to
+   limits in configuration files in command-line options.
+
+   *Viktor Dukhovni*
+
  * Deprecated the `ENGINE` API.  Engines should be replaced with providers
    going forward.
 
diff --git a/doc/man3/SSL_CONF_cmd.pod b/doc/man3/SSL_CONF_cmd.pod
index 753d6778df..97ebff047f 100644
--- a/doc/man3/SSL_CONF_cmd.pod
+++ b/doc/man3/SSL_CONF_cmd.pod
@@ -178,12 +178,17 @@ See L<openssl-ciphers(1)> for more information.
 
 =item B<-min_protocol> I<minprot>, B<-max_protocol> I<maxprot>
 
-Sets the minimum and maximum supported protocol.  Currently supported
-protocol values are B<SSLv3>, B<TLSv1>, B<TLSv1.1>, B<TLSv1.2>, B<TLSv1.3>
-for TLS and B<DTLSv1>, B<DTLSv1.2> for DTLS, and B<None> for no limit.
-If either bound is not specified then only the other bound applies,
-if specified.  To restrict the supported protocol versions use these
-commands rather than the deprecated alternative commands below.
+Sets the minimum and maximum supported protocol.
+Currently supported protocol values are B<SSLv3>, B<TLSv1>, B<TLSv1.1>,
+B<TLSv1.2>, B<TLSv1.3> for TLS; B<DTLSv1>, B<DTLSv1.2> for DTLS, and B<None>
+for no limit.
+If either the lower or upper bound is not specified then only the other bound
+applies, if specified.
+If your application supports both TLS and DTLS you can specify any of these
+options twice, once with a bound for TLS and again with an appropriate bound
+for DTLS.
+To restrict the supported protocol versions use these commands rather than the
+deprecated alternative commands below.
 
 =item B<-record_padding> I<padding>
 
@@ -389,7 +394,11 @@ This sets the minimum supported SSL, TLS or DTLS version.
 
 Currently supported protocol values are B<SSLv3>, B<TLSv1>, B<TLSv1.1>,
 B<TLSv1.2>, B<TLSv1.3>, B<DTLSv1> and B<DTLSv1.2>.
-The value B<None> will disable the limit.
+The SSL and TLS bounds apply only to TLS-based contexts, while the DTLS bounds
+apply only to DTLS-based contexts.
+The command can be repeated with one instance setting a TLS bound, and the
+other setting a DTLS bound.
+The value B<None> applies to both types of contexts and disables the limits.
 
 =item B<MaxProtocol>
 
@@ -397,7 +406,11 @@ This sets the maximum supported SSL, TLS or DTLS version.
 
 Currently supported protocol values are B<SSLv3>, B<TLSv1>, B<TLSv1.1>,
 B<TLSv1.2>, B<TLSv1.3>, B<DTLSv1> and B<DTLSv1.2>.
-The value B<None> will disable the limit.
+The SSL and TLS bounds apply only to TLS-based contexts, while the DTLS bounds
+apply only to DTLS-based contexts.
+The command can be repeated with one instance setting a TLS bound, and the
+other setting a DTLS bound.
+The value B<None> applies to both types of contexts and disables the limits.
 
 =item B<Protocol>
 
diff --git a/doc/man5/config.pod b/doc/man5/config.pod
index 58948b4b78..2618cef588 100644
--- a/doc/man5/config.pod
+++ b/doc/man5/config.pod
@@ -299,10 +299,15 @@ section with the configuration for that name. For example:
 
 The configuration name B<system_default> has a special meaning.  If it
 exists, it is applied whenever an B<SSL_CTX> object is created.  For example,
-to impose a system-wide minimum on protocol version:
+to impose system-wide minimum TLS and DTLS protocol versions:
 
  [tls_system_default]
  MinProtocol = TLSv1.2
+ MinProtocol = DTLSv1.2
+
+The minimum TLS protocol is applied to B<SSL_CTX> objects that are TLS-based,
+and the minimum DTLS protocol to those are DTLS-based.
+The same applies also to maximum versions set with B<MaxProtocol>.
 
 Each configuration section consists of name/value pairs that are parsed
 by B<SSL_CONF_cmd(3)>, which will be called by SSL_CTX_config() or
diff --git a/ssl/ssl_conf.c b/ssl/ssl_conf.c
index aefe8ad203..fe9b8ec3ea 100644
--- a/ssl/ssl_conf.c
+++ b/ssl/ssl_conf.c
@@ -303,6 +303,13 @@ static int protocol_from_string(const char *value)
         const char *name;
         int version;
     };
+    /*
+     * Note: To avoid breaking previously valid configurations, we must retain
+     * legacy entries in this table even if the underlying protocol is no
+     * longer supported.  This also means that the constants SSL3_VERSION, ...
+     * need to be retained indefinitely.  This table can only grow, never
+     * shrink.
+     */
     static const struct protocol_versions versions[] = {
         {"None", 0},
         {"SSLv3", SSL3_VERSION},
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index de8212747f..d8aab20e92 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1679,11 +1679,22 @@ int ssl_check_version_downgrade(SSL *s)
  */
 int ssl_set_version_bound(int method_version, int version, int *bound)
 {
+    int valid_tls;
+    int valid_dtls;
+
     if (version == 0) {
         *bound = version;
         return 1;
     }
 
+    valid_tls = version >= SSL3_VERSION && version <= TLS_MAX_VERSION_INTERNAL;
+    valid_dtls =
+        DTLS_VERSION_LE(version, DTLS_MAX_VERSION_INTERNAL) &&
+        DTLS_VERSION_GE(version, DTLS1_BAD_VER);
+
+    if (!valid_tls && !valid_dtls)
+        return 0;
+
     /*-
      * Restrict TLS methods to TLS protocol versions.
      * Restrict DTLS methods to DTLS protocol versions.
@@ -1694,31 +1705,24 @@ int ssl_set_version_bound(int method_version, int version, int *bound)
      * configurations.  If the MIN (supported) version ever rises, the user's
      * "floor" remains valid even if no longer available.  We don't expect the
      * MAX ceiling to ever get lower, so making that variable makes sense.
+     *
+     * We ignore attempts to set bounds on version-inflexible methods,
+     * returning success.
      */
     switch (method_version) {
     default:
-        /*
-         * XXX For fixed version methods, should we always fail and not set any
-         * bounds, always succeed and not set any bounds, or set the bounds and
-         * arrange to fail later if they are not met?  At present fixed-version
-         * methods are not subject to controls that disable individual protocol
-         * versions.
-         */
-        return 0;
+        break;
 
     case TLS_ANY_VERSION:
-        if (version < SSL3_VERSION || version > TLS_MAX_VERSION_INTERNAL)
-            return 0;
+        if (valid_tls)
+            *bound = version;
         break;
 
     case DTLS_ANY_VERSION:
-        if (DTLS_VERSION_GT(version, DTLS_MAX_VERSION_INTERNAL) ||
-            DTLS_VERSION_LT(version, DTLS1_BAD_VER))
-            return 0;
+        if (valid_dtls)
+            *bound = version;
         break;
     }
-
-    *bound = version;
     return 1;
 }