Uploaded image for project: 'ActiveMQ'
  1. ActiveMQ
  2. AMQ-6013

Restrict classes that can be serialized in ObjectMessages

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 5.12.0
    • Fix Version/s: 5.11.3, 5.13.0, 5.12.2
    • Component/s: None
    • Labels:
      None

      Description

      At some points we do (de)serialization of JMS Object messages inside the broker (HTTP, Stomp, Web Console, ...). We need to restrict classes that can be serialized in this way.

        Issue Links

          Activity

          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a7e2a44fe8d4435ae99532eb0ab852e6247f7b16 in activemq's branch refs/heads/master from Dejan Bosanac
          [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=a7e2a44 ]

          https://issues.apache.org/jira/browse/AMQ-6013 - restrict classes which can be serialized inside the broker

          Show
          jira-bot ASF subversion and git services added a comment - Commit a7e2a44fe8d4435ae99532eb0ab852e6247f7b16 in activemq's branch refs/heads/master from Dejan Bosanac [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=a7e2a44 ] https://issues.apache.org/jira/browse/AMQ-6013 - restrict classes which can be serialized inside the broker
          Hide
          dejanb Dejan Bosanac added a comment - - edited

          The classes are restricted by default to the following packages

          java.lang
          java.util
          org.apache.activemq
          org.fusesource.hawtbuf
          com.thoughtworks.xstream.mapper

          which are needed for normal functioning of http and stomp packages. If you need to send object messages via http, you need to add desired packages. You can do that with by using org.apache.activemq.SERIALIZABLE_PACKAGES system property. For example:

          -Dorg.apache.activemq.SERIALIZABLE_PACKAGES="java.lang,java.util,org.apache.activemq,org.fusesource.hawtbuf,com.thoughtworks.xstream.mapper,com.mycompany.myapp"
          Show
          dejanb Dejan Bosanac added a comment - - edited The classes are restricted by default to the following packages java.lang java.util org.apache.activemq org.fusesource.hawtbuf com.thoughtworks.xstream.mapper which are needed for normal functioning of http and stomp packages. If you need to send object messages via http, you need to add desired packages. You can do that with by using org.apache.activemq.SERIALIZABLE_PACKAGES system property. For example: -Dorg.apache.activemq.SERIALIZABLE_PACKAGES= "java.lang,java.util,org.apache.activemq,org.fusesource.hawtbuf,com.thoughtworks.xstream.mapper,com.mycompany.myapp"
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e100638244c4ca5eb2a1f16bcdc671c9859c2694 in activemq's branch refs/heads/master from Dejan Bosanac
          [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=e100638 ]

          https://issues.apache.org/jira/browse/AMQ-6013 - init serializable packages statically

          Show
          jira-bot ASF subversion and git services added a comment - Commit e100638244c4ca5eb2a1f16bcdc671c9859c2694 in activemq's branch refs/heads/master from Dejan Bosanac [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=e100638 ] https://issues.apache.org/jira/browse/AMQ-6013 - init serializable packages statically
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6f03921b31d9fefeddb0f4fa63150ed1f94a14b1 in activemq's branch refs/heads/activemq-5.11.x from Dejan Bosanac
          [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=6f03921 ]

          https://issues.apache.org/jira/browse/AMQ-6013 - restrict classes which can be serialized inside the broker

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6f03921b31d9fefeddb0f4fa63150ed1f94a14b1 in activemq's branch refs/heads/activemq-5.11.x from Dejan Bosanac [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=6f03921 ] https://issues.apache.org/jira/browse/AMQ-6013 - restrict classes which can be serialized inside the broker
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 73a0caf758f9e4916783a205c7e422b4db27905c in activemq's branch refs/heads/activemq-5.11.x from Dejan Bosanac
          [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=73a0caf ]

          https://issues.apache.org/jira/browse/AMQ-6013 - init serializable packages statically

          Show
          jira-bot ASF subversion and git services added a comment - Commit 73a0caf758f9e4916783a205c7e422b4db27905c in activemq's branch refs/heads/activemq-5.11.x from Dejan Bosanac [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=73a0caf ] https://issues.apache.org/jira/browse/AMQ-6013 - init serializable packages statically
          Hide
          djencks David Jencks added a comment -

          I'd expect you'd want to check if the class is allowed based on its name before loading it in case it has static initialization code that does something unwanted.
          Loading the allowed packages list only once might be considerably more efficient.

          Show
          djencks David Jencks added a comment - I'd expect you'd want to check if the class is allowed based on its name before loading it in case it has static initialization code that does something unwanted. Loading the allowed packages list only once might be considerably more efficient.
          Hide
          dejanb Dejan Bosanac added a comment -

          Hi David,
          yes, it makes sense to check security only on the class name without loading it. Thanks for that, I'll fix it on Monday. I'll need to check how XStream is doing it and if this problem exists there as well.

          As for loading the list only once, it's done by the second commit so that should be in there already. Let me know if you thought of something else, or I missed something.

          Show
          dejanb Dejan Bosanac added a comment - Hi David, yes, it makes sense to check security only on the class name without loading it. Thanks for that, I'll fix it on Monday. I'll need to check how XStream is doing it and if this problem exists there as well. As for loading the list only once, it's done by the second commit so that should be in there already. Let me know if you thought of something else, or I missed something.
          Hide
          dejanb Dejan Bosanac added a comment -

          Hi David,

          I looked at this some more and I don't think we have a problem. If you take a look at

          https://github.com/apache/activemq/blob/master/activemq-client/src/main/java/org/apache/activemq/util/ClassLoadingAwareObjectInputStream.java#L125

          we load the class without initializing it (second parameter is false), so the static code is not executed until the class if used the first time.

          Show
          dejanb Dejan Bosanac added a comment - Hi David, I looked at this some more and I don't think we have a problem. If you take a look at https://github.com/apache/activemq/blob/master/activemq-client/src/main/java/org/apache/activemq/util/ClassLoadingAwareObjectInputStream.java#L125 we load the class without initializing it (second parameter is false), so the static code is not executed until the class if used the first time.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d7a3b9406b8496c3f1508bebf3c7ff5367374b90 in activemq's branch refs/heads/master from Gary Tully
          [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=d7a3b94 ]

          https://issues.apache.org/jira/browse/AMQ-6029 - make certs available to tunnle servlet - HttpsNeedClientAuthSendAndReceiveTest regression, add javax.security so login exceptions can propogate over http - https://issues.apache.org/jira/browse/AMQ-6013

          Show
          jira-bot ASF subversion and git services added a comment - Commit d7a3b9406b8496c3f1508bebf3c7ff5367374b90 in activemq's branch refs/heads/master from Gary Tully [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=d7a3b94 ] https://issues.apache.org/jira/browse/AMQ-6029 - make certs available to tunnle servlet - HttpsNeedClientAuthSendAndReceiveTest regression, add javax.security so login exceptions can propogate over http - https://issues.apache.org/jira/browse/AMQ-6013
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e7a4b53f799685e337972dd36ba0253c04bcc01f in activemq's branch refs/heads/activemq-5.12.x from Dejan Bosanac
          [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=e7a4b53 ]

          https://issues.apache.org/jira/browse/AMQ-6013 - restrict classes which can be serialized inside the broker

          Show
          jira-bot ASF subversion and git services added a comment - Commit e7a4b53f799685e337972dd36ba0253c04bcc01f in activemq's branch refs/heads/activemq-5.12.x from Dejan Bosanac [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=e7a4b53 ] https://issues.apache.org/jira/browse/AMQ-6013 - restrict classes which can be serialized inside the broker
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7eb9b218b2705cf9273e30ee2da026e43b6dd4e0 in activemq's branch refs/heads/activemq-5.12.x from Dejan Bosanac
          [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=7eb9b21 ]

          https://issues.apache.org/jira/browse/AMQ-6013 - init serializable packages statically

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7eb9b218b2705cf9273e30ee2da026e43b6dd4e0 in activemq's branch refs/heads/activemq-5.12.x from Dejan Bosanac [ https://git-wip-us.apache.org/repos/asf?p=activemq.git;h=7eb9b21 ] https://issues.apache.org/jira/browse/AMQ-6013 - init serializable packages statically
          Hide
          3riverdev Brett E. Meyer added a comment -

          Is there any explanation available for why this change was made? Why are the restrictions necessary?

          Show
          3riverdev Brett E. Meyer added a comment - Is there any explanation available for why this change was made? Why are the restrictions necessary?
          Hide
          mbechler Moritz Bechler added a comment -

          Btw, you might want to have a look at your own org.apache.activemq.web.WebClient (as this is still on the whitelist). Have not checked under which conditition the static factory gets initialized exactly (it seems to be for the web console), but one can certainly do some mischief (I'd say mainly DOS) if it is.

          I'd also suggest that you announce this change in a much more prominent way (maybe even get a CVE for it), as it both has very serious security implications if it goes unpatched and also very well might break some peoples code. And you should also make it very clear that one should be very careful what to add to the whitelist.

          To answer Brett (as a third party):
          Java deserialization on not completely trusted input is inherently dangerous. The amount of code reachable by just deseralizing some input is insane. There are many instances where developers are careless (or even simply don't care) what can be done with their deserialization routines (also there can be nasty interactions between different pieces of code) and the default deserialization routine allows one to use anything you have on your classpath. We have seen three major libraries contain code that leads to remote arbitrary code execution. And there are more to come.
          Imho, we really need to either fix the primitive or drop it from all the specs that are/allow using it in potentially dangerous way.

          Show
          mbechler Moritz Bechler added a comment - Btw, you might want to have a look at your own org.apache.activemq.web.WebClient (as this is still on the whitelist). Have not checked under which conditition the static factory gets initialized exactly (it seems to be for the web console), but one can certainly do some mischief (I'd say mainly DOS) if it is. I'd also suggest that you announce this change in a much more prominent way (maybe even get a CVE for it), as it both has very serious security implications if it goes unpatched and also very well might break some peoples code. And you should also make it very clear that one should be very careful what to add to the whitelist. To answer Brett (as a third party): Java deserialization on not completely trusted input is inherently dangerous. The amount of code reachable by just deseralizing some input is insane. There are many instances where developers are careless (or even simply don't care) what can be done with their deserialization routines (also there can be nasty interactions between different pieces of code) and the default deserialization routine allows one to use anything you have on your classpath. We have seen three major libraries contain code that leads to remote arbitrary code execution. And there are more to come. Imho, we really need to either fix the primitive or drop it from all the specs that are/allow using it in potentially dangerous way.
          Hide
          3riverdev Brett E. Meyer added a comment - - edited

          Fair enough on the reasoning – makes sense. However, I'd definitely agree that this fix needs to be announced through several channels, since it's a breaking change for many users.

          Also consider supporting additional means to set org.apache.activemq.SERIALIZABLE_PACKAGES (other than a system property), perhaps through a ActiveMQComponent property, etc.

          Additionally, provide logging on startup that communicates the restriction and how to customize it.

          Show
          3riverdev Brett E. Meyer added a comment - - edited Fair enough on the reasoning – makes sense. However, I'd definitely agree that this fix needs to be announced through several channels, since it's a breaking change for many users. Also consider supporting additional means to set org.apache.activemq.SERIALIZABLE_PACKAGES (other than a system property), perhaps through a ActiveMQComponent property, etc. Additionally, provide logging on startup that communicates the restriction and how to customize it.
          Hide
          dejanb Dejan Bosanac added a comment -

          Hi Brett, yes we already discussed this on the dev list and decided to tackle this problem. I raised AMQ-6077 for this work and we should address it with 5.13.1 release.

          Show
          dejanb Dejan Bosanac added a comment - Hi Brett, yes we already discussed this on the dev list and decided to tackle this problem. I raised AMQ-6077 for this work and we should address it with 5.13.1 release.
          Hide
          iali Imran Ali added a comment -

          Based on Moritz Bechler comment:
          Can you please confirm if this fix is also based on
          http://cwe.mitre.org/data/definitions/502.html

          for following CVEs
          CVE-2015-8103 and CVE-2015-4852

          http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-8103
          http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-4852

          If so you are right as a note on Vulnerability Notes Database suggest following

          Developers need to re-architect their applications, and should be suspicious of deserialized data from untrusted sources

          Show
          iali Imran Ali added a comment - Based on Moritz Bechler comment: Can you please confirm if this fix is also based on http://cwe.mitre.org/data/definitions/502.html for following CVEs CVE-2015-8103 and CVE-2015-4852 http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-8103 http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-4852 If so you are right as a note on Vulnerability Notes Database suggest following Developers need to re-architect their applications, and should be suspicious of deserialized data from untrusted sources
          Hide
          dejanb Dejan Bosanac added a comment -

          Hi Imran, yes it's the same root of the issue. I'm working on documenting and disclosing everything, so more information will be available in the coming days.

          Show
          dejanb Dejan Bosanac added a comment - Hi Imran, yes it's the same root of the issue. I'm working on documenting and disclosing everything, so more information will be available in the coming days.
          Hide
          mbechler Moritz Bechler added a comment -

          Yes, that's correct.

          There has been some discussion about how to assign CVE's for these issues on oss-security that imho has not really come to a sensible conclusion (these both directly refer to commons-collections, I think we should have a more general one). If you don't want to go through that again, I think refering to CVE-2015-4852 would be most appropriate choice (at least Oracle has suggested that they are okay with it's reuse: http://seclists.org/oss-sec/2015/q4/306).
          Maybe you should check back with the Apache Security Team to see what's their opinion on this.

          Show
          mbechler Moritz Bechler added a comment - Yes, that's correct. There has been some discussion about how to assign CVE's for these issues on oss-security that imho has not really come to a sensible conclusion (these both directly refer to commons-collections, I think we should have a more general one). If you don't want to go through that again, I think refering to CVE-2015-4852 would be most appropriate choice (at least Oracle has suggested that they are okay with it's reuse: http://seclists.org/oss-sec/2015/q4/306 ). Maybe you should check back with the Apache Security Team to see what's their opinion on this.
          Hide
          dejanb Dejan Bosanac added a comment -

          This issue is related to CVE-2015-5254 as described at http://activemq.apache.org/security-advisories.data/CVE-2015-5254-announcement.txt

          Show
          dejanb Dejan Bosanac added a comment - This issue is related to CVE-2015-5254 as described at http://activemq.apache.org/security-advisories.data/CVE-2015-5254-announcement.txt
          Hide
          trejkaz Trejkaz added a comment -

          The commit to fix this makes serializablePackages a public mutable array:

              public static final String[] serializablePackages;
          

          That in itself is not the best idea for security.

          Show
          trejkaz Trejkaz added a comment - The commit to fix this makes serializablePackages a public mutable array: public static final String [] serializablePackages; That in itself is not the best idea for security.

            People

            • Assignee:
              dejanb Dejan Bosanac
              Reporter:
              dejanb Dejan Bosanac
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development