-
Notifications
You must be signed in to change notification settings - Fork 910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARTEMIS-4763 support .class values for non string attributes, propert… #4924
Conversation
Does this need to have validation on allowed class types added? Just wondering if there are any potential security concerns like we recently had with the OpenWire protocol not validating class types. |
In general we have learned through a number of security reports that blindly creating any class instance is usually not the greatest idea. It would be beneficial to at least scope the class created to an instance of an expected type, the test seems to be creating Transformer types to validating that before newInstance somehow would be beneficial. |
The other big one that seems to pop up a lot is java serialization CVEs, basically the same issue... allowing instantiation of Java objects without validation that the type isn't something malicious |
I don't know that it helps, the values in question come from configuration. We have no choice but to trust configuration, i.e: the file system, where our sources live. These are exiting extension points, where the config provides the implementation. Any malicious intervention will implement any required interface if that is enforced. Any allow list gate will have to be configured in some way, probably on the file system. in short, I am not convinced of an interface check being of any great value when the threat is from file system compromise. Having said that, if there is value in the additional check, and I guess the value is that it makes it a little harder (if that makes any difference) it would need to be done before every newInstance of this sort to be effective. The xml parser does the same thing for one, in support of the same use case. Again, it is trusting config. |
I have opened https://issues.apache.org/jira/browse/ARTEMIS-4766 to follow up. |
@gtully - You are right that it may not help much in this case since it's server side config and if you have access to the file system to modify jars on the classpath or update the config it's already likely too late. This is different than the OpenWire CVE where a client could send in the malicious command so they did not need access. I figured it was still worth bringing it up for discussion as I still think it's a good idea to play it safer and make it a bit more strict. There are also 2 other nice things about adding a new interface and requiring a type besides security reasons that I think make it worthwhile.
|
There is a value in defense in depth. Attackers are scanning source code trees looking for unprotected deserialization and then crafting entry points. While the config is stored on a filesystem out of the box —- in the wild files become ConfigMaps, mounts become network mounts, config files are stored in scm, or other CI/CD tool chain, etc etc. |
In regards to a follow up...my opinion is the validation should be done now and not as a follow and included as part of this change. Creating Jiras often leads to things just getting forgotten about and never done and I think this is important enough to not just put it off for later. Matt makes a good point about defense in depth and after thinking about it I would be -1 on this PR without adding some way to validate the class type now. There's been way too many CVEs that have popped up that involve class loading and serialization so no reason to risk it. |
I agree w/ @cshannon here. There should be a setting to support honoring a valid list of package names -- there could even be an out-of-the-box default end-users could use to avoid having to change the allowed package name configuration.. "org.apache.activemq.artemis.divert.transform.custom", etc. At a minimum, this cuts out attack vectors that leverage using problematic classes from 3rd party dependencies. I think this is one of those "cover our bases" type situation where there needs to be a configuration to prevent it in the event of a security event -- even if it is always the end-user's fault. The first question isn't going to be a rational discussion about the facts around what the end-user may have misconfigured, it is going to be -- "Why didn't Artemis have a config to prevent this?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gtully - The updated validation looks good, I like that it's making sure the type is what is expected for all the different configuration spots which I think is nicer vs just catching an exception if we can't instantiate it like it previously did. Also, using classLoader.loadClass()
is good as this prevents initializing the class before the check is done.
on the allow list for package names for extension points |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, have you run the full test suite on this set of changes?
yes! |
thank you for all the feedback |
…ies config of metrics plugins