[JENKINS-37566] - Always run setAccessible() in privileged blocks

Resolves 5 FB issues
jenkinsci · Oct 7, 2016 · 755473f · 755473f
1 parent 9c40e36
commit 755473f
Show file tree

Hide file tree

Showing 9 changed files with 143 additions and 36 deletions.
diff --git a/src/main/java/hudson/remoting/Launcher.java b/src/main/java/hudson/remoting/Launcher.java
@@ -36,6 +36,7 @@
 import javax.net.ssl.SSLSocketFactory;
 import javax.net.ssl.TrustManagerFactory;
 import org.jenkinsci.remoting.util.IOUtils;
+import org.jenkinsci.remoting.util.ReflectionUtils;
 import org.w3c.dom.Document;
 import org.w3c.dom.NodeList;
 import org.xml.sax.SAXException;
@@ -77,17 +78,13 @@
 import java.net.HttpURLConnection;
 import java.net.Authenticator;
 import java.net.PasswordAuthentication;
-import java.security.GeneralSecurityException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.security.cert.X509Certificate;
 import java.security.cert.CertificateException;
-import java.security.NoSuchAlgorithmException;
-import java.security.KeyManagementException;
-import java.security.SecureRandom;
 import java.util.Properties;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -138,8 +135,8 @@ public void setTextMode(boolean b) {
     @Option(name="-cp",aliases="-classpath",metaVar="PATH",
             usage="add the given classpath elements to the system classloader.")
     public void addClasspath(String pathList) throws Exception {
-        Method $addURL = URLClassLoader.class.getDeclaredMethod("addURL", URL.class);
-        $addURL.setAccessible(true);
+        final Method $addURL = URLClassLoader.class.getDeclaredMethod("addURL", URL.class);
+        ReflectionUtils.makeAccessible($addURL);
 
         for(String token : pathList.split(File.pathSeparator))
             $addURL.invoke(ClassLoader.getSystemClassLoader(),new File(token).toURI().toURL());

diff --git a/src/main/java/hudson/remoting/RemoteClassLoader.java b/src/main/java/hudson/remoting/RemoteClassLoader.java
@@ -32,6 +32,8 @@
 import java.net.URL;
 import java.net.MalformedURLException;
 import java.net.URLClassLoader;
+import java.security.AccessController;
+import java.security.PrivilegedExceptionAction;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.Map;
@@ -46,6 +48,7 @@
 import java.util.logging.Logger;
 
 import org.jenkinsci.constant_pool_scanner.ConstantPoolScanner;
+import org.jenkinsci.remoting.util.ReflectionUtils;
 
 import javax.annotation.CheckForNull;
 
@@ -327,7 +330,7 @@ ClassReference toRef(ClassFile2 cf) {
     static {
         try {
             gCLL = ClassLoader.class.getDeclaredMethod("getClassLoadingLock", String.class);
-            gCLL.setAccessible(true);
+            ReflectionUtils.makeAccessible(gCLL);
         } catch (NoSuchMethodException x) {
             // OK, Java 6
         } catch (Exception x) {

diff --git a/src/main/java/hudson/remoting/RemoteInvocationHandler.java b/src/main/java/hudson/remoting/RemoteInvocationHandler.java
@@ -52,6 +52,7 @@
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import org.jenkinsci.remoting.RoleChecker;
+import org.jenkinsci.remoting.util.ReflectionUtils;
 
 /**
  * Sits behind a proxy object and implements the proxy logic.
@@ -884,7 +885,7 @@ protected Serializable perform(Channel channel) throws Throwable {
                 Method m = choose(clazz);
                 if(m==null)
                     throw new IllegalStateException("Unable to call " + methodName + ". No matching method found in " + Arrays.toString(clazz) + " for " + o);
-                m.setAccessible(true);  // in case the class is not public
+                ReflectionUtils.makeAccessible(m);
                 Object r;
                 try {
                     r = m.invoke(o, arguments);

diff --git a/src/main/java/hudson/remoting/Which.java b/src/main/java/hudson/remoting/Which.java
@@ -23,6 +23,8 @@
  */
 package hudson.remoting;
 
+import org.jenkinsci.remoting.util.ReflectionUtils;
+
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
@@ -33,6 +35,7 @@
 import java.net.URLConnection;
 import java.net.JarURLConnection;
 import java.lang.reflect.Field;
+import java.security.PrivilegedActionException;
 import java.util.zip.ZipFile;
 import java.util.jar.JarFile;
 import java.util.logging.Logger;
@@ -141,19 +144,25 @@ public static File jarFile(Class clazz) throws IOException {
                 Object delegate = is;
                 while (delegate.getClass().getEnclosingClass()!=ZipFile.class) {
                     Field f = delegate.getClass().getDeclaredField("delegate");
-                    f.setAccessible(true);
+                    if (!ReflectionUtils.makeAccessibleOrLog(f, LOGGER, Level.FINE)) {
+                        continue;
+                    }
+
                     delegate = f.get(delegate);
                     //JENKINS-5922 - workaround for CertificateReaderInputStream; JBoss 5.0.0, EAP 5.0 and EAP 5.1
                     if(delegate.getClass().getName().equals("java.util.jar.JarVerifier$VerifierStream")){
                         f = delegate.getClass().getDeclaredField("is");
-                        f.setAccessible(true);
+                        if (!ReflectionUtils.makeAccessibleOrLog(f, LOGGER, Level.FINE)) {
+                            continue;
+                        }
                         delegate = f.get(delegate);
                     }
                 }
                 Field f = delegate.getClass().getDeclaredField("this$0");
-                f.setAccessible(true);
-                ZipFile zipFile = (ZipFile)f.get(delegate);
-                return new File(zipFile.getName());
+                if (ReflectionUtils.makeAccessibleOrLog(f, LOGGER, Level.FINE)) {
+                    ZipFile zipFile = (ZipFile)f.get(delegate);
+                    return new File(zipFile.getName());
+                } // Else fall through till alternative actions or exception
             } catch (NoSuchFieldException e) {
                 // something must have changed in JBoss5. fall through
                 LOGGER.log(Level.FINE, "Failed to resolve vfszip into a jar location",e);
@@ -201,8 +210,9 @@ public static File jarFile(Class clazz) throws IOException {
                     // so this just keeps getting tricker and trickier...
                     try {
                         Field f = ZipFile.class.getDeclaredField("name");
-                        f.setAccessible(true);
-                        return new File((String) f.get(jarFile));
+                        if (ReflectionUtils.makeAccessibleOrLog(f, LOGGER, Level.FINE)) {
+                            return new File((String) f.get(jarFile));
+                        }
                     } catch (NoSuchFieldException e) {
                         LOGGER.log(Level.INFO, "Failed to obtain the local cache file name of "+resURL, e);
                     } catch (IllegalAccessException e) {

diff --git a/src/main/java/org/jenkinsci/remoting/nio/SelectableFileChannelFactory.java b/src/main/java/org/jenkinsci/remoting/nio/SelectableFileChannelFactory.java
@@ -1,5 +1,7 @@
 package org.jenkinsci.remoting.nio;
 
+import org.jenkinsci.remoting.util.ReflectionUtils;
+
 import java.io.BufferedInputStream;
 import java.io.BufferedOutputStream;
 import java.io.File;
@@ -20,6 +22,7 @@
 import java.nio.channels.SelectableChannel;
 import java.nio.channels.SocketChannel;
 import java.nio.channels.spi.SelectorProvider;
+import java.security.PrivilegedActionException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -47,12 +50,12 @@ protected FileInputStream unwrap(InputStream i) {
         if (i instanceof BufferedInputStream) {
             try {
                 Field $in = FilterInputStream.class.getDeclaredField("in");
-                $in.setAccessible(true);
+                ReflectionUtils.makeAccessible($in);
                 return unwrap((InputStream)$in.get(i));
             } catch (NoSuchFieldException e) {
                 warn(e);
                 return null;
-            } catch (IllegalAccessException e) {
+            } catch (IllegalAccessException | PrivilegedActionException e) {
                 warn(e);
                 return null;
             }
@@ -67,12 +70,12 @@ protected FileOutputStream unwrap(OutputStream i) {
         if (i instanceof BufferedOutputStream) {
             try {
                 Field $in = FilterOutputStream.class.getDeclaredField("out");
-                $in.setAccessible(true);
+                ReflectionUtils.makeAccessible($in);
                 return unwrap((OutputStream)$in.get(i));
             } catch (NoSuchFieldException e) {
                 warn(e);
                 return null;
-            } catch (IllegalAccessException e) {
+            } catch (IllegalAccessException | PrivilegedActionException e) {
                 warn(e);
                 return null;
             }
@@ -111,11 +114,11 @@ public SocketChannel create(FileDescriptor fd) {
                 FileDescriptor.class,
                 InetSocketAddress.class
             );
-            $c.setAccessible(true);
+            ReflectionUtils.makeAccessible($c);
 
             // increment the FileDescriptor use count since we are giving it to SocketChannel
             Method $m = fd.getClass().getDeclaredMethod("incrementAndGetUseCount");
-            $m.setAccessible(true);
+            ReflectionUtils.makeAccessible($m);
             $m.invoke(fd);
 
             return (SocketChannel)$c.newInstance(SelectorProvider.provider(), fd, null);
@@ -128,7 +131,7 @@ public SocketChannel create(FileDescriptor fd) {
         } catch (ClassNotFoundException e) {
             warn(e);
             return null;
-        } catch (IllegalAccessException e) {
+        } catch (IllegalAccessException | PrivilegedActionException e) {
             warn(e);
             return null;
         } catch (InvocationTargetException e) {

diff --git a/src/main/java/org/jenkinsci/remoting/util/ReflectionUtils.java b/src/main/java/org/jenkinsci/remoting/util/ReflectionUtils.java
@@ -0,0 +1,95 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2016, Oleg Nenashev, CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package org.jenkinsci.remoting.util;
+
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
+import javax.annotation.CheckReturnValue;
+import javax.annotation.Nonnull;
+import java.lang.reflect.AccessibleObject;
+import java.security.AccessController;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+/**
+ * Provides some reflection util methods.
+ * This class is not designed for the external use.
+ * @author Oleg Nenashev
+ */
+@Restricted(NoExternalUse.class)
+public class ReflectionUtils {
+
+    private ReflectionUtils() {
+        // Instantiation is prohibited
+    }
+
+    /**
+     * Makes a method accessible using reflection.
+     * There is no automatic accessibility status recovery.
+     * @param object Object to be modified
+     * @throws PrivilegedActionException Missing permissions or other modification error.
+     *      This exception cannot happen if the object is accessible when the method is invoked.
+     */
+    public static void makeAccessible(final @Nonnull AccessibleObject object) throws PrivilegedActionException {
+        if (object.isAccessible()) {
+            // No need to change anything
+            return;
+        }
+        AccessController.doPrivileged(
+            new PrivilegedExceptionAction<Void>() {
+                @Override
+                public Void run() throws Exception {
+                    object.setAccessible(true);
+                    return null;
+                }
+            });
+    }
+
+
+    /**
+     * Makes a method accessible using reflection.
+     * In the case of {@link PrivilegedActionException} logs them without propagating exception.
+     * There is no automatic accessibility status recovery.
+     * @param object Object to be modified
+     * @param logger Logger for errors
+     * @param level  Logging level for errors
+     * @return {@code true} on success path, {@code false} if the modification error happens
+     */
+    @CheckReturnValue
+    public static boolean makeAccessibleOrLog(final @Nonnull AccessibleObject object,
+                                              final @Nonnull Logger logger,
+                                              final @Nonnull Level level) {
+        try {
+            makeAccessible(object);
+        } catch (PrivilegedActionException ex) {
+            logger.log(level, String.format("Cannot make the field {0} accessible", object), ex);
+            return false;
+        }
+        return true;
+    }
+
+}
diff --git a/src/test/java/org/jenkinsci/remoting/engine/HandlerLoopbackLoadStress.java b/src/test/java/org/jenkinsci/remoting/engine/HandlerLoopbackLoadStress.java
@@ -45,14 +45,7 @@
 import java.nio.channels.SelectionKey;
 import java.nio.channels.ServerSocketChannel;
 import java.nio.channels.SocketChannel;
-import java.security.KeyManagementException;
-import java.security.KeyPair;
-import java.security.KeyPairGenerator;
-import java.security.KeyStore;
-import java.security.KeyStoreException;
-import java.security.MessageDigest;
-import java.security.NoSuchAlgorithmException;
-import java.security.UnrecoverableKeyException;
+import java.security.*;
 import java.security.cert.Certificate;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
@@ -76,6 +69,7 @@
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
+import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import javax.net.ssl.KeyManagerFactory;
 import javax.net.ssl.SSLContext;
@@ -100,6 +94,7 @@
 import org.jenkinsci.remoting.protocol.IOHubRegistrationCallback;
 import org.jenkinsci.remoting.protocol.cert.BlindTrustX509ExtendedTrustManager;
 import org.jenkinsci.remoting.util.Charsets;
+import org.jenkinsci.remoting.util.ReflectionUtils;
 import org.jenkinsci.remoting.util.SettableFuture;
 import org.kohsuke.args4j.CmdLineException;
 import org.kohsuke.args4j.CmdLineParser;
@@ -358,14 +353,13 @@ public Void call() throws Exception {
         }
     }
 
+    @CheckForNull
     private static Method _getProcessCpuTime(OperatingSystemMXBean operatingSystemMXBean) {
         Method getProcessCpuTime;
         try {
             getProcessCpuTime = operatingSystemMXBean.getClass().getMethod("getProcessCpuTime");
-            getProcessCpuTime.setAccessible(true);
-        } catch (ClassCastException e) {
-            getProcessCpuTime = null;
-        } catch (NoSuchMethodException e) {
+            ReflectionUtils.makeAccessible(getProcessCpuTime);
+        } catch (ClassCastException | NoSuchMethodException | PrivilegedActionException e) {
             getProcessCpuTime = null;
         }
         return getProcessCpuTime;

diff --git a/src/test/java/org/jenkinsci/remoting/nio/Main.java b/src/test/java/org/jenkinsci/remoting/nio/Main.java
@@ -1,5 +1,7 @@
 package org.jenkinsci.remoting.nio;
 
+import org.jenkinsci.remoting.util.ReflectionUtils;
+
 import java.io.FileInputStream;
 import java.io.FilterInputStream;
 import java.io.IOException;
@@ -91,7 +93,7 @@ private static FileInputStream unwrap(InputStream i) throws Exception {
         while (true) {
             if (i instanceof FilterInputStream) {
                 Field $in = FilterInputStream.class.getDeclaredField("in");
-                $in.setAccessible(true);
+                ReflectionUtils.makeAccessible($in);
                 i = (InputStream)$in.get(i);
                 continue;
             }

diff --git a/src/test/java/org/jenkinsci/remoting/protocol/impl/ConnectionHeadersTest.java b/src/test/java/org/jenkinsci/remoting/protocol/impl/ConnectionHeadersTest.java
@@ -28,6 +28,8 @@
 import java.util.Collections;
 import java.util.Map;
 import java.util.TreeMap;
+
+import org.jenkinsci.remoting.util.ReflectionUtils;
 import org.junit.Test;
 
 import static org.hamcrest.Matchers.containsString;
@@ -200,7 +202,7 @@ public void utilityClass_1() throws Exception {
     @Test
     public void utilityClass_2() throws Exception {
         Constructor<ConnectionHeaders> constructor = ConnectionHeaders.class.getDeclaredConstructor();
-        constructor.setAccessible(true);
+        ReflectionUtils.makeAccessible(constructor);
         try {
             constructor.newInstance();
             fail();