Conversation
dlmarion
changed the title
|
Full IT build passed |
phrocker
left a comment
phrocker
left a comment
There was a problem hiding this comment.
A couple of questions. I'm not sure my assertions are correct. I ran a quick test but may be I misunderstood my test?
| @Override | ||
| public ClassLoader getClassLoader(String context) { | ||
| public void init(ContextClassLoaderEnvironment env) { | ||
| String urlPatternProperty = env.getConfiguration().get(URL_PATTERN_PROPERTY); |
There was a problem hiding this comment.
The factory normalizes the context URL, so file:///opt/ctx/a.jar becomes file:/opt/ctx/a.jar, but the pattern is matched exactly as configured. An admin who sets file:///opt/ctx/.* gets:
user pattern matches: false -- and then contexts are denied, is this expected?
There was a problem hiding this comment.
The changes added in 2e8e0b3 should address this.
| try { | ||
| return URI.create(p).toURL(); | ||
| URL url = new URL(p); | ||
| checkArgument(urlPattern.matcher(url.toExternalForm()).matches(), |
There was a problem hiding this comment.
This doesn't normalize the path does it? Should the URL be normalized to any URL containing .. to avoid path traversal?
There was a problem hiding this comment.
The changes added in 2e8e0b3 should address this.
Assisted-by: OpenAI GPT-6 Luna
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
I made reference to normalization using URI in my other comments, but that doesn't handle URL-encoded things like /%2E%2E/
However, this would:
new URL(url, Paths.get(URLDecoder.decode(url.getPath())).normalize().toString().replace("\\","/"));This context classloader factory used to be a test. It was only moved to be the default implementation in 3.0 after the VFS stuff was stripped out. However, I think that it should not be the default. I think it's fine if we went back to using this for tests only. The default implementation should be a dummy/pass-through factory that just returns the system classloader for every context. If a user wants different behavior, that's when they set it to a different factory.
| urlPattern = | ||
| Pattern.compile(urlPatternProperty.replaceAll(":///", ":/").replaceAll("://", ":/")); |
There was a problem hiding this comment.
I don't think we should modify the user's configuration here. The pattern should be expected to validate against any normalized URL. If the user configures a pattern that isn't normalized, we shouldn't guess what they intended, and should just accept the pattern as-is, rejecting if needed.
| // Match and return the actual filesystem target, not the possibly traversing URL spelling. | ||
| // toRealPath also resolves symbolic links, so an allowed path cannot escape via a symlink. |
There was a problem hiding this comment.
I don't think this should be considered an escape. This prevents legitimate use cases for file system organization/management. If the pattern matches, it means we trust whatever is at that location... even if it's a symlink. This is no different than trusting an HTTP location that is a proxy for another location, or any other URL-redirect mechanism. Whether or not the resource at the given URL is the actual resource, or a proxy, is entirely dependent on the URL resolver. We don't need to complicate it... we can just trust the location or not, as it is specified.
If we really want to avoid path traversal bugs, then we can address that with normalization:
url.toURI().normalize().toURL();| } else if ("hdfs".equalsIgnoreCase(uri.getScheme())) { | ||
| // Match and return the actual filesystem target, not the possibly traversing URL spelling. | ||
| // resolvePath also resolves symbolic links, so an allowed path cannot escape via a symlink. | ||
| // This may subsequently fail if the HdfsURLStreamHandlerProvider is not on the classpath | ||
| org.apache.hadoop.fs.Path resolved = | ||
| FileSystem.get(uri, new Configuration()).resolvePath(new org.apache.hadoop.fs.Path(uri)); | ||
| url = resolved.toUri().toURL(); |
There was a problem hiding this comment.
Same comment here.
Also, there are some weird quirks with how some versions of Hadoop resolve paths as URIs (apache/hadoop#8307). I'm not sure this would handle path traversal avoidance through normalization better than just using URI.normalize.
The comment also makes reference to a specific URLStreamHandlerProvider, but any URLStreamHandlerProvider for hdfs would work for converting it to a URL.
This branch has not been deployed
3 participants
No description provided.