[JENKINS-37566] - Always run setAccessible and classloader constructors in the privileged blocks #118

oleg-nenashev · 2016-10-07T21:57:36Z

- Always run setAccessible() from an privileged block (via Util class)
- Create Classloaders inside privileged blocks

Just fixes about 7 issues reported by FindBugs.

https://issues.jenkins-ci.org/browse/JENKINS-37566

Resolves 5 FB issues

…ClassLoader

oleg-nenashev

Well, I cannot request changes in my own PR. So convenient :(

oleg-nenashev · 2016-10-07T21:59:17Z

src/main/java/hudson/remoting/Which.java

@@ -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;


Self-:bug: causes infinite cycle

oleg-nenashev · 2016-10-07T21:59:39Z

src/main/java/hudson/remoting/Which.java

                    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;


Maybe better to stop execution as well

oleg-nenashev · 2016-10-07T22:00:33Z

src/main/java/org/jenkinsci/remoting/nio/SelectableFileChannelFactory.java

@@ -128,7 +131,7 @@ public SocketChannel create(FileDescriptor fd) {
        } catch (ClassNotFoundException e) {


Multi-catch around should be cleaned up during the refactoring

oleg-nenashev · 2016-10-07T22:01:24Z

src/test/java/org/jenkinsci/remoting/engine/HandlerLoopbackLoadStress.java

-import java.security.MessageDigest;
-import java.security.NoSuchAlgorithmException;
-import java.security.UnrecoverableKeyException;
+import java.security.*;


Self-:bug:
Tried editing something in IDEA, going back to my caves

…elds as accessible

oleg-nenashev · 2016-10-25T10:54:40Z

@reviewbybees since the test failure is unrelated

ghost · 2016-10-25T11:37:15Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

kohsuke · 2016-10-27T01:17:29Z

src/main/java/org/jenkinsci/remoting/util/ReflectionUtils.java

+     * @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 {


🐛 This is a wrong usage of AccessController.doPrivileged() that creates a security hole. It allows untrusted caller to use this public method to elevate privilege and make any method/field accessible.

It's best not to fiddle with AcessController IMO. The only situation where it makes a difference is when remoting is in trusted code source and the client app code is not trusted. Remoting deals too much with classloader and other potentially dangerous code. It's best just not to let the untrusted caller use remoting at all, which can be achieved by not using AccessController.doPrivileged()

Trusted caller can always do it via the reflection API, no?

Anyway, restriction of this Utility class access is reasonable

Trusted caller can always do it via the reflection API, no?

I'm not sure how to interpret that.

A decade ago, I worked on JAXB, which eventually became a part of JavaSE. A part of that integration work involves making JAXB safely usable from untrusted code (think of applet/JavaFX, for example), and that experience taught me a lot about the danger and difficulty of correctly using privilege escalation.

The findbugs explanation of DP_DO_INSIDE_DO_PRIVILEGED does not discuss any implication of privilege escalations and down right dangerous.

IIUC, the motivation here is to make this module FindBugs clean, and in that case I recommend simply disabling this detector.

I can tell you more about this if you really care about, but it's much too long to type it in.

That's why I submitted it as a separate PR. I went through some docs/discussions on this topic and decided it may be reasonable to perform such change.

The motivation is to really make FindBugs clean, but I agree it requires more investigation. Putting it on hold

…kinsci#118

…per discussion in jenkinsci#118

@stephenc

* [JENKINS-37566] - FindBugs: Unclosed stream in hudson.remoting.Capability * [JENKINS-37566] - FindBugs: Stream not closed on Exception path in InitializeJarCacheMain#copyFile * [JENKINS-37566] - Fix Util#makeResource and remove obsolete hack * Extra Util#makeResource() polishing * Deprecate obsolete Util#mkdirs() * Exceptional case in setLastModifiedTime * Handle exception case during temp file deletion in FileSystemJarCache * Synchronize ProxyWriter#closed in write() * Synchronization of ProxyWriter#channel * Get rid of the obsolete collection in Channel#properties, fix synchronization * Checksum#calculateFor() - no need to put if absent since there is a check above * UWF_UNWRITTEN_FIELD in ExportTable$Entry * NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE in JnlpAgentEndpointResolver. Now we use a more agressive check * UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR in org.jenkinsci.remoting.nio.NioChannelHub$NioTransport.abort * UPM_UNCALLED_PRIVATE_METHOD - hudson.remoting.ChunkedOutputStream.frameSize() * FindBugs: Revert the stream closure at emoting Capability, just a design to be reworked * Unrealistic NP_NULL_ON_SOME_PATH in NioChannelHub * EI_EXPOSE_REP in DiagnosedStreamCorruptionException (diagnostics code) * SE_TRANSIENT_FIELD_NOT_RESTORED in PreloadJarTask, RemoteClassLoader, RemoteInvocationHandler and UserRequest * Leftover false-positive UG_SYNC_SET_UNSYNC_GET in Channel * EI_EXPOSE_REP in ChannelBuilder#properties * Suppress DMI_NONSERIALIZABLE_OBJECT_WRITTEN in ClassLoaderHolder#writeObject() * Better handling of Streams in hudson.remoting.Capability (still weird) * Fix the Util#makeResource() behavior for nested folders * FileSystemJarCache: tmp file may be renamed * JENKINS-37566 - Modification of Channel#dumpDiagnostics() actually is not required after the merge of #122 * Disable DP_DO_INSIDE_DO_PRIVILEGED as per feeback from @kohsue in #118 * Fix the FindBugs filter * [NP_NULL_ON_SOME_PATH] - Prevent NPE in ExportTable#diagnoseInvalidObjectId() * [RV_RETURN_VALUE_IGNORED_BAD_PRACTICE] - Propagate exceptions when FileSystemJarCache#retrieve() fails to retrieve the target * [DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED] - Ignore the warning as per discussion in #118 * [VO_VOLATILE_INCREMENT] - Suppress warning in Channel#send() * FindBugs - Which#jarFile() should not suppress all exceptions * FindBugs - Suppress URF_UNREAD_FIELD in Pipewindow#Real * [JENKINS-37566] - Channel#properties should be a ConcurrentHashMap Reason - prevent possible delays and deadlocks in the getter method. Addresses the feedback from @stephenc * [JENKINS-37566] - DiagnosedStreamCorruptionException readAhead/readBack handlers should use a more efficient clone() command * [JENKINS-37566] - Suppress UG_SYNC_SET_UNSYNC_GET after switching to the ConcurrentHashMap * [JENKINS-37566] - hudson.remoting.Util should be tolerant against InvalidPathException * [JENKINS-37566] - make ProxyWriter more asynchronous (follow-up to @stephenc’s review) * [JENKINS-37566] - Add a comment about race condition as suggested by @stephenc

oleg-nenashev · 2018-03-03T23:53:34Z

We may hit it in Java9, but I have no plan to work on it anytime soon

jglick · 2020-10-12T13:22:19Z

src/main/java/hudson/remoting/RemoteClassLoader.java

+        // Create new RemoteClassLoader otherwise
+        final RemoteClassLoader created;
+        try {
+            created = AccessController.doPrivileged(


You should only ever need to use this in case you are running with a security manager enabled, which for Remoting we are not. So I do not see the point of this PR. If it is just to get SpotBugs to shut up, use the warning suppression annotation, or just configure a SpotBugs exclude for that warning in the POM.

And there is a Java JEP proposing to deprecate the security manager, and associated methods like this.

jtnord · 2021-05-18T18:41:24Z

Is it time to close this PR given comments from KK and Jesse?

jeffret-b · 2021-05-19T01:00:03Z

@oleg-nenashev , I'm going to close this, based on the various comments and how outdated it is. If you still want to do something with it you can reopen it.

ghost · 2021-05-21T18:54:30Z

No objections

…

On Wed, May 19, 2021, 03:00 'Jeff Thompson' via engineering-code-reviews < ***@***.***> wrote: @oleg-nenashev <https://github.com/oleg-nenashev> , I'm going to close this, based on the various comments and how outdated it is. If you still want to do something with it you can reopen it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#118 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACQGUVHNQXNK3GDPNAPT2ELTOMESPANCNFSM4CSF7DLA> . -- You received this message because you are subscribed to the Google Groups "engineering-code-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to ***@***.*** To view this discussion on the web visit https://groups.google.com/a/cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/remoting/pull/118/c843667080%40github.com <https://groups.google.com/a/cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/remoting/pull/118/c843667080%40github.com?utm_medium=email&utm_source=footer> .

oleg-nenashev added 3 commits October 7, 2016 23:53

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

755473f

Resolves 5 FB issues

[JENKINS-37566] - DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED i Remote…

a185cb2

…ClassLoader

[JENKINS-37566] - Fix imports and code formatting

408bc16

oleg-nenashev commented Oct 7, 2016

View reviewed changes

oleg-nenashev added the work-in-progress label Oct 9, 2016

oleg-nenashev changed the title ~~[JENKINS-37566] - Always run setAccessible~~ [JENKINS-37566] - Always run setAccessible and classloader constructors in the privileged blocks Oct 13, 2016

oleg-nenashev added 2 commits October 25, 2016 11:31

[JENKINS-37566] - JBoss magic should really fail if we cannot mark fi…

574a815

…elds as accessible

Restore imports in the HandlerLoobbackLoadStress

353ee7e

oleg-nenashev added this to the remoting-3.1 milestone Oct 25, 2016

oleg-nenashev added needs-review and removed work-in-progress labels Oct 25, 2016

kohsuke reviewed Oct 27, 2016

View reviewed changes

oleg-nenashev added the on-hold label Nov 2, 2016

oleg-nenashev added a commit to oleg-nenashev/remoting that referenced this pull request Feb 17, 2017

Disable DP_DO_INSIDE_DO_PRIVILEGED as per feeback from @kohsue in jen…

49fbad0

…kinsci#118

oleg-nenashev added a commit to oleg-nenashev/remoting that referenced this pull request Mar 6, 2017

[DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED] - Ignore the warning as …

88da198

…per discussion in jenkinsci#118

oleg-nenashev added the work-in-progress label May 3, 2017

oleg-nenashev removed needs-review work-in-progress labels Nov 20, 2017

jglick requested changes Oct 12, 2020

View reviewed changes

jeffret-b closed this May 19, 2021

jglick deleted the bug/JENKINS-37566-doPrivileged branch May 19, 2021 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-37566] - Always run setAccessible and classloader constructors in the privileged blocks #118

[JENKINS-37566] - Always run setAccessible and classloader constructors in the privileged blocks #118

oleg-nenashev commented Oct 7, 2016

oleg-nenashev left a comment

oleg-nenashev Oct 7, 2016

oleg-nenashev Oct 7, 2016

oleg-nenashev Oct 7, 2016

oleg-nenashev Oct 7, 2016

oleg-nenashev commented Oct 25, 2016

ghost commented Oct 25, 2016

kohsuke Oct 27, 2016 •

edited

Loading

oleg-nenashev Oct 27, 2016

oleg-nenashev Oct 27, 2016

kohsuke Oct 27, 2016

oleg-nenashev Nov 2, 2016

oleg-nenashev commented Mar 3, 2018

jglick Oct 12, 2020

jglick May 18, 2021

jtnord commented May 18, 2021 •

edited

Loading

jeffret-b commented May 19, 2021

ghost commented May 21, 2021 via email

		@@ -128,7 +131,7 @@ public SocketChannel create(FileDescriptor fd) {
		} catch (ClassNotFoundException e) {

[JENKINS-37566] - Always run setAccessible and classloader constructors in the privileged blocks #118

[JENKINS-37566] - Always run setAccessible and classloader constructors in the privileged blocks #118

Conversation

oleg-nenashev commented Oct 7, 2016

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Oct 25, 2016

ghost commented Oct 25, 2016

kohsuke Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord commented May 18, 2021 • edited Loading

jeffret-b commented May 19, 2021

ghost commented May 21, 2021 via email

kohsuke Oct 27, 2016 •

edited

Loading

jtnord commented May 18, 2021 •

edited

Loading