8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts

Reviewed-by: mullan

Author: Jamil Nimeh

src/java.base/share/classes/sun/security/action/GetPropertyAction.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,6 +28,7 @@
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.Properties;
+import sun.security.util.Debug;
 
 /**
  * A convenience class for retrieving the string value of a system
@@ -160,4 +161,66 @@ public Properties run() {
             );
         }
     }
+
+    /**
+     * Convenience method for fetching System property values that are timeouts.
+     * Accepted timeout values may be purely numeric, a numeric value
+     * followed by "s" (both interpreted as seconds), or a numeric value
+     * followed by "ms" (interpreted as milliseconds).
+     *
+     * @param prop the name of the System property
+     * @param def a default value (in milliseconds)
+     * @param dbg a Debug object, if null no debug messages will be sent
+     *
+     * @return an integer value corresponding to the timeout value in the System
+     *      property in milliseconds.  If the property value is empty, negative,
+     *      or contains non-numeric characters (besides a trailing "s" or "ms")
+     *      then the default value will be returned.  If a negative value for
+     *      the "def" parameter is supplied, zero will be returned if the
+     *      property's value does not conform to the allowed syntax.
+     */
+    public static int privilegedGetTimeoutProp(String prop, int def, Debug dbg) {
+        if (def < 0) {
+            def = 0;
+        }
+
+        String rawPropVal = privilegedGetProperty(prop, "").trim();
+        if (rawPropVal.length() == 0) {
+            return def;
+        }
+
+        // Determine if "ms" or just "s" is on the end of the string.
+        // We may do a little surgery on the value so we'll retain
+        // the original value in rawPropVal for debug messages.
+        boolean isMillis = false;
+        String propVal = rawPropVal;
+        if (rawPropVal.toLowerCase().endsWith("ms")) {
+            propVal = rawPropVal.substring(0, rawPropVal.length() - 2);
+            isMillis = true;
+        } else if (rawPropVal.toLowerCase().endsWith("s")) {
+            propVal = rawPropVal.substring(0, rawPropVal.length() - 1);
+        }
+
+        // Next check to make sure the string is built only from digits
+        if (propVal.matches("^\\d+$")) {
+            try {
+                int timeout = Integer.parseInt(propVal);
+                return isMillis ? timeout : timeout * 1000;
+            } catch (NumberFormatException nfe) {
+                if (dbg != null) {
+                    dbg.println("Warning: Unexpected " + nfe +
+                            " for timeout value " + rawPropVal +
+                            ". Using default value of " + def + " msec.");
+                }
+                return def;
+            }
+        } else {
+            if (dbg != null) {
+                dbg.println("Warning: Incorrect syntax for timeout value " +
+                        rawPropVal + ". Using default value of " + def +
+                        " msec.");
+            }
+            return def;
+        }
+    }
 }

src/java.base/share/classes/sun/security/provider/certpath/OCSP.java

@@ -26,10 +26,7 @@
 
 import java.io.IOException;
 import java.io.OutputStream;
-import java.net.URI;
-import java.net.URL;
-import java.net.HttpURLConnection;
-import java.net.URLEncoder;
+import java.net.*;
 import java.security.cert.CertificateException;
 import java.security.cert.CertPathValidatorException;
 import java.security.cert.CertPathValidatorException.BasicReason;
@@ -41,7 +38,7 @@
 import java.util.List;
 import java.util.Map;
 
-import sun.security.action.GetIntegerAction;
+import sun.security.action.GetPropertyAction;
 import sun.security.util.Debug;
 import sun.security.util.Event;
 import sun.security.util.IOUtils;
@@ -70,29 +67,36 @@ public final class OCSP {
     private static final Debug debug = Debug.getInstance("certpath");
 
     private static final int DEFAULT_CONNECT_TIMEOUT = 15000;
+    private static final int DEFAULT_READ_TIMEOUT = 15000;
 
     /**
-     * Integer value indicating the timeout length, in seconds, to be
-     * used for the OCSP check. A timeout of zero is interpreted as
-     * an infinite timeout.
+     * Integer value indicating the timeout length, in milliseconds, to be
+     * used for establishing a connection to an OCSP responder. A timeout of
+     * zero is interpreted as an infinite timeout.
      */
-    private static final int CONNECT_TIMEOUT = initializeTimeout();
+    private static final int CONNECT_TIMEOUT = initializeTimeout(
+            "com.sun.security.ocsp.timeout", DEFAULT_CONNECT_TIMEOUT);
+
+    /**
+     * Integer value indicating the timeout length, in milliseconds, to be
+     * used for reading an OCSP response from the responder.  A timeout of
+     * zero is interpreted as an infinite timeout.
+     */
+    private static final int READ_TIMEOUT = initializeTimeout(
+            "com.sun.security.ocsp.readtimeout", DEFAULT_READ_TIMEOUT);
 
     /**
      * Initialize the timeout length by getting the OCSP timeout
      * system property. If the property has not been set, or if its
      * value is negative, set the timeout length to the default.
      */
-    private static int initializeTimeout() {
-        @SuppressWarnings("removal")
-        Integer tmp = java.security.AccessController.doPrivileged(
-                new GetIntegerAction("com.sun.security.ocsp.timeout"));
-        if (tmp == null || tmp < 0) {
-            return DEFAULT_CONNECT_TIMEOUT;
+    private static int initializeTimeout(String prop, int def) {
+        int timeoutVal =
+                GetPropertyAction.privilegedGetTimeoutProp(prop, def, debug);
+        if (debug != null) {
+            debug.println(prop + " set to " + timeoutVal + " milliseconds");
         }
-        // Convert to milliseconds, as the system property will be
-        // specified in seconds
-        return tmp * 1000;
+        return timeoutVal;
     }
 
     private OCSP() {}
@@ -183,17 +187,18 @@ public static byte[] getOCSPBytes(List<CertId> certIds, URI responderURI,
                     Base64.getEncoder().encodeToString(bytes), UTF_8));
 
             if (encodedGetReq.length() <= 255) {
-                @SuppressWarnings("deprecation")
-                var _unused = url = new URL(encodedGetReq.toString());
+                url = new URI(encodedGetReq.toString()).toURL();
                 con = (HttpURLConnection)url.openConnection();
+                con.setConnectTimeout(CONNECT_TIMEOUT);
+                con.setReadTimeout(READ_TIMEOUT);
                 con.setDoOutput(true);
                 con.setDoInput(true);
                 con.setRequestMethod("GET");
             } else {
                 url = responderURI.toURL();
                 con = (HttpURLConnection)url.openConnection();
                 con.setConnectTimeout(CONNECT_TIMEOUT);
-                con.setReadTimeout(CONNECT_TIMEOUT);
+                con.setReadTimeout(READ_TIMEOUT);
                 con.setDoOutput(true);
                 con.setDoInput(true);
                 con.setRequestMethod("POST");
@@ -223,6 +228,8 @@ public static byte[] getOCSPBytes(List<CertId> certIds, URI responderURI,
             return (contentLength == -1) ? con.getInputStream().readAllBytes() :
                     IOUtils.readExactlyNBytes(con.getInputStream(),
                             contentLength);
+        } catch (URISyntaxException urise) {
+            throw new IOException(urise);
         } finally {
             if (con != null) {
                 con.disconnect();

src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -50,7 +50,8 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
-import sun.security.action.GetIntegerAction;
+
+import sun.security.action.GetPropertyAction;
 import sun.security.x509.AccessDescription;
 import sun.security.x509.GeneralNameInterface;
 import sun.security.x509.URIName;
@@ -127,8 +128,12 @@ class URICertStore extends CertStoreSpi {
     // allowed when downloading CRLs
     private static final int DEFAULT_CRL_READ_TIMEOUT = 15000;
 
+    // Default connect and read timeouts for CA certificate fetching (15 sec)
+    private static final int DEFAULT_CACERT_CONNECT_TIMEOUT = 15000;
+    private static final int DEFAULT_CACERT_READ_TIMEOUT = 15000;
+
     /**
-     * Integer value indicating the connect timeout, in seconds, to be
+     * Integer value indicating the connect timeout, in milliseconds, to be
      * used for the CRL download. A timeout of zero is interpreted as
      * an infinite timeout.
      */
@@ -137,30 +142,44 @@ class URICertStore extends CertStoreSpi {
                           DEFAULT_CRL_CONNECT_TIMEOUT);
 
     /**
-     * Integer value indicating the read timeout, in seconds, to be
+     * Integer value indicating the read timeout, in milliseconds, to be
      * used for the CRL download. A timeout of zero is interpreted as
      * an infinite timeout.
      */
     private static final int CRL_READ_TIMEOUT =
         initializeTimeout("com.sun.security.crl.readtimeout",
                           DEFAULT_CRL_READ_TIMEOUT);
 
+    /**
+     * Integer value indicating the connect timeout, in milliseconds, to be
+     * used for the CA certificate download. A timeout of zero is interpreted
+     * as an infinite timeout.
+     */
+    private static final int CACERT_CONNECT_TIMEOUT =
+            initializeTimeout("com.sun.security.cert.timeout",
+                    DEFAULT_CACERT_CONNECT_TIMEOUT);
+
+    /**
+     * Integer value indicating the read timeout, in milliseconds, to be
+     * used for the CA certificate download. A timeout of zero is interpreted
+     * as an infinite timeout.
+     */
+    private static final int CACERT_READ_TIMEOUT =
+            initializeTimeout("com.sun.security.cert.readtimeout",
+                    DEFAULT_CACERT_READ_TIMEOUT);
+
     /**
      * Initialize the timeout length by getting the specified CRL timeout
      * system property. If the property has not been set, or if its
      * value is negative, set the timeout length to the specified default.
      */
     private static int initializeTimeout(String prop, int def) {
-        Integer tmp = GetIntegerAction.privilegedGetProperty(prop);
-        if (tmp == null || tmp < 0) {
-            return def;
-        }
+        int timeoutVal =
+                GetPropertyAction.privilegedGetTimeoutProp(prop, def, debug);
         if (debug != null) {
-            debug.println(prop + " set to " + tmp + " seconds");
+            debug.println(prop + " set to " + timeoutVal + " milliseconds");
         }
-        // Convert to milliseconds, as the system property will be
-        // specified in seconds
-        return tmp * 1000;
+        return timeoutVal;
     }
 
     /**
@@ -276,6 +295,8 @@ static CertStore getInstance(AccessDescription ad) {
                 connection.setIfModifiedSince(lastModified);
             }
             long oldLastModified = lastModified;
+            connection.setConnectTimeout(CACERT_CONNECT_TIMEOUT);
+            connection.setReadTimeout(CACERT_READ_TIMEOUT);
             try (InputStream in = connection.getInputStream()) {
                 lastModified = connection.getLastModified();
                 if (oldLastModified != 0) {