[JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist #3120

jglick · 2017-10-30T18:23:47Z

See JENKINS-47736 and JEP 200.

Downstream of jenkinsci/remoting#208.

Proposed changelog entries

JEP-200 integrated. See blog post.

@reviewbybees @jenkinsci/code-reviewers

jglick · 2017-11-28T14:10:52Z

Seems to have had some snapshot confusion with jenkins-test-harness; will simplify by just cutting a release of that.

…ome an IOException caught by Descriptor.save.

jtnord

seems ok - some comments left inline

jtnord · 2017-12-05T10:12:39Z

core/src/main/java/jenkins/security/ClassFilterImpl.java

+                LOGGER.log(Level.FINE, "permitting {0} since it is an array", name);
+                return false;
+            }
+            if (Throwable.class.isAssignableFrom(c)) {


I am sure throwables used to be blocked? was there not a good reason to block them - has this changed?

I do not recall any case of Throwables being (deliberately) blocked. In fact I recall some suggestion to allow org.codehaus.groovy.runtime.powerassert.PowerAssertionError and the like through despite their whole package subtree being in the standard blacklist.

I tried for a while to whitelist individual Throwables but found it completely impractical: every miscellaneous kind of exception that might get thrown in the course of a typical Remoting call wound up having to be added. I think this was in UserResponse, despite the existence of ProxyException, though it has been a while.

Also ErrorAction tries to directly record any Throwable terminating a Pipeline step, only using ProxyException for problematic classes.

In practice Throwables tend to just have a message and stack trace and be unlikely to be a vector for exploits. We can always continue to blacklist any individual class we suspect of being problematic.

jtnord · 2017-12-05T10:15:03Z

core/src/main/java/jenkins/security/ClassFilterImpl.java

+        });
+    }
+
+    private static final Pattern CLASSES_JAR = Pattern.compile("(file:/.+/)WEB-INF/lib/classes[.]jar");


NIT: code conventions is static fields are declared before non static fields.

I wanted to keep this close to its sole usage.

jtnord · 2017-12-05T10:18:26Z

core/src/main/java/jenkins/security/CustomClassFilter.java

+    /**
+     * Determine whether a class should be permitted by {@link ClassFilter#isBlacklisted(Class)} of {@link ClassFilter#DEFAULT}.
+     * @param c the class
+     * @return true to permit it when it would normally be rejected (for example due to having a custom serialization method and being from a third-party library);


NIT: Boolean.TRUE not true. etc.

Well, sure, but autoboxing makes that pretty transparent anyway.

jtnord · 2017-12-05T10:23:42Z

core/src/main/resources/jenkins/security/whitelisted-classes.txt

+org.acegisecurity.userdetails.User
+org.apache.commons.fileupload.disk.DiskFileItem
+org.apache.commons.fileupload.util.FileItemHeadersImpl
+org.eclipse.jgit.lib.ObjectId


why is this defined here and not in the git-client plugin?

It could be, of course; ditto a few others classes coming from job-dsl, maven-plugin, and workflow-support. Logistically I just wanted to make sure I was producing a jenkins.war that did not introduce regressions to popular plugins, without requiring people to apply recent plugin updates. TBD.

My proposal would be to exclude whitelist to a plugin and to mark it as a detached one. In such case we will be able to quickly deliver patches to whitelists if something goes south.... If XStream manages to startup from config.xml... So I am +1 for keeping what we have here

jtnord · 2017-12-05T10:25:38Z

test/src/test/java/jenkins/install/SetupWizardTest.java

@@ -148,15 +148,15 @@ private String jsonRequest(JenkinsRule.WebClient wc, String path) throws Excepti

    private static final class CustomUpdateSite extends UpdateSite {

-        private final TemporaryFolder tmpdir;
+        private final File tmpdir;


where does this now get cleaned up?

The @Rule at the top of the class.

jtnord · 2017-12-05T10:27:06Z

test/src/test/java/jenkins/security/ClassFilterImplTest.java

+    @Test
+    public void whitelistSanity() throws Exception {
+        try (InputStream is = ClassFilterImpl.class.getResourceAsStream("whitelisted-classes.txt")) {
+            List<String> lines = IOUtils.readLines(is);


readLines(InputStream) uses the platform default charset and not UTF-8. so this test will be platform dependant

True, will fix.

jtnord · 2017-12-05T10:28:43Z

test/src/test/java/jenkins/security/ClassFilterImplTest.java

+                try {
+                    Class.forName(line);
+                } catch (ClassNotFoundException x) {
+                    System.err.println("skipping checks of unknown class " + line);


this code seems to be just wasting cycles - please add an assert if you expect all classes to be covered, or move this outside of a test (if you want to manually check)

The whole test case takes about 1s, and this line helps identify classes contributed by plugins (as in discussion above).

jtnord · 2017-12-05T10:31:38Z

test/src/test/java/jenkins/security/Security218CliTest.java

@@ -254,6 +254,7 @@ public void checkRoles(RoleChecker checker) throws SecurityException {
                    }
                });
            } catch (Exception ex) {
+                ex.printStackTrace();


looks like this will dump out stack traces even for a successful test (which is bad) as stack traces for OK tests look scary when running and set of triggers in human brains.

Generally agree. In this case it is a security test and if you are running it interactively (i.e., -Dtest=…) you should be aware that an exception is wanted here. I specifically added this line because I had trouble diagnosing a problem in the test previously, and this made things much clearer.

@stephenc

…nkinsci#3120 which I have asked for help from @stephenc diagnosing.

…e for purposes of whitelisting. <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-jar-plugin</artifactId> <configuration> <archive> <manifestEntries> <Jenkins-ClassFilter-Whitelisted>true</Jenkins-ClassFilter-Whitelisted> </manifestEntries> </archive> </configuration> </plugin>

…rely log violations.

oleg-nenashev

OK, I'd guess that's it for this weekly

oleg-nenashev · 2018-01-12T21:19:55Z

core/src/main/java/jenkins/security/ClassFilterImpl.java

@@ -76,6 +80,11 @@ public static void register() {
            return;
        }
        ClassFilter.setDefault(new ClassFilterImpl());
+        if (SUPPRESS_ALL) {
+            LOGGER.warning("All class filtering suppressed. Your Jenkins installation is at risk from known attacks. See https://jenkins.io/redirect/class-filter/");


That blog post does not describe the blacklist concerns, but we can update the link later

allxiao · 2018-01-15T08:58:46Z

Thanks @oleg-nenashev for mentioning us.
It looks some of the Azure plugins are affected. We're working on the migration.

To confirm, the blacklist/whitelist only applies to the following scenarios:

objects that is serialized to XML using XStream(2), such as the build configuration
objects returned by a Callable in a slave invocation

Right?

oleg-nenashev · 2018-01-15T09:32:19Z

@ArieShout Yes, it's correct. You can find some guidelines here: https://jenkins.io/blog/2018/01/13/jep-200/#for-plugin-developers and ~~https://groups.google.com/forum/#!topic/jenkinsci-dev/We-14-z_YXU~~. Let me know if you need any other info, we will get the guidelines extended.

oleg-nenashev · 2018-01-15T09:33:37Z

Sorry, correct DEV ML link: https://groups.google.com/forum/#!topic/jenkinsci-dev/EALjDtS4riU

allxiao · 2018-01-16T09:08:32Z

We have applied the fix to the Azure plugins (azure-vm-agents specifically) and ~~will cut new release soon.~~

It seems the org.jvnet.localizer.Localizable is already in the whitelist, so we're done on our side.

Thanks,

@stephenc

* Slave.JnlpJar.getURL did not work in some modes when core had a snapshot dependency on Remoting. * Starting to implement Channel.Listener.read/write. * Now obtaining response timing statistics. * For now, avoiding timestamped snapshots, as it caused problems for #3120 which I have asked for help from @stephenc diagnosing. * Simplified logging a bit. * onResponse * hudson.FilePath$Mkdirs is a lot more readable than hudson.FilePath$13. * Specific snapshot. * onJar * LoggingChannelListener * remoting.version=3.17 * Making a few test assertions more lenient to adapt to jenkinsci/remoting#247.

basil · 2022-09-14T20:34:52Z

core/src/main/resources/jenkins/security/whitelisted-classes.txt

+java.lang.reflect.Proxy
+java.net.URI
+java.net.URL
+java.security.KeyRep


@jglick Do you remember what the use case for commit e876849 was? I'm wondering if we need to be worried about XStream trying to serialize/deserialize these types on Java 17, where it would probably fail because we haven't opened those modules.

Do you remember what the use case for commit e876849 was?

Sorry, no. Probably there was a test failure somewhere, in which case there is no need to worry (it would show up if this is really a problem on 17).

XStream trying to serialize/deserialize these types

More likely in Remoting.

[JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist.

903b446

jglick added the work-in-progress The PR is under active development, not ready to the final review label Oct 30, 2017

jglick requested a review from oleg-nenashev October 30, 2017 19:07

jglick changed the title ~~[JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist.~~ [JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist Oct 30, 2017

jglick mentioned this pull request Oct 30, 2017

[JENKINS-47736] Draft JEP: Switch Remoting/XStream blacklist to a whitelist jenkinsci/jep#23

Merged

jglick added 4 commits November 13, 2017 10:03

Merge branch 'master' into whitelist-JENKINS-47736

d8a5d61

Merge branch 'master' into whitelist-JENKINS-47736

ca542c0

Updated remoting.

36cfe89

Merge branch 'master' into whitelist-JENKINS-47736

01c8c55

jenkins-test-harness 2.32

b0f2c33

This was referenced Nov 30, 2017

jenkins-test-harness.version=2.32 jenkinsci/plugin-pom#90

Merged

POM updates jenkinsci/cloudbees-folder-plugin#116

Merged

jglick added 6 commits December 1, 2017 16:17

Merge branch 'master' into whitelist-JENKINS-47736

e2c1ae1

Bump.

67bc386

Merge branch 'master' into whitelist-JENKINS-47736

55d8f4d

Adjusting test after jenkinsci#3167, since now the rejection will bec…

5fa7d0c

…ome an IOException caught by Descriptor.save.

Support dynamically-loaded plugins in CustomClassFilter.Contributed.

5c749cf

Adding some whitelist entries corresponding to keys.

e876849

jtnord reviewed Dec 5, 2017

View reviewed changes

Forgotten charset.

f3623d7

jglick added a commit to jglick/jenkins that referenced this pull request Dec 5, 2017

For now, avoiding timestamped snapshots, as it caused problems for je…

cab2663

…nkinsci#3120 which I have asked for help from @stephenc diagnosing.

Merge branch 'master' into whitelist-JENKINS-47736

8adc5e4

jglick mentioned this pull request Dec 6, 2017

POM update jenkinsci/copyartifact-plugin#97

Merged

jglick added 2 commits December 5, 2017 22:19

org.jenkinsci.plugins.gitclient.RemotingTest.testRemotability

e65601b

jglick mentioned this pull request Dec 7, 2017

POM update jenkinsci/bitbucket-branch-source-plugin#90

Merged

jglick added 3 commits December 19, 2017 14:34

Merge branch 'master' into whitelist-JENKINS-47736

dfc1940

Bump.

14ff05f

Allow whitelists to contain blank lines and comments.

837d0c9

jglick added 3 commits January 12, 2018 09:15

https://github.com/jenkinsci/dependency-check-plugin/pull/20

293897f

Adding general kill switches for the whitelist and blacklist which me…

1c219ba

…rely log violations.

Some additional whitelist entries for RubyBoolean.

f4be029

jglick mentioned this pull request Jan 12, 2018

[JEP-200] Serialize String not JSONObject jenkinsci/saltstack-plugin#116

Merged

More ruby-runtime.

5f44ee2

This was referenced Jan 12, 2018

[JEP-200] POM updates jenkinsci/nexus-platform-plugin#16

Closed

[JENKINS-49506] Whitelisting classes used by BuildImageStepExecution.BuildImageTask jenkinsci/kubernetes-pipeline-plugin#66

Merged

oleg-nenashev approved these changes Jan 12, 2018

View reviewed changes

https://github.com/jenkinsci/kubernetes-pipeline-plugin/pull/66

e3bf773

jglick mentioned this pull request Jan 12, 2018

[JEP-200] Whitelist PackId for use from IdentifyPackageCallable jenkinsci/crx-content-package-deployer-plugin#8

Merged

jglick added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jan 12, 2018

Fixed order of entries.

47be7c3

jglick merged commit 47be7c3 into jenkinsci:master Jan 12, 2018

jglick added a commit that referenced this pull request Jan 12, 2018

[JEP-200] [JENKINS-47736] Merged #3120: ClassFilterImpl

cb4903c

jglick deleted the whitelist-JENKINS-47736 branch January 12, 2018 22:41

jglick mentioned this pull request Jan 12, 2018

[JEP-200] Marked as Final, and recording some late changes jenkinsci/jep#45

Merged

zackliu mentioned this pull request Jan 16, 2018

Add ClassFilter as Jenkins changed to white list jenkinsci/azure-vm-agents-plugin#98

Merged

This was referenced Jan 16, 2018

Some more refinements to ClassFilterImpl shutoff during development #3237

Merged

Test: RobustReflectionConverter gracefully loads config fields now blocked by JEP-200 #3247

Merged

basil reviewed Sep 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist #3120

[JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist #3120

jglick commented Oct 30, 2017 •

edited

Loading

jglick commented Nov 28, 2017

jtnord left a comment

jtnord Dec 5, 2017

jglick Dec 5, 2017

jtnord Dec 5, 2017

jglick Dec 5, 2017

jtnord Dec 5, 2017

jglick Dec 5, 2017

jtnord Dec 5, 2017

jglick Dec 5, 2017

oleg-nenashev Dec 14, 2017

jtnord Dec 5, 2017

jglick Dec 5, 2017

jtnord Dec 5, 2017

jglick Dec 5, 2017

jtnord Dec 5, 2017

jglick Dec 5, 2017

jtnord Dec 5, 2017

jglick Dec 5, 2017

oleg-nenashev left a comment

oleg-nenashev Jan 12, 2018

allxiao commented Jan 15, 2018

oleg-nenashev commented Jan 15, 2018 •

edited

Loading

oleg-nenashev commented Jan 15, 2018

allxiao commented Jan 16, 2018 •

edited

Loading

basil Sep 14, 2022

jglick Sep 14, 2022

[JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist #3120

[JENKINS-47736] Switch Remoting/XStream blacklist to a whitelist #3120

Conversation

jglick commented Oct 30, 2017 • edited Loading

Proposed changelog entries

jglick commented Nov 28, 2017

jtnord 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allxiao commented Jan 15, 2018

oleg-nenashev commented Jan 15, 2018 • edited Loading

oleg-nenashev commented Jan 15, 2018

allxiao commented Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Oct 30, 2017 •

edited

Loading

oleg-nenashev commented Jan 15, 2018 •

edited

Loading

allxiao commented Jan 16, 2018 •

edited

Loading