Fix for Issue: CVE-2022-41852 (CVE has been rejected) #25
Conversation
CVE-2022-41852 # Issue Description: Those using JXPath to interpret untrusted XPath expressions may be vulnerable to a remote code execution attack. All JXPathContext class functions processing a XPath string are vulnerable except compile() and compilePath() function. The XPath expression can be used by an attacker to load any Java class from the classpath resulting in code execution. # CVSS Version 3.x: 9.8 Critical # Affected versions: 1.3 and earlier # Vulnerability Type: Remote Code Execution (RCE) # Steps to Reproduce: - Issue reported here is, all functions in the class JXPathContext (except compile and compilePath) are vulnerable to a remote code execution attack. - An arbitrary code can be injected in the xpath values passed to these functions, and it allows triggering java classes that can exploit the target machine. - For instance, the iterate() method in the JXPathContext class, can be invoked by passing the xpath argument value as, java.lang.Thread.sleep(9999999) or java.lang.Class.forName("ExploitTest"). These examples can result in triggering the injected java code, and can exploit the target machine. - Example: JXPathContext context = JXPathContext.newContext(new Test() ); Iterator result = context.iterate("java.lang.Thread.sleep(9999999)"); System.out.println("result.hasNext() - " + result.hasNext()); # Workarounds: - No workaround available # Fix: Please note that, the fix added here is via a new system property (enabling this property, will address the issue). This will ensure that we are not breaking functionality for existing users. - In order to fix this issue, a filter class "JXPathFilter.java" is added in JXPath. This filter object will be used to validate if the xpath passed to JXPath is a valid one - This new filter class, for now implements JXPathClassFilter interface, which is used to check the java classes passed in xpath. In future, we could implement any other interface, to verify other cases as well. - In this filter, the class filter validates to see if the class being loaded in JXPath is part of the restriction list. The restriction can be configured via a system property "jxpath.class.deny" - System property "jxpath.class.deny" can be set to specify the list of restricted classnames. This property takes a list of java classnames (use comma as separator to specify more than one class). If this property is not set, it exposes any java class to javascript Example: jxpath.class.deny=java.lang.Runtime will deny exposing java.lang.Runtime class via xpath, while all other classes will be exposed. - When a new extension function is created by JXPath, it uses the filter to check if the xpath supplied is a valid string, only then function instances are created. - The changes are added to pass JXPathFilter instance as an additional argument to the methods that create the ExtensionFunction objects. In addition, the ClassLoaderUtil class is modified to use the JXPathFilter instance when a new Class instance is created. Please note that, the changes here are added as overloaded methods, so that we are not affecting existing code for anyone. # Unit Testing: - Verified that the RCE vulnerability is no more applicable for any of the functions in the JXPathContext class. - Verified different code areas and found that the fix is addressed in all cases. Testing covers Class Functions, Package Funcions, Function Libraries and functions exposed in JXPathContext. - All the test cases for this change is covered in unit test code: in ExtensionFunctionTest.java. 10 new test methods are added in this class, to make sure all possible areas are covered. # Summary: - The changes added in the pull request, takes effect only if the newly added System property "jxpath.class.deny" is set. This ensures that existing users of jxpath are not affected by this change. - The new system property can be configured based on the environment and user needs. - In order to fix CVE-2022-41852, we need to configure "jxpath.class.deny=java.lang.Class" which will ensure that Class.forName() call is blocked when passed via xpath strings.
bhmohanr-techie
changed the title
|
Is anyone following up? |
@Paradox98 I was trying to reachout via Apache dev community to get this PR reviewed, but my subscription request to dev-community is still pending. So, I couldn't find a reviewer yet for this PR. Thanks. |
|
@garydgregory @markt-asf @jvz @kinow @Paradox98 Dear Apache JXPath Maintainers, I'm requesting your review for this PR #25 which will fix the recent reported vulnerability CVE-2022-41852. I have also raised a JIRA ticket for the same, JXPATH-201 I also see one other PR raised for the same. The other PR #26 is raised on top of this PR raised by me, with one minor change that I have explained below.
Also, I have contributed a similar fix in one other opensource project (where I have used the same logic to introduce a deny list to prevent Remote Code Execution vulnerability). My contribution in that project was reviewed and approved by the maintainers, Please refer: mock-server/mockserver#1466 Please review the approach in my PR #25 and let me know your thoughts. I'm open to any suggestions or feedback from you, Thankyou. |
|
It looks like your commit didn't pass ci review. |
@Paradox98 Yes, the ci review failed here, since JAVA_HOME variable is not defined in the machine. But, this error is not related to the changes added in this PR, but it exists even in some of the earlier PRs as well. Please let me know if there is a way to define the JAVA_HOME variable in the ci environment, Thanks. |
|
@bhmohanr-techie thanks for your effort, but we highly suggest to use the whitelist approach in #26. The main problem is that the default behaviour of the library is not changing. That means if users will just update the jxpath jar file they'll still be vulnerable and they'll need to set a System property in addition. The next problem is that blacklisting individual components is not feasible for large projects. For example, if your project has 100 classes and you just want to allow access to one class you will need to set a system property for 99 classes e.g. Furthermore, I have checked that "jxpath.class.deny=java.lang.Class" will not stop exploitation of CVE-2022-41852 as described by you, feel free to verify with xpath string "java.lang.Thread.sleep(1000000)". For those reasons, we think that the allowlist is the better approach to go forward. |
@0roman I'm sorry to say that I still don't agree to your statement... In the PR #26 you are changing the default behavior of jxpath... I would request you think from a user perspective, as a user if I move to a latest version of jxpath, the bare minimum thing that I expect is there should be nothing breaking in existing behavior. Having said that, with changes added in PR #26, if I do an upgrade, jxpath will not work for me, unless I set the newly added system property (this applies even if I'm not affected by this vulnerability). The next problem is that with your approach of whitelisting individual components is not feasible for large projects. For example, if your project has 100 classes and you just want to deny access to one class you will need to set a system property for 99 classes e.g. System.setProperty("jxpath.class.allow", "com.example1, com.example2, com.example1, ..."); There is no hard restriction in jxpath to this, we should be capable of supporting jxpath to any kind of user (may it be a user allowing only a single class, or a user who allows jxpath to access a bunch of his trusted classes) "jxpath.class.deny=java.lang.Class" will stop exploitation of CVE-2022-41852 with xpath string "java.lang.Thread.sleep(1000000)". This is already added in my unit test code as well which was copied in your PR #26 as well, I would request you to check this again. So, I still think deny list is the best approach... As I mentioned earlier, a similar vulnerability was fixed by me in one other opensource component, PR: mock-server/mockserver#1466. Over there, I had used deny list approach, which was review, approved and merged in their main release as well. So, this is an accepted approach in other opensource packages as well. So, please let JXPath maintainers review the approach and decide the best suited one. We should take their valuable suggestion/feedback. Thanks. |
|
@bhmohanr-techie With the deny list approach, users do not get any protection whatsoever if they don't change their configurations. This means they stay insecure by default. In both approaches, you have to adjust your default configuration in order to be protected. Since our main goal here is to protect users who use the library, making it necessary to change the configuration by explicitly mentioning which classes are allowed is the best way to guarantee that they will not be vulnerable. Moreover, with a denylist, you can always forget/miss some dangerous classes. It is much harder to know all insecure classes to deny than to know which classes you trust. I think that while backward compatibility is great, protecting users by default takes precedence. |
|
I'm still not convinced, Please help me understand one thing. Let's say I'm a JXPath user, and I'm not affected this vulnerability. JXPath is working all fine in my environment. Now, if we go ahead with you approach of specifying an allow list, then if I upgrade to the next JXPath version, then JXPath will stop working in my environment straight after upgrade, and I will need to set a system property to get it working again. This is an unnecessary ask from a user, who didn't have any problem with JXPath so far. We are affecting existing user behavior, which to me is not acceptable for any product/solution. So, let please wait for JXPath maintainers on this, we should stick to what they suggest? They have better understanding of the JXPath code base and customers. @garydgregory @markt-asf @jvz @kinow @Paradox98 requesting your valuable suggestions here. Thanks. |
|
Please note it is highly likely that all the CVEs issued by Google / oss-fuzz for JXPath without consultation with the ASF and in breach of the rules for CNAs will be resolved as invalid. Separately, if JXPath opts to provide a feature or features to support users who wish to process untrusted input without validation or sanitisation then a deny list would never be acceptable. A possible approach would be an allow list that defaults to everything that users could then narrow if they wish. For the avoidance of doubt, my current position is that JXPath is intended to be used with trusted input. I haven't performed an in-depth review of JXPath, so if anyone is aware of reasons why JXPath should be expected to handle untrusted input safely, please speak up. |
|
Thanks for contributing! I strongly prefer the allow list approach in #26 and will leave specific code feedback in that PR. However, that PR wouldn't have been possible without your work here! |
|
Hello @bhmohanr-techie
|
|
Please fix the issue with CVS 2022-41852. |
Note that the CVE has been rejected. |
markt-asf
changed the title
|
If someone is interested in the rejected issue, have a look at https://github.com/Warxim/CVE-2022-41852. There is also some interesting discussion in #26. |
Issue ID:
Issue Description:
Those using JXPath to interpret untrusted XPath expressions may be vulnerable to a remote code execution attack. All JXPathContext class functions processing a XPath string are vulnerable except compile() and compilePath() function. The XPath expression can be used by an attacker to load any Java class from the classpath resulting in code execution.
CVSS Version 3.x:
9.8 Critical
Affected versions:
1.3 and earlier
Vulnerability Type:
Remote Code Execution (RCE)
Steps to Reproduce:
JXPathContext context = JXPathContext.newContext(new Test() );
Iterator result = context.iterate("java.lang.Thread.sleep(9999999)");
System.out.println("result.hasNext() - " + result.hasNext());
Workarounds:
Fix:
Please note that, the fix added here is via a new system property (enabling this property, will address the issue). This will ensure that we are not breaking functionality for existing users.
Unit Testing:
Summary: