commons-jxpath
commons-jxpath copied to clipboard
apache
Add an allow list for classes that can be loaded by JXPath
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.
Remote Code Execution (RCE)
-
Issue reported here is, all functions in the class JXPathContext (except compile and compilePath) are vulnerable to a remote code execution attack when used with untrusted input.
-
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());
-
No workaround available
The fix added here is via a new system property "jxpath.class.allow". This list is empty by default meaning that nothing is allowed by default. Users should then explicitly configure which classes are allowed.
- 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 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.allow"
- System property "jxpath.class.allow" can be set to specify the list of allowed 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 disallows all classes.
- 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.
This is based on #25 and the logic is changed from a deny list into an allow list
@kyakdan I'm not sure what happened on your rebase but you should not have to edit pom.xml or changes.xml.
@madrob @garydgregory Thanks for the review.
@garydgregory I've rebased again and the PR does not contain changes to pom.xml or changes.xml now.
@Warxim Many thanks for the detailed explanation and the PoC of still insecure methods. I've added checks to the MethodFunciton class that checks the declaring class before invoking the method. A unit test for the use case you described is added to verify the fix. Could you give it a try now and give feedback if you find further cases that are not yet secured?
@garydgregory @Warxim @madrob I've address your review. Could you have look?
I might take time to review this weekend but it could be a few weeks before a release candidate is ready.
@garydgregory Thanks in advance for the feedback. Any estimation of when you would be able to have a look?
@garydgregory Apologies for being so blunt, but don't you think a few weeks might be a bit too long for such a high-severity issue? Maybe I misunderstood the ease of exploiting it, but it seems like quite a nasty vulnerability.
If releasing a 1.3.1 with this fix only would speed up review and release procedures then I support this. A version 1.4 with this fix and all other already applied changes could be done later.
On Thu, Nov 3, 2022 at 8:43 AM JanHron @.***> wrote:
@garydgregory https://github.com/garydgregory Apologies for being so blunt, but don't you think a few weeks might be a bit too long for such a high-severity issue? Maybe I misunderstood the ease of exploiting it, but it seems like quite a nasty vulnerability.
In short: no. I am a volunteer here, so I don't get paid, I have other priorities and interests; I work here on a best-effort basis based on my POV. This is the classic story of free and open-source development.
Gary
— Reply to this email directly, view it on GitHub https://github.com/apache/commons-jxpath/pull/26#issuecomment-1302042765, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJB6N7HH3D4T2GGHHSFPADWGOXP3ANCNFSM6AAAAAARFNKCMY . You are receiving this because you were mentioned.Message ID: @.***>
We should clarify if the Whitelist on classes includes inherited methods.
Be careful with default Whitelist, String.wait() and .class/.getClass() (or generally all methods inherited from object?) should probably not be included. Similar Integer/Boolean.getProperty
We should clarify if the Whitelist on classes includes inherited methods.
Be careful with default Whitelist, String.wait() and .class/.getClass() (or generally all methods inherited from object?) should probably not be included. Similar Integer/Boolean.getProperty
Good point! The current implementation does not allow any classes if not otherwise configured by the user. We can merge and release this variant so that users can use get a secure version of the library. A default secure allow-list can be done in a follow-up update.
In short: no. I am a volunteer here, so I don't get paid, I have other priorities and interests; I work here on a best-effort basis based on my POV. This is the classic story of free and open-source development. Gary
@garydgregory No need to get all defensive, I did not want to start one of those discussions where a random nobody demands attention from a volunteer member of an open source project. I'm well aware of the situation and your work is very much appreciated. Apologies for the misunderstanding. My background is in security, so my expectations about what should happen whenever a critical vulnerability is found may be skewed a bit in favor of immediate action.
So, do I understand correctly that this vulnerability is as serious as it sounds - that unsanitized user input into a JXPath expression leads to getting a remote shell, or is there some caveat I'm missing? If it's really this dangerous, would you please consider making a HotFix release of 1.3 with just this fix so that the users of commons-jxpath can patch their projects (many of them open-source too, BTW) and sleep well at night?
And it's not like I'm just asking you to do more unpaid work - I'm willing to help too! If there's anything I can do to help move this along, just let me know.
So, do I understand correctly that this vulnerability is as serious as it sounds - that unsanitized user input into a JXPath expression leads to getting a remote shell
As I understand it, it is about untrusted expressions not about untrusted payload instances or object trees. But I could be wrong, Is it both?
CVE-2022-41852 was issued in error and is in the process of being updated so the state becomes "rejected" / "disputed" / "issued in error" or whatever is the most appropriate state. There is no security vulnerability to be fixed here. JEXL is NOT expected to safely handle untrusted input. Whether a better set of defaults can be provided to help users avoid shooting themselves in the foot is a different question. And a reasonable aim for this or any other PR. Please update the title of this PR to something more appropriate that does not suggest that a security vulnerability exists here.
We do not agree here. Xpath injections are pretty common, see https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=xpath. The past shows that a lot of applications from well-known companies were vulnerable to XPath injections when an attacker was able to control the XPath expression completely or partially. Sanitization of user input failed in multiple cases. It might be expected by developers that the worst-case scenario resulting from an XPath injection would be disclosing sensitive information, but no one would usually expect arbitrary code execution. In Apache Commons JXPath it is enough to have partial control of the XPath expression to call methods from an arbitrary class. @Warxim has demonstrated how easily an exploit might work, see https://hackinglab.cz/en/blog/remote-code-execution-in-jxpath-library-cve-2022-41852/. As a maintainer of a popular open-source library, you have no control over how users might use the library.
We would appreciate it if apache-commons would take more responsibility here and start notifying users about the potential security risks related to untrusted input vectors instead of merely saying "library is not intended to parse untrusted input". The CVE description is valid since it clearly says "Those using JXPath to interpret untrusted XPath expressions may be vulnerable to a remote code execution attack". We are not arguing about an internal unexposed functionality here, but about an actual feature of the library that can have serious security implications.
If you don't plan to assign CVEs for issues resulting from untrusted user input or plan to reject valid ones, you should consider adding a clear warning on the official project page along the lines: "Attention: The library is not intended to be used with untrusted input. Untrusted input could lead to Remote Code Execution, Denial of Service or other unexpected behavior". That would help developers to be aware of the potential security risks or consider switching to other libraries that are robust against untrusted input data. We also suggest adding such a warning to all apache-commons libraries that are not intended to be used with untrusted user input. Some small notes in the documentation are not enough to raise the required attention. In the case of JXpath, neither the current project page (https://github.com/apache/commons-jxpath) nor the user guide (https://commons.apache.org/proper/commons-jxpath/users-guide.html) contains any information that JXPath cannot safely handle untrusted input.
JEXL is NOT expected to safely handle untrusted input.
Be careful, this is about xpath, and also even Jexl should handle data securely, it’s just a question if expressions/programs should be trusted or not.
Not sure why there are some comments on JEXL which is a different library (commons-jexl) than commons-jxpath. The PR is for JXPath. But maybe I do not get the full picture.
Yes, untrusted input could also be handled by the applications which are using commons-jxpath. But that would put the burden on hundreds of applications comming up with solutions on their own. And still they would not be sure if they did it correctly.
The PR presented here makes jxpath secure by default - and hence should be shipped as soon as possible with high priority from my point of view.
Sorry about the JEXL / JXPath mix-up. Trying to respond to lots of similar issues about projects with similar names.
The CVE record has been updated to invalid so my request to edit the title of this PR to remove the CVE reference stands.
My previous comments stand for JXPath. It is an expression language intended to manipulate (quoting from the project description) "graphs of objects of all kinds: JavaBeans, Maps, Servlet contexts, DOM etc, including mixtures thereof." It is not reasonable to expect a tool designed for that purpose to safely handle untrusted input unless using an API explicitly documented to do so. Hence this is not a security issue and will not be treated as such.
Treating an application's XPath injection vulnerability as a issue in JXPath is equivalent to calling an application's SQL injection vulnerability an issue in the JDBC driver. The vulnerability is in the application, not the library the application is (ab)using.
I have concerns about a filter approach for JXPath since experience across multiple projects has shown that trying to limit such tools to a subset of safe operations is difficult and typically leads to a long series issues being reported as researchers find loopholes that allow the filtering to be bypassed. However, those approaches often used deny lists whereas this PR uses an allow list so it is starting from a much safer position.
I wonder if a JXPath equivalent (and it could be a very loose equivalent) to prepared statements in the SQL world could be useful.
Generally, if the community consensus is to add some form of feature to handle unsafe input then it needs to be clearly documented for that purpose and the community has to be prepared to handle security vulnerability reports against that feature.
@markt-asf I think a reasonable approach would be to make the default methods safe and provide an unsecured alternative for people who really need this functionality, with a big fat warning about injection risk. Thus, the casual users would be protected and the power users would still have all the tools they need. I like your idea about prepared statements too.
The CVE record has been updated to invalid so my request to edit the title of this PR to remove the CVE reference stands.
For clarity, this is not true. The record has been marked DISPUTED which is entirely different to REJECT per the CVE website. (https://www.cve.org/ResourcesSupport/FAQs) Disputed is when there is disagreement between a vendor and security researcher so CVE Program leaves the record as is and updates the status. Rejected is when the record was placed in error and is not a vulnerability and the record could potentially be returned to the reserved pool in the future. The CVE is currently still valid and tied to the vulnerability just with the updated status so people can further research the vulnerability themselves.
It is true that, similarly to SQL injections, Xpath injections are vulnerabilities in the applications that use the library. However, the application's responsibility ends when the library exposes functionality that enables propagating a vulnerability to other system components. In the case of SQL injections, the scope matters. If a crafted SQL payload would result in an injection that allows direct code execution outside the database context, we should treat this case as a security issue. There are multiple examples where libraries enabled code execution by mistake or as a feature. Those issues were addressed and fixed. Here are a few SQL examples:
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-4019 (MySQL)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-3095 (SQL engine IBM DB2
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-4061 (Microsoft SQL Server)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-1889 (Big SQL in IBM InfoSpere)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3414 (SQLite)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3415 (SQLite)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3416 (SQLite)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-2520 (Apple sqllite)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-2519 (Apple sqllite)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-2518 (Apple sqllite)
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-2513 (Apple sqllite)
Recently, Jazzer found an issue of a similar nature in OSS-Fuzz in a HyperSQL (https://nvd.nist.gov/vuln/detail/CVE-2022-41853). The maintainer quickly acknowledged the security risk and agreed to issue a CVE and a fix release. Based on the previous arguments, we still think this is a security issue for which a CVE should be issued.
That said, I'll adjust the PR title to get it merged. Our goal here is to ensure that users get a secure version of the library and the PR title seems to be a blocker for this to happen.
Now as @kyakdan has renamed the PR's title to "get it merged" - is there a plan / schedule when this will be done and a new version with this included will be released? It would be good if that could be communicated. I am pretty sure there a several projects which are waiting for a release. I hope it will be done very soon as otherwise we need to replace JXPath in our project as we are not allowed to use libs with known security issues with that high criticality.
There is no security vulnerability. This PR will be dealt with with the same priority as any other enhancement request.
The CVE record has been updated to invalid so my request to edit the title of this PR to remove the CVE reference stands.
For clarity, this is not true. The record has been marked DISPUTED which is entirely different to REJECT per the CVE website. (https://www.cve.org/ResourcesSupport/FAQs) Disputed is when there is disagreement between a vendor and security researcher so CVE Program leaves the record as is and updates the status. Rejected is when the record was placed in error and is not a vulnerability and the record could potentially be returned to the reserved pool in the future. The CVE is currently still valid and tied to the vulnerability just with the updated status so people can further research the vulnerability themselves.
DISPUTED is the wrong state for these issues, then, they should be REJECT as they were placed in error, are not a vulnerability (as well as against CNA rules at the time of assignment).
The CVE record has been updated to invalid so my request to edit the title of this PR to remove the CVE reference stands.
For clarity, this is not true. The record has been marked DISPUTED which is entirely different to REJECT per the CVE website. (https://www.cve.org/ResourcesSupport/FAQs) Disputed is when there is disagreement between a vendor and security researcher so CVE Program leaves the record as is and updates the status. Rejected is when the record was placed in error and is not a vulnerability and the record could potentially be returned to the reserved pool in the future. The CVE is currently still valid and tied to the vulnerability just with the updated status so people can further research the vulnerability themselves.
DISPUTED is the wrong state for these issues, then, they should be REJECT as they were placed in error, are not a vulnerability (as well as against CNA rules at the time of assignment).
DISPUTED is the correct state since there is a dispute between the researcher and the maintainer. They CVE was not placed in error since obviously the researcher and others here do consider it a vulnerability. The DISPUTED tag tells individuals to research the issue; which they should do because this is an issue that could allow RCE in an application. At the very least having the CVE brings to light that there is no warning in the library to not pass untrusted input and encourages individuals to implement their own controls if the package maintainer will not.
@markt-asf I understand if this is not in the threat model for JXPath. However, I can't seem to find any documentation explicitly stating JXPath should only be used to process trusted input, which could lead to user confusion and incorrect usages (which includes our fuzzing efforts). Would it make sense to update the documentation to make this fact very clear to users?
I still think this PR as is provides a lot of values for users. Perhaps once this gets merged, we could also update documentation on how this can be used to provide users with a safe way of using JXPath on potentially untrusted inputs?
OK, just saw that CVE-2022-41852 is marked as REJECTED: https://nvd.nist.gov/vuln/detail/CVE-2022-41852
OK, just saw that CVE-2022-41852 is marked as REJECTED: https://nvd.nist.gov/vuln/detail/CVE-2022-41852
I just saw this too. I disagree with this and think it should have remained DISPUTED but I'm not the CNA. At a minimum the documentation should be updated to highlight the risks of using this library and include that it should not accept any untrusted input. I think it's irresponsible to argue the CVE to be REJECTED without first updating the documentation since new users of the library may not be aware of this issue now.
Agreed, it needs to be documented. Either as a warning that it is not possible to have untrusted expressions or with a pointer to the filter mechanism discussed here. I will propose something.