[GEOT-7587] Improve handling of XPath expressions #4797

sikeoka · 2024-06-04T15:16:22Z

This PR refactors all jxpath initialization into a new utility method that disables calling functions from XPath expressions and fixes an incomplete class check that allowed complex feature code to be used on simple features. Some unnecessary dependency exclusions were also removed to clean up the pom.xml (commons-jxpath has no transitive dependencies).

Checklist

I have read the contribution guidelines.
I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
First PR targets the main branch (backports managed later; ignore for branch specific issues).
Avoid Java 9+ split packages.
All the build checks are green (see automated QA checks).

For core and extension modules:

New unit tests have been added covering the changes.
Documentation has been updated (if change is visible to end users).
There is an issue in GeoTools Jira (except for changes not visible to end users).
Commit message(s) must be in the form [GEOT-XYZW] Title of the Jira ticket.
Bug fixes and small new features are presented as a single commit.
The commit targets a single objective (if multiple focuses cannot be avoided, each one is in its own commit, and has a separate ticket describing it).

modules/extension/xsd/xsd-core/src/test/java/org/geotools/xsd/StreamingParserTest.java

davidblasby

Looks like a great addition. I don't really know the code, so I cannot say too much - sorry.

I think increasing the 5 second to something like 20 seconds would make it very unlikely to have a busy-machine problem. However, this is just a guess - its okay the way it is.

...ension/complex/src/test/java/org/geotools/filter/expression/FeaturePropertyAccessorTest.java

jodygarnett

I am approving, with some minor feedback (test case threadsafety, and method name choice). I do not feel comfortable about "request changes" as that would block PR.

@sikeoka I will wait for your response/consideration before merge and backport.

jodygarnett · 2024-06-05T01:02:21Z

modules/extension/xsd/xsd-core/src/test/java/org/geotools/xsd/StreamingParserTest.java

+    public void testParseWithJavaMethod() throws Exception {
+        ByteArrayInputStream in = new ByteArrayInputStream("<mails></mails>".getBytes());
+        StreamingParser parser =
+                new StreamingParser(new MLConfiguration(), in, "java.lang.Thread.sleep(30000)");


I am a bit confused as to choosing a time based example? Why not java.lang.Math.abs(-1.0) - and test. to see that 1.0 is returned. This would be a test that is not sensitive to build server speed?

This is a type of issue that is commonly referred to as blind. Just because something is being done doesn't mean that the result is actually sent anywhere so I needed some kind of side effect that I could check for. I originally tested this using System.exit but that isn't as JUnit or Maven friendly.

Okay that makes sense, thanks for explaining.

jodygarnett · 2024-06-05T01:04:54Z

...mplex/src/main/java/org/geotools/data/complex/expression/FeaturePropertyAccessorFactory.java

-            }
-
+            JXPathContext context =
+                    JXPathUtils.newSafeContext(object, false, this.namespaces, true);


Minor feedback, newSafeContext may promise too much, if attribute security is used for something like GeoFence security plugin this would not know enough to prevent specific data traversals.

A name like newDataContext would be more clear this is about providing access to data only, not functions.

I named this based on two existing cases for similar types of issues although getSafeContext would be a more consistent name than newSafeContext:
org.geoserver.template.TemplateUtils.getSafeConfiguration()
org.geotools.ysld.YamlUtil.getSafeYaml()

jodygarnett · 2024-06-05T01:11:57Z

modules/extension/xsd/xsd-core/src/main/java/org/geotools/xsd/impl/jxpath/JXPathUtils.java

+     * @param lenient whether the context is in lenient mode
+     * @return the context
+     */
+    public static JXPathContext newSafeContext(Object contextBean, boolean lenient) {


Thanks for the clear javadocs I appreciate it.

Minor feedback: Consider newDataContext to accurately describe the restriction of no methods. In a more complicated security model such as GeoFence data traversal may be considered "unsafe" leading to this method name being misleading / incomplete.

I really don't understand this comment since GeoFence has no dependence on gt-complex or commons-jxpath.

I was trying to indicate that specific attributes may be restricted (using a geofence as an example of a plugin that offers this kind of functionality).

So "safe" indicates no functions, but it would not be safe from using xpath to traverse data to access a restricted columns.

As indicated this was minor feedback only.

aaime · 2024-06-05T17:33:15Z

The backport to 29.x failed:

The process '/usr/bin/git' failed with exit code 1

stderr

error: could not apply e53e5170ba... [GEOT-7587] Improve handling of XPath expressions
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config advice.mergeConflict false"

stdout

Auto-merging modules/extension/complex/src/main/java/org/geotools/data/complex/expression/FeaturePropertyAccessorFactory.java
CONFLICT (content): Merge conflict in modules/extension/complex/src/main/java/org/geotools/data/complex/expression/FeaturePropertyAccessorFactory.java
Auto-merging modules/extension/complex/src/main/java/org/geotools/data/complex/expression/MapPropertyAccessorFactory.java
CONFLICT (content): Merge conflict in modules/extension/complex/src/main/java/org/geotools/data/complex/expression/MapPropertyAccessorFactory.java
Auto-merging modules/extension/complex/src/test/java/org/geotools/filter/expression/FeaturePropertyAccessorTest.java
Auto-merging modules/extension/xsd/xsd-core/pom.xml
CONFLICT (content): Merge conflict in modules/extension/xsd/xsd-core/pom.xml

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-29.x 29.x
# Navigate to the new working tree
cd .worktrees/backport-29.x
# Create a new branch
git switch --create backport-4797-to-29.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick e53e5170ba71521728875a436c80616cfb03c1e8
# Push it to GitHub
git push --set-upstream origin backport-4797-to-29.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-29.x

Then, create a pull request where the base branch is 29.x and the compare/head branch is backport-4797-to-29.x.

javacoderpyl · 2024-08-08T07:39:18Z

Hi, bro, since the versions 30.4, 31.2, and 29.6 of GeoTools that fix the vulnerability CVE-2024-36404 are all compiled jar files based on JDK 11, could you release a version that fixes this vulnerability but is compiled with JDK 8, such as version 28.x?

jodygarnett · 2024-08-08T13:55:29Z

No one has volunteered to do that. We have a public release procedure if you wish to volunteer.

Even if you back port this fix, there are other vulnerabilities in older versions so why bother?

javacoderpyl · 2024-08-08T14:20:21Z

No one has volunteered to do that. We have a public release procedure if you wish to volunteer.

Even if you back port this fix, there are other vulnerabilities in older versions so why bother?

Because our projects are based on Jdk8, the version of jdk11 cannot be run in our project. If I can study the open course and release a jdk 8 version of geotools that solves this vulnerability, how should I get this course?

sikeoka force-pushed the GEOT-7587 branch from 582ac17 to fc74f90 Compare June 4, 2024 15:24

davidblasby reviewed Jun 4, 2024

View reviewed changes

modules/extension/xsd/xsd-core/src/test/java/org/geotools/xsd/StreamingParserTest.java Outdated Show resolved Hide resolved

davidblasby approved these changes Jun 4, 2024

View reviewed changes

...ension/complex/src/test/java/org/geotools/filter/expression/FeaturePropertyAccessorTest.java Show resolved Hide resolved

[GEOT-7587] Improve handling of XPath expressions

e53e517

sikeoka force-pushed the GEOT-7587 branch from fc74f90 to e53e517 Compare June 4, 2024 17:20

jodygarnett approved these changes Jun 5, 2024

View reviewed changes

jodygarnett added backport 30.x Backport bot will backport to 30.x on merge backport 31.x Automatic backport to 31.x branch labels Jun 5, 2024

jodygarnett merged commit f0c9961 into geotools:main Jun 5, 2024
23 checks passed

This was referenced Jun 5, 2024

[Backport 30.x] [GEOT-7587] Improve handling of XPath expressions #4800

Merged

[Backport 31.x] [GEOT-7587] Improve handling of XPath expressions #4801

Merged

aaime added the backport 29.x label Jun 5, 2024

aaime added the failed backport label Jun 5, 2024

jodygarnett removed failed backport backport 29.x labels Jun 6, 2024

foyaga mentioned this pull request Jul 2, 2024

GeoServer property 表达式注入代码执行漏洞（CVE-2024-36401） foyaga/blog-timeline#294

Open

mirandadam mentioned this pull request Jul 3, 2024

Geoserver build for patch CVE-2024-36401 GeoNode/geonode-docker#44

Closed

sikeoka deleted the GEOT-7587 branch July 15, 2024 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GEOT-7587] Improve handling of XPath expressions #4797

[GEOT-7587] Improve handling of XPath expressions #4797

sikeoka commented Jun 4, 2024 •

edited

Loading

davidblasby left a comment

jodygarnett left a comment

jodygarnett Jun 5, 2024

sikeoka Jun 5, 2024 •

edited

Loading

jodygarnett Jun 5, 2024

jodygarnett Jun 5, 2024

sikeoka Jun 5, 2024 •

edited

Loading

jodygarnett Jun 5, 2024

sikeoka Jun 5, 2024

jodygarnett Jun 5, 2024

aaime commented Jun 5, 2024

javacoderpyl commented Aug 8, 2024

jodygarnett commented Aug 8, 2024

javacoderpyl commented Aug 8, 2024

[GEOT-7587] Improve handling of XPath expressions #4797

[GEOT-7587] Improve handling of XPath expressions #4797

Conversation

sikeoka commented Jun 4, 2024 • edited Loading

Checklist

davidblasby left a comment

Choose a reason for hiding this comment

jodygarnett left a comment

Choose a reason for hiding this comment

jodygarnett Jun 5, 2024

Choose a reason for hiding this comment

sikeoka Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

jodygarnett Jun 5, 2024

Choose a reason for hiding this comment

jodygarnett Jun 5, 2024

Choose a reason for hiding this comment

sikeoka Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

jodygarnett Jun 5, 2024

Choose a reason for hiding this comment

sikeoka Jun 5, 2024

Choose a reason for hiding this comment

jodygarnett Jun 5, 2024

Choose a reason for hiding this comment

aaime commented Jun 5, 2024

javacoderpyl commented Aug 8, 2024

jodygarnett commented Aug 8, 2024

javacoderpyl commented Aug 8, 2024

sikeoka commented Jun 4, 2024 •

edited

Loading

sikeoka Jun 5, 2024 •

edited

Loading

sikeoka Jun 5, 2024 •

edited

Loading