Community
Participate
Working Groups
Created attachment 224051 [details] example Hi, In Gemini Web we are using Bundle.findEntries(String path, String filePattern, boolean recurse) when we have to serve a user request. By specification (OSGi Enterprise) we must not serve static content from several forbidden directories: "For confidentiality reasons, a Web Runtime must not return any static content for paths that start with one of the following prefixes: WEB-INF/ OSGI-INF/ META-INF/ OSGI-OPT/" Gemini Web checks for requests like this: http://localhost:8080/<context-path>/WEB-INF/web.xml and returns 404 Not Found However for requests like this http://localhost:8080/<context-path>/WEB-INF./web.xml Gemini Web does not check anything because basically directory with name "WEB-INF." is not present in the bundle. We expect that Bundle.findEntries(...) will return an empty enumeration. The problem is reproducible when the bundle is install from a directory and only on Windows OS - Bundle.findEntries(...) returns a valid URL to WEB-INF/web.xml. I made several experiments and here is the result: " new File("META-INF./MANIFEST.MF").getCanonicalPath(): C:\temp\META-INF\MANIFEST.MF new File("META-INF./MANIFEST.MF").getAbsolutePath(): C:\temp\META-INF.\MANIFEST.MF new File("META-INF./MANIFEST.MF").exists(): true new File("META-INF/MANIFEST.MF").getCanonicalPath(): C:\temp\META-INF\MANIFEST.MF new File("META-INF/MANIFEST.MF").getAbsolutePath(): C:\temp\META-INF\MANIFEST.MF new File("META-INF/MANIFEST.MF").exists(): true " When Bundle.findEntries(...) is invoked, at some point of time it invokes DirBundleFile.getFile(...) where only exists() check is performed " if (!BundleFile.secureAction.exists(file)) { return null; } " I think that a check should be made whether the canonical and absolute paths are equals. I'm attaching very simple example that illustrates the problem. You can install it from archive and from an exploded form in order to check the differences. Regards Violeta
(In reply to comment #0) > I think that a check should be made whether the canonical and absolute paths > are equals. > Of course the absolute path should be normalised before then.
This seems similar to bug 320546. But it is a case where some form of a URL is providing an alias to another entry in the bundle. We could probably add code to do a similar check that we did for bug 320546, but I think the client of findEntries should also be sanitizing the input to make sure there are no ./ occurrences the input.
(In reply to comment #2) > We could probably add code to do a similar check that we did for bug 320546, > but I think the client of findEntries should also be sanitizing the input to > make sure there are no ./ occurrences the input. We will add additional checks in Gemini Web. But please note that Only on Windows this is true: <directory> == <directory>. On Linux and MAC <directory> != <directory>. So it is acceptable to have for example "META-INF." and this directory will be totally different from "META-INF" directory.
Hi, I'm decreasing the priority of the issue as we provided additional checks in Gemini Web. However this is a valid issue so I'm leaving it open. Regards Violeta
(In reply to comment #4) > Hi, > > I'm decreasing the priority of the issue as we provided additional checks in > Gemini Web. However this is a valid issue so I'm leaving it open. > > Regards > Violeta That is fine. Would you happen to have a patch ready for a fix?
I'm removing security flag since I don't really think handing back entries that actually do exist in the bundle, but for which the input path does not match exactly the entry name is a security issue.
Hi, We faced another one issue connected to this bug. If the bundle is installed from a directory and the bundle for example contains test.txt file. Call to Bundle.getEntry("test.TXT") will return a valid URL. Note that the file extension used in getEntry() is with upper case letters. This can be reproduced on Windows and MAC. I prepared a patch that fixes the both issues. Please find it attached. The idea is the following: The canonical path returns the real file, on the other hand the absolute path will keep the path as it is provided. i.e. - in case absolute path is META-INF./MANIFEST.MF -> the canonical will be META-INF/MANIFEST.MF - in case absolute path is test.TXT -> the canonical will be test.txt So if we compare the canonical and absolute paths and if they are not equal then NULL will be return. One point here is that the canonical path is normalized (/../, /./, // are removed) so if such symbols are available in the absolute path we have to remove them first and then to make the comparison. I'm doing checks before normalization because I do not want to make worse the performance. I'm looking forward to your comments. Thanks Violeta
Created attachment 226730 [details] patch proposal
I am worried about fixing this bug. 1) The current proposal fails for calls to Bundle.getEntry("/") this should never return null since '/' indicates the root of the bundle. The proposal fails because the compare between the canonical and the absolute path fails since the canonical path is removing the last '/' 2) I worry about other cases where this will cause unexpected failures. I'm not sure I understand the severity of this bug. It it about consistency between the different platforms? As it stands now the content being requested does really exist in the bundle, but perhaps being access with an unexpected alias.
Violeta, I'm not sure I will be convinced to change the default behavior of Equinox here. If this issue is greatly impacting your project or Virgo then perhaps the final fix can be enabled with some config option?
Created attachment 229125 [details] Improved patch
(In reply to comment #9) > I am worried about fixing this bug. > > 1) The current proposal fails for calls to Bundle.getEntry("/") this should > never return null since '/' indicates the root of the bundle. The proposal > fails because the compare between the canonical and the absolute path fails > since the canonical path is removing the last '/' Added additional check in the proposed patch. When the path "/" the checks will be skipped. > > 2) I worry about other cases where this will cause unexpected failures. > > I'm not sure I understand the severity of this bug. It it about consistency > between the different platforms? As it stands now the content being > requested does really exist in the bundle, but perhaps being access with an > unexpected alias. https://bugs.eclipse.org/bugs/show_bug.cgi?id=395246 https://bugs.eclipse.org/bugs/show_bug.cgi?id=397311 (In reply to comment #10) > Violeta, I'm not sure I will be convinced to change the default behavior of > Equinox here. If this issue is greatly impacting your project or Virgo then > perhaps the final fix can be enabled with some config option? It is OK for us. Regards Violeta
Hi, Can you comment the proposal? Thanks a lot. Violeta
(In reply to comment #13) > Hi, > > Can you comment the proposal? > > Thanks a lot. > Violeta Hi Violeta, the patch still seems to do the check by default. Could you introduce some option such that this is not enabled by default?
Created attachment 229500 [details] Patch proposal Hi, Here is a new patch that introduces a new property. When the property is enabled the new functionality will be executed. Here is what I would like to add to the documentation: " osgi.strictBundleEntryPath On some operating systems Equinox may return a bundle entry that is requested with a path that does not correspond to an actual bundle entry path. For example, on Windows file name comparison operations are case insensitive. When this property is set to true, Equinox operates more strictly by checking that the requested bundle entry path exists. The default value is false for compatibility with the previous behaviour of Equinox. This property is relevant only when the bundle is installed from a folder and is ignored otherwise. " Thanks Violeta
I reworked the patch a bit. Please review the final commit here: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=7591e36abd6786ae4b6a3e57e9d3d07f4be3cad2 I also released the new documentation here: http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=3fcd2bfab6ed9177c635bb61c1f6288a99ac7033 Thanks Violeta!
Created attachment 229529 [details] Additional patch for commit 7591e36abd6786ae4b6a3e57e9d3d07f4be3cad2 Hi, Unfortunately the committed change does not work properly on Windows OS. If we do not use a canonical path as a parent when creating the File object then there are use cases when: - the canonical path for the File object is C:\temp\virgo-tomcat-server-3.6.0.RELEASE\work\deployer\s\global\1\0\org.eclipse.gemini.blueprint.core_1.0.2.RELEASE.jar\META-INF\MANIFEST.MF - the absolute path for the File object is C:\temp\VIRGO-~3.REL\work\deployer\s\global\1\0\org.eclipse.gemini.blueprint.core_1.0.2.RELEASE.jar\META-INF\MANIFEST.MF I'm proposing a change to the existing commit. I'm looking forward to your comments. Thanks Violeta
Reopen the issue.
Created attachment 229550 [details] alternative fix Here is an alternative. I do not want to add another String member variable to the DirBundleFile to just hold the canonical path. This patch moves the check for the configuration option outside of DirBundleFile and instead passes the boolean into the constructor. The constructor then uses the boolean to decide if it should convert the bundlefile File param to a canonical File. That way we don't need to store the canonical Path string because the basefile will already be in the correct canonical form. Does this patch work for you?
Hi, This change works for me. Please note that on row 96 of DirBundleFile you calculate again the canonical path of something that is canonical already. Thanks Violeta
(In reply to comment #20) > Hi, > > This change works for me. I released the fix with commit: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=5ad325360d09f89d8cb3766164e9340041aec535 > > Please note that on row 96 of DirBundleFile you calculate again the > canonical path of something that is canonical already. Then I noticed that I ignored your comment here! and released another fix for that with commit: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=671f70606091d87ad97c36ae13febe49cf69cf11 > > Thanks > Violeta Thanks you!
While trying to upgrade Netbinox to Equinox 3.9.1 https://netbeans.org/bugzilla/show_bug.cgi?id=240903 I have found one incompatible change caused by this issue. Rather than changing the constructor public DirBundleFile(File basefile) throws IOException; to have two arguments, adding another one and keeping the old one would be more compatible: public DirBundleFile(File basefile) throws IOException { this(basefile, false); } public DirBundleFile(File basefile, boolean enableStrictBundleEntryPath) throws IOException; Btw. otherwise the upgrade went more smoothly than last time (bug 292135 and bug 344850 back then). Thanks for being very compatible.
(In reply to Jaroslav Tulach from comment #22) > > Btw. otherwise the upgrade went more smoothly than last time (bug 292135 and > bug 344850 back then). Thanks for being very compatible. Keep in mind that the equinox adaptor interfaces are not considered API. We do not make guarantees that these interfaces will not change in incompatible ways from one release to the next. They simply have too many internal implementation details to do so. For the 4.4 Luna release the framework internals have been refactored to use a generic dependency model. This is going to require all Equinox specific adaptors implementations to migrate to the new internals.
