Add an allow list for classes that can be loaded by JXPath #26

kyakdan · 2022-10-14T17:05:38Z

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

madrob

Thanks for the contribution, we also would need upgrade notes somewhere.

madrob · 2022-10-18T16:35:53Z

src/main/java/org/apache/commons/jxpath/ClassFunctions.java

+ // give chance to ClassFilter to filter out, if present
+ try {
+ if (jxPathFilter != null && !jxPathFilter.exposeToXPath(functionClass.getName())) {
+ throw new ClassNotFoundException(functionClass.getName());


We shouldn't be using exceptions for control flow here, let's throw JXPathException directly.

Good point! Changed to throwing a JXPathException directly.

madrob · 2022-10-18T16:36:08Z

src/main/java/org/apache/commons/jxpath/FunctionLibrary.java

@@ -77,12 +79,30 @@ public Set getUsedNamespaces() {
 */
 public Function getFunction(String namespace, String name,
 Object[] parameters) {
+ return getFunction(namespace, name, parameters, null);


I worry that people will continue calling this method on upgrade and continue to be insecure by default. Instead of passing null, I think a better default would be to pass either the environment-based of a fail-closed (deny all) implementation of JXPathFilter. Same logic applies for other places where we overload the method signatures.

Excellent point. Changed all usages to use a default value of the JXPathFilter that is initialized to the filter set by the system property of the allow list. This guarantees consistent behavior among all method calls.

madrob · 2022-10-18T16:36:24Z

src/main/java/org/apache/commons/jxpath/ri/JXPathClassFilter.java

+ * System property "jxpath.class.allow" can be set to specify the list of allowd 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 no java classes
+ * Ex: jxpath.class.allow=java.lang.Runtime will allow exposing java.lang.Runtime class via xpath, while all other


Should also document the wildcard * for allowing all classes.

madrob · 2022-10-18T16:36:38Z

src/main/java/org/apache/commons/jxpath/ri/JXPathContextReferenceImpl.java

-import org.apache.commons.jxpath.JXPathNotFoundException;
-import org.apache.commons.jxpath.JXPathTypeConversionException;
-import org.apache.commons.jxpath.Pointer;
+import org.apache.commons.jxpath.*;


The project does not appear to use wildcard imports, let's stick with existing conventions for now.

madrob · 2022-10-18T16:36:54Z

src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java

+ * @version $Revision$ $Date$
+ */
+public class JXPathFilter implements JXPathClassFilter {
+ ArrayList<String> allowedClassesList = null;


This could probably be a Set.

madrob · 2022-10-18T16:37:08Z

src/test/java/org/apache/commons/jxpath/ri/model/BeanModelTestCase.java

 // }
 }

+ @Override
+ public void tearDown() throws Exception {
+ super.tearDown();


Very minor, should call super.tearDown at the end of the method not the beginning.

garydgregory

Hello @kyakdan
See scattered comments.

garydgregory · 2022-10-19T11:13:10Z

src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java

+ * For instance, it JXPathClassFilter interface, which is used to check if any restricted java classes are passed via xpath
+ * JXPath uses this filter instance when an extension function instance is created.
+ *
+ * @author bhmohanr-techie


We don't use author tags. Credit it given in the changes.xml file after a PR is merged (to avoid conflicts with other PRs).

garydgregory · 2022-10-19T11:13:27Z

src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java

+ * JXPath uses this filter instance when an extension function instance is created.
+ *
+ * @author bhmohanr-techie
+ * @version $Revision$ $Date$


We don't use Subversion anymore.

Changed to @since 1.4

garydgregory · 2022-10-19T11:14:18Z

src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java

+ *
+ * @author bhmohanr-techie
+ * @version $Revision$ $Date$
+ */


New public and protected elements should have a Javadoc since tag for the next version, which should be 1.4.

garydgregory · 2022-10-19T11:15:17Z

src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java

+ public void init() {
+ String restrictedClasses = System.getProperty("jxpath.class.allow");
+ allowedClassesList = null;
+ if (restrictedClasses != null && restrictedClasses.trim().length() > 0) {


Use String#isEmpty()

garydgregory · 2022-10-19T11:15:32Z

src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java

+ }
+
+ public void init() {
+ String restrictedClasses = System.getProperty("jxpath.class.allow");


Use final where you can.

garydgregory · 2022-10-19T11:16:10Z

src/test/java/org/apache/commons/jxpath/ri/compiler/TestFunctions3.java

+import org.apache.commons.jxpath.Functions;
+import org.apache.commons.jxpath.JXPathContext;
+
+import java.util.*;


Don't use star imports.

Removed. The import was not used anyway.

garydgregory · 2022-10-19T11:21:07Z

@kyakdan I'm not sure what happened on your rebase but you should not have to edit pom.xml or changes.xml.

garydgregory · 2022-10-19T11:30:27Z

src/main/java/org/apache/commons/jxpath/Functions.java

+ * @param jxPathFilter the XPath filter
+ * @return Function
+ */
+ Function getFunction(String namespace, String name, Object[] parameters, JXPathFilter jxPathFilter);


This needs to be a default method to avoid breaking binary compatibility. This is a misconfiguration in our parent POM.

This method has different implementations for the three classes implementing the Functions interface, it cannot be made final. So, in order to not break binary compatibility, I've removed this function and made the implemented getFunction methods use a filter based on the jxpath.allow.class system property. This way we don't have to explicitly pass a filter parameter to the getFunction method.

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. 9.8 Critical 1.3 and earlier 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. - 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.

kyakdan · 2022-10-19T19:52:47Z

@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 · 2022-10-19T19:56:40Z

src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java

+ * JXPath uses this filter instance when an extension function instance is created.
+ */
+public class JXPathFilter implements JXPathClassFilter {
+ private Set<String> allowedClassesList = null;


Maybe rename the set to just allowedClasses, since it is not list anymore?

Good catch. Changed. I've also simplified the constructor.

garydgregory · 2022-10-19T20:16:57Z

src/test/java/org/apache/commons/jxpath/ri/compiler/ExtensionFunctionTest.java

- new PackageFunctions(
- "org.apache.commons.jxpath.ri.compiler.",
- "jxpathtest"));
+ new PackageFunctions(


What's changed here? There are too many formatting changes to make this simple to review.
In general, don't reformat existing code in PRs, it just makes noise in the PR.

This reason is my IDE auto-reformating the file. I've only kept the relevant changes that are part of this PR.

Warxim · 2022-10-19T20:18:42Z

src/main/java/org/apache/commons/jxpath/ClassFunctions.java

@@ -91,6 +92,13 @@ public Function getFunction(
 final String namespace,
 final String name,
 Object[] parameters) {
+ JXPathFilter jxPathFilter = new JXPathFilter();


Not sure, but I am a little bit worried that this can be quite limiting to developers. We force them to use System.setProperty in order to allow some classes in jxpath functions. Also, it is not possible to change the filter per JXContext

I think we should have some kind of interface JXPathFilter that will have all required methods (e.g. isClassNameExposed(className)) that will be used as the parameter of the overloaded method Functions.getFunction(...). The overloaded method would create the SystemPropertyJXPathFilter implementation that uses system properties to specify filters.

Then we would add field to JXPathContext that would hold the default JXPathFilter for given context (by default our properties based filter). We would also add methods to change this filter, so that the developer can configure per-context filters.

The JXPathFilter itself should be either interface with all the methods or just some filter-holder that would contain all the filters (interfaces JXPathClassFilter, JXPathXyzFilter, ...), so that we can pass it through the structure.

Thanks for the proposal. I've added an interface JXPathFilter and an implementation SystemPropertyJXPathFilter based on the system property of the allow list. Moreover, I have added a new default method getFunction to the Functions interface that takes a filter as a parameter. The default implementation simply calls the getFunction without the filter and can be overridden by implemented classes. This guarantees binary compatibility as @garydgregory pointer out.

This has the additional benefit of not breaking the binary compatibility of the Functions interface

Warxim

I finally got time to check the code of JXPath and I found out that this solution is unfortunately not secure enough. It will prevent the simple fully-qualified class name access, however, there is one more problem in the code. It still allows calling methods on context object like size for List. There is also support for using methods on parameter objects (see for example PackageFunctions.getFunction(…) and its block within parameters.length condition. In the end, this functionality allows us to traverse the objects and also load any classes.

For example, I have created context for simple data class:

JXPathContext context = JXPathContext.newContext(new Data());

The class itself does not contain anything:

public class Data {

}

I also created DangerousClass (in real-life scenario, this could be the ClassPathXmlApplicationContext from Spring), which contains dangerous code:

public class DangerousClass {

    public DangerousClass() {
        System.out.println("Dangerous class created!");
    }

    public void run(String param) {
        System.out.println(String.format("Running dangerous test with parameter '%s'!", param));
        System.exit(42);
    }

}

I have to start in context of Data.class, but I can easily go and load the DangerousClass and then invoke the dangerous function run:

JXPathContext context = JXPathContext.newContext(new Data());
String jxPath = "run(newInstance(loadClass(getClassLoader(getClass(/)), \"com.warxim.dangerous.DangerousClass\")), \"warxim\")"
Object result = context.getValue(jxPath);

This will lead to the following output:

Dangerous class created!
Running dangerous test with parameter 'warxim'!

Process finished with exit code 42

Conclusion: It is crucial to secure all the mechanisms that allow calling methods on the objects.

kyakdan · 2022-11-05T04:21:12Z

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.

JanHron · 2022-11-07T15:48:49Z

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.

ecki · 2022-11-07T16:15:26Z

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?

markt-asf · 2022-11-08T17:22:19Z

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.

kyakdan · 2022-11-09T04:15:38Z

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.

ecki · 2022-11-11T03:37:48Z

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.

stephanborn · 2022-11-11T08:33:39Z

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.

markt-asf · 2022-11-11T10:15:45Z

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.

JanHron · 2022-11-11T10:58:17Z

@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.

hummelm10 · 2022-11-11T14:16:19Z

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.

kyakdan · 2022-11-14T15:36:27Z

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.

stephanborn · 2022-11-18T08:42:11Z

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.

markt-asf · 2022-11-18T08:46:01Z

There is no security vulnerability. This PR will be dealt with with the same priority as any other enhancement request.

iamamoose · 2022-11-18T12:51:58Z

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).

hummelm10 · 2022-11-18T16:54:33Z

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.

oliverchang · 2022-11-21T04:16:32Z

@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?

stephanborn · 2022-11-22T14:29:09Z

OK, just saw that CVE-2022-41852 is marked as REJECTED: https://nvd.nist.gov/vuln/detail/CVE-2022-41852

hummelm10 · 2022-11-22T14:39:11Z

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.

ecki · 2022-11-23T14:10:55Z

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.

This should work as a caveat as discussed in apache#26

ecki · 2022-11-24T13:11:04Z

Any comment on my PR?

kyakdan · 2023-02-13T05:12:35Z

@garydgregory @markt-asf Is there any update on when this PR can be merged?

supermaurio · 2023-03-10T13:31:16Z

@kyakdan in the current form, every class requested from ClassLoaderUtil needs to be whitelisted.
This means that by default, even basic methods like JXPathContext.newContext() will fail (as org.apache.commons.jxpath.ri.JXPathContextFactoryReferenceImpl will be filtered by default)
Is this intended behaviour?

schlm3 · 2023-03-17T09:47:03Z

Like many of the JXPath users community, I also think that this is a security issue because of JXPath's default configuration.
However, it seems that the mitigation approach taken by @kyakdan will not make it into the official code.

The question for me now is, how all the usages of JXPath out there could be secured without having the project responsibles to change code in the project. But for all those concerned about having that security issue in their apps, there is (as far as I could find out) a secure way to get rid of the problem. You need to set the default "Functions" implementation to YOUR default instead of silently using the default from JXPathContext (which is JXPathContext.GENERIC_FUNCTIONS ). All you have to do is adding the following line for each instance of JXPathContext in your Application:
context.setFunctions(new org.apache.commons.jxpath.FunctionLibrary());

Of course, you will loose any way of using direct Java Class functions. But the approach also offers the possibility to add your own, secured implementation of the JXPath's PackageFunction, which could be one with a "whitelist" approach like proposed in this thread.
Many usages of JXPath however won't even require the Package Functions if they only need to traverse graphs of objects.

You could even provide your own JXPathContextFactory to your Application VM such that any Usage of JXPathContext.newContext() will return a secured context by default.

kyakdan · 2023-03-17T09:57:29Z

@kyakdan in the current form, every class requested from ClassLoaderUtil needs to be whitelisted. This means that by default, even basic methods like JXPathContext.newContext() will fail (as org.apache.commons.jxpath.ri.JXPathContextFactoryReferenceImpl will be filtered by default) Is this intended behaviour?

You can provide a pattern to allow a group of classes, e.g., org.w3c.*. This allows all classes in the org.w3c package. You can use * as a pattern to allow everything which matches the current behavior.

ecki · 2023-03-17T22:21:43Z

I don’t think „most“ is correct, since this was not much requested before and many users probably only issue static jxpath expressions or use it as a cheap java eval language anyway.

having said that, your mentioning of the FunctionLibrary is on point, and it also looks like a better way to filter. Jxpath can just ship (configurable) alternatives, which especially have allow lists for functions, can turn java evaluation off or restrict classes and methods (similar to like the PR does, ideally without the need to allow internal methods)

bhmohanr-techie mentioned this pull request Oct 18, 2022

Fix for Issue: CVE-2022-41852 (CVE has been rejected) #25

Open

madrob suggested changes Oct 18, 2022

View reviewed changes

kyakdan force-pushed the master branch 3 times, most recently from 1da4f86 to d6e0c94 Compare October 19, 2022 05:36

garydgregory requested changes Oct 19, 2022

View reviewed changes

garydgregory reviewed Oct 19, 2022

View reviewed changes

kyakdan force-pushed the master branch from d6e0c94 to d82787b Compare October 19, 2022 15:34

bharathmohanraj and others added 8 commits October 19, 2022 21:49

Throw JXPathException directly when filtering a function class

02dc4f5

Enable the allow-list by default for all getFunction methods

c248c0b

Update JXPathClassFilter documentation to mention the allow-all option

6d5e13f

Organize Imports

0e337f0

Convert the restricted classes into a Set

b306289

Fix Checkstyle and PMD errors

ff5ab0a

Minor refactoring and clean-up

021b8ce

kyakdan force-pushed the master branch from d82787b to 4ae80cb Compare October 19, 2022 19:49

Warxim reviewed Oct 19, 2022

View reviewed changes

kyakdan force-pushed the master branch 2 times, most recently from fa9fb1e to 751b035 Compare October 19, 2022 20:13

garydgregory reviewed Oct 19, 2022

View reviewed changes

kyakdan force-pushed the master branch from 751b035 to 32e73bb Compare October 19, 2022 20:17

Warxim reviewed Oct 19, 2022

View reviewed changes

kyakdan force-pushed the master branch 2 times, most recently from 8a301f3 to ea34611 Compare October 19, 2022 20:36

Use JXPathFilter based on the jxpath.allow.class system property

27d1ec0

This has the additional benefit of not breaking the binary compatibility of the Functions interface

kyakdan force-pushed the master branch from ea34611 to 10691c4 Compare October 20, 2022 04:55

Warxim suggested changes Oct 20, 2022

View reviewed changes

kyakdan changed the title ~~Fix CVE-2022-41852~~ Add an allow list for classes that can be loaded by JXPath Nov 14, 2022

ecki added a commit to ecki/commons-jxpath that referenced this pull request Nov 23, 2022

index.html: Add warning about expression risks

99e6f77

This should work as a caveat as discussed in apache#26

ecki mentioned this pull request Nov 23, 2022

index.html: Add warning about expression risks #38

Closed

RomRom1 mentioned this pull request Nov 28, 2022

[FP]: CVE-2022-41852 has been rejected jeremylong/DependencyCheck#5092

Open

merks mentioned this pull request Dec 9, 2022

org.apache.commons.jxpath_1.3.0.v200911051830.jar (Apache Commons JXPath Vulnerability) eclipse-platform/eclipse.platform#291

Open

Add an allow list for classes that can be loaded by JXPath #26

Are you sure you want to change the base?

Add an allow list for classes that can be loaded by JXPath #26

Conversation

kyakdan commented Oct 14, 2022 • edited Loading

madrob 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

garydgregory 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

garydgregory commented Oct 19, 2022

garydgregory Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyakdan commented Oct 19, 2022

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

Warxim Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Warxim Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Warxim left a comment • edited Loading

Choose a reason for hiding this comment

kyakdan commented Nov 5, 2022

JanHron commented Nov 7, 2022 • edited Loading

ecki commented Nov 7, 2022 via email • edited Loading

markt-asf commented Nov 8, 2022

kyakdan commented Nov 9, 2022

ecki commented Nov 11, 2022

stephanborn commented Nov 11, 2022

markt-asf commented Nov 11, 2022

JanHron commented Nov 11, 2022

hummelm10 commented Nov 11, 2022

kyakdan commented Nov 14, 2022

stephanborn commented Nov 18, 2022

markt-asf commented Nov 18, 2022

iamamoose commented Nov 18, 2022

hummelm10 commented Nov 18, 2022 • edited Loading

oliverchang commented Nov 21, 2022

stephanborn commented Nov 22, 2022

hummelm10 commented Nov 22, 2022

ecki commented Nov 23, 2022

ecki commented Nov 24, 2022

kyakdan commented Feb 13, 2023

supermaurio commented Mar 10, 2023

schlm3 commented Mar 17, 2023 • edited Loading

kyakdan commented Mar 17, 2023

ecki commented Mar 17, 2023

kyakdan commented Oct 14, 2022 •

edited

Loading

garydgregory Oct 19, 2022 •

edited

Loading

Warxim Oct 19, 2022 •

edited

Loading

Warxim Oct 19, 2022 •

edited

Loading

Warxim left a comment •

edited

Loading

JanHron commented Nov 7, 2022 •

edited

Loading

ecki commented Nov 7, 2022 via email •

edited

Loading

hummelm10 commented Nov 18, 2022 •

edited

Loading

schlm3 commented Mar 17, 2023 •

edited

Loading