You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/10/18 16:37:47 UTC
[GitHub] [commons-jxpath] madrob commented on a diff in pull request #26: Fix CVE-2022-41852
madrob commented on code in PR #26:
URL: https://github.com/apache/commons-jxpath/pull/26#discussion_r998470797
##########
src/main/java/org/apache/commons/jxpath/ri/JXPathContextReferenceImpl.java:
##########
@@ -27,23 +27,10 @@
import java.util.Vector;
import java.util.Map.Entry;
-import org.apache.commons.jxpath.CompiledExpression;
-import org.apache.commons.jxpath.ExceptionHandler;
-import org.apache.commons.jxpath.Function;
-import org.apache.commons.jxpath.Functions;
-import org.apache.commons.jxpath.JXPathContext;
-import org.apache.commons.jxpath.JXPathException;
-import org.apache.commons.jxpath.JXPathFunctionNotFoundException;
-import org.apache.commons.jxpath.JXPathInvalidSyntaxException;
-import org.apache.commons.jxpath.JXPathNotFoundException;
-import org.apache.commons.jxpath.JXPathTypeConversionException;
-import org.apache.commons.jxpath.Pointer;
+import org.apache.commons.jxpath.*;
Review Comment:
The project does not appear to use wildcard imports, let's stick with existing conventions for now.
##########
src/main/java/org/apache/commons/jxpath/ri/JXPathFilter.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.jxpath.ri;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+
+/**
+ * A filter to be used by JXPath, to evaluate the xpath string values to impose any restrictions.
+ * This class implements specific filter interfaces, and implements methods in those.
+ * 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
+ * @version $Revision$ $Date$
+ */
+public class JXPathFilter implements JXPathClassFilter {
+ ArrayList<String> allowedClassesList = null;
Review Comment:
This could probably be a Set.
##########
src/test/java/org/apache/commons/jxpath/ri/model/BeanModelTestCase.java:
##########
@@ -44,14 +44,23 @@
public abstract class BeanModelTestCase extends JXPathTestCase {
private JXPathContext context;
- public void setUp() {
+ @Override
+ public void setUp() throws Exception {
// if (context == null) {
- context = JXPathContext.newContext(createContextBean());
- context.setLocale(Locale.US);
- context.setFactory(getAbstractFactory());
+ super.setUp();
+ System.setProperty("jxpath.class.allow", "*");
+ context = JXPathContext.newContext(createContextBean());
+ context.setLocale(Locale.US);
+ context.setFactory(getAbstractFactory());
// }
}
+ @Override
+ public void tearDown() throws Exception {
+ super.tearDown();
Review Comment:
Very minor, should call super.tearDown at the end of the method not the beginning.
##########
src/main/java/org/apache/commons/jxpath/ri/JXPathClassFilter.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.jxpath.ri;
+
+/**
+ * Class filter (optional) to be used by JXPath.
+ *
+ * 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
Review Comment:
Should also document the wildcard * for allowing all classes.
##########
src/main/java/org/apache/commons/jxpath/ClassFunctions.java:
##########
@@ -92,6 +93,28 @@ public Function getFunction(
String namespace,
String name,
Object[] parameters) {
+ return getFunction(namespace, name, parameters, null);
+ }
+
+ public Function getFunction(
+ String namespace,
+ String name,
+ Object[] parameters,
+ JXPathFilter jxPathFilter) {
+
+ // give chance to ClassFilter to filter out, if present
+ try {
+ if (jxPathFilter != null && !jxPathFilter.exposeToXPath(functionClass.getName())) {
+ throw new ClassNotFoundException(functionClass.getName());
Review Comment:
We shouldn't be using exceptions for control flow here, let's throw JXPathException directly.
##########
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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org