You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2023/04/18 11:33:41 UTC
[GitHub] [mina-sshd] gnodet opened a new pull request, #362: Fix rooted filesystem
gnodet opened a new pull request, #362:
URL: https://github.com/apache/mina-sshd/pull/362
- More unix like
- Symlinks improvements
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a diff in pull request #362: Fix rooted filesystem
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1172873194
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
+ if (namesList.isEmpty()) {
Review Comment:
Better to use *GenericUtils#isEmpty*
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
Review Comment:
Does not have to be a *List* - a *Collection* will do just fine.
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
+ if (namesList.isEmpty()) {
+ return root == null ? fs.getPath(".") : root;
+ }
+
+ Path cleanedPathToResolve = buildPath(fs, namesList);
+ return root.resolve(cleanedPathToResolve);
+ }
+
+ private static Path buildPath(FileSystem fs, List<String> namesList) {
Review Comment:
Should use *GenericUtils#isEmpty(nameList)* as well and handle the case (including throwing an **explicit** exception) if not allowed to be empty (or return *null* - whatever seems more reasonable). Personally I prefer returning *null*
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet merged pull request #362: [SSHD-1324] Rooted file system can leak informations
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #362:
URL: https://github.com/apache/mina-sshd/pull/362
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #362: Fix rooted filesystem
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1516999345
It would be extremely helpful to have meaningful commit messages. Just "More unix like" is _not_ helpful. If someone looks at this code a few months or years from now he or she will have no idea what this is about from the git history. There's not even any link to any issue, PR, or mailing list message. In this case I do not expect the commit message to give a full explanation, but brief descriptions should be possible.
At the very least there must be a meaningful summary line so that `git log --oneline` gives a result that correctly summarizes the scope and topic.
Absent a JIRA or Github issue, a link back to this PR might also helpful.
Something like
```
RootedFileSystem: more Unix-like path handling
Clean up and fix path checks to be more like in Unix.
Add unit tests for Unix, OSX, and Windows via JimFs.
See also [1].
[1] https://github.com/apache/mina-sshd/pull/362
```
should be the minimum for the first commit. And likewise for the second commit.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a diff in pull request #362: Fix rooted filesystem
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1174607919
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
Review Comment:
It is a matter of style - I believe in total user responsibility - that is why I allow a *Collection* - I see it the **user**'s responsibility to enforce order. If the user wants to use a *Set* then it is his/her responsibility. You seem to adopt a more paternalistic approach that feels that one should protect users from themselves. It is a valid approach, but one that I do not subscribe to as (IMO) it inevitably leads to rigid code. My approach is simple: use the "weakest" parameter that you need to achieve your goal - the user is responsible for correct invocation.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] pyckle commented on a diff in pull request #362: Fix rooted filesystem
Posted by "pyckle (via GitHub)" <gi...@apache.org>.
pyckle commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1174612833
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
Review Comment:
I can agree with this approach in many situations, but there's no practical use case here. Having a `Set` (or any other unordered collection for that matter) for a set of path components makes absolutely no sense. The `Collection` type in the signature only will confuse a developer as the signature doesn't semantically reflect what the function accomplishes.
And if this scenario was a use case (which I would be very curious to see), calling `buildPath(..., new ArrayList(namesCollection),...)` would be an easy way to use this function.
With that said, I'm not a maintainer, and I made a PR for @gnodet which makes this change (and fixes the other issues) here: https://github.com/gnodet/mina-sshd/pull/2/files
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] 07070529 commented on pull request #362: [SSHD-1324] Rooted file system can leak informations
Posted by "07070529 (via GitHub)" <gi...@apache.org>.
07070529 commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1664847859
Thanks a lot for your reply, and we appreciate your prompt attention to this issue.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on pull request #362: [SSHD-1324] Rooted file system can leak informations
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1531465871
@tomaswolf I plan to squash the commits into a single commit
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] pyckle commented on a diff in pull request #362: Fix rooted filesystem
Posted by "pyckle (via GitHub)" <gi...@apache.org>.
pyckle commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1174612833
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
Review Comment:
I can agree with this approach in many situations, but there's no practical use case here. Having a `Set` (or any other unordered collection for that matter) for a set of path components makes absolutely no sense. The `Collection` type in the signature only will confuse a developer as the signature doesn't semantically reflect what the function accomplishes.
And if this scenario was a use case (which I would be very curious to see), calling `buildPath(..., new ArrayList(namesCollection),...)` would be an easy way to use this function.
With that said, I'm not a maintainer, and I made a PR for @gnodet which makes this change (and addresses the other issues) here: https://github.com/gnodet/mina-sshd/pull/2/files
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on pull request #362: [SSHD-1324] Rooted file system can leak informations
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1664219647
> We'd greatly appreciate it if you could give us some advice on whether the [CVE-2023-35887](https://github.com/advisories/GHSA-mjmq-gwgm-5qhm) vulnerability affects Apache MINA 2.1.X and 2.2.X?
The issue affects the Apache Mina *SSHD* project, not the Apache Mina library.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] pyckle commented on pull request #362: Fix rooted filesystem
Posted by "pyckle (via GitHub)" <gi...@apache.org>.
pyckle commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1519110747
I made the suggested changes here: https://github.com/gnodet/mina-sshd/pull/2/files
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #362: Fix rooted filesystem
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1174415617
##########
sshd-common/src/test/java/org/apache/sshd/common/file/root/RootedFileSystemProviderTest.java:
##########
@@ -20,60 +20,118 @@
package org.apache.sshd.common.file.root;
import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.nio.channels.Channel;
import java.nio.channels.FileChannel;
+import java.nio.charset.StandardCharsets;
import java.nio.file.DirectoryStream;
import java.nio.file.FileSystem;
+import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
+import java.nio.file.LinkOption;
+import java.nio.file.NoSuchFileException;
import java.nio.file.OpenOption;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
+import java.util.List;
import java.util.Objects;
import java.util.Random;
import java.util.TreeSet;
+import java.util.function.Supplier;
+import com.google.common.jimfs.Configuration;
+import com.google.common.jimfs.Jimfs;
import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.util.test.CommonTestSupportUtils;
import org.apache.sshd.util.test.NoIoTestCase;
+import org.junit.AfterClass;
+import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.FixMethodOrder;
import org.junit.Test;
import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
import org.junit.runners.MethodSorters;
+import org.junit.runners.Parameterized;
/**
* Tests the RootedFileSystemProvider implementation of {@link java.nio.file.spi.FileSystemProvider} checking that
* permissions for generic FS commands are not permitted outside of the root directory.
- *
+ * <p>
* Individual tests are form pairs (e.g. testX, testXInvalid) where testXInvalid is expected to test a parent path of
* {@link RootedFileSystem#getRoot()}
*
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
@Category({ NoIoTestCase.class })
+@RunWith(Parameterized.class)
public class RootedFileSystemProviderTest extends AssertableFile {
- private static RootedFileSystem fileSystem;
- private static Path rootSandbox;
+ private static final String DOESNT_EXIST = "../doesnt_exist/../";
- public RootedFileSystemProviderTest() {
- super();
- }
+ private static FileSystem unixInMemFs;
+ private static FileSystem windowsInMemFs;
+ private static FileSystem osxInMemFs;
+ private final FileSystem hostFilesystem;
+ private final RootedFileSystem fileSystem;
+ private final Path rootSandbox;
+ private final FileHelper fileHelper;
- @BeforeClass
- public static void initializeFileSystem() throws IOException {
+ public RootedFileSystemProviderTest(String fsType, Supplier<FileSystem> hostFilesystem) throws Exception {
+ super();
+ this.hostFilesystem = hostFilesystem.get();
Path targetFolder = Objects.requireNonNull(
CommonTestSupportUtils.detectTargetFolder(RootedFileSystemProviderTest.class),
"Failed to detect target folder");
- rootSandbox = FileHelper.createTestSandbox(targetFolder.resolve(TEMP_SUBFOLDER_NAME));
+ Path targetFolderOnHostFs = getTargetFolderOnHostFs(targetFolder);
+ fileHelper = new FileHelper();
+ rootSandbox = fileHelper.createTestSandbox(targetFolderOnHostFs.resolve(TEMP_SUBFOLDER_NAME));
fileSystem = (RootedFileSystem) new RootedFileSystemProvider().newFileSystem(rootSandbox, Collections.emptyMap());
}
+ @Parameterized.Parameters(name = "{0}FS")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(
+ new Object[] { "Windows", (Supplier<FileSystem>) () -> windowsInMemFs },
+ new Object[] { "Unix", (Supplier<FileSystem>) () -> unixInMemFs },
+ new Object[] { "MacOS", (Supplier<FileSystem>) () -> osxInMemFs },
+ new Object[] { "Native", (Supplier<FileSystem>) FileSystems::getDefault });
+ }
+
+ @BeforeClass
+ public static void initializeFileSystem() {
+ unixInMemFs = Jimfs.newFileSystem(Configuration.unix());
+ windowsInMemFs = Jimfs.newFileSystem(Configuration.windows());
+ osxInMemFs = Jimfs.newFileSystem(Configuration.osX());
+ }
+
+ @AfterClass
+ public static void afterClass() {
+ List<Exception> failures = new ArrayList<>();
+ for (FileSystem fs : Arrays.asList(unixInMemFs, windowsInMemFs, osxInMemFs)) {
+ try {
+ fs.close();
+ } catch (Exception ex) {
+ failures.add(ex);
+ }
+ }
+ Assert.assertEquals(Collections.emptyList(), failures);
+ }
+
+ private Path getTargetFolderOnHostFs(Path targetFolder) {
+ return this.hostFilesystem.getSeparator().equals("\\")
+ ? this.hostFilesystem.getPath("C:", targetFolder.toString())
Review Comment:
This looks suspicious. In CI, we're on drive D:.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] 07070529 commented on pull request #362: [SSHD-1324] Rooted file system can leak informations
Posted by "07070529 (via GitHub)" <gi...@apache.org>.
07070529 commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1663958270
@gnodet Hi, we see from the NVD that this issue is related to vulnerability CVE-2023-35887 (https://nvd.nist.gov/vuln/detail/CVE-2023-35887).
Then in the content of the reference link (https://lists.apache.org/thread/b9qgtqvhnvgfpn0w1gz918p21p53tqk2), we see:
This issue affects Apache MINA: from 1.0 before 2.10.
Sorry, we're a little confused:
1. The affected software scope is different with the title (Affected versions: Apache MINA SSHD 1.0 before 2.10).
2. We looked for a lot of information, including the Apache MINA community, but we didn't see any discussion about whether this issue affected the Apache MINA.
We'd greatly appreciate it if you could give us some advice on whether the CVE-2023-35887 vulnerability affects Apache MINA?
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on pull request #362: [SSHD-1324] Rooted file system can leak informations
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1540575190
> I suggest we publish a 2.10.0 without this change, and do a 2.10.1 once this is ready.
I second that...
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a diff in pull request #362: Fix rooted filesystem
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1172862559
##########
sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedDirectoryStream.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.sshd.common.file.root;
+
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Path;
+import java.util.Iterator;
+
+/**
+ * secure directory stream proxy for a {@link RootedFileSystem}
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ */
+class RootedDirectoryStream implements DirectoryStream<Path> {
Review Comment:
I think this class (and its constructor) should be *public* since we want to give our users the flexibility to implement behavior override if they should feel like it.
##########
sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemUtils.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.sshd.common.file.root;
+
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+
+/**
+ * Utility functions for rooted file utils
+ */
+public final class RootedFileSystemUtils {
+
+ private RootedFileSystemUtils() {
+ // do not construct
+ }
+
+ static void validateSafeRelativeSymlink(Path target) {
Review Comment:
I believe the method should also be *public*
##########
sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedSecureDirectoryStream.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.sshd.common.file.root;
+
+import java.io.IOException;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.LinkOption;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.SecureDirectoryStream;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.FileAttributeView;
+import java.util.Set;
+
+/**
+ * A secure directory stream proxy for a {@link RootedFileSystem}
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ */
+class RootedSecureDirectoryStream extends RootedDirectoryStream implements SecureDirectoryStream<Path> {
+
+ RootedSecureDirectoryStream(RootedFileSystem rfs, SecureDirectoryStream<Path> delegate) {
+ super(rfs, delegate);
+ }
+
+ @Override
+ public SecureDirectoryStream<Path> newDirectoryStream(Path path, LinkOption... options) throws IOException {
+ return new RootedSecureDirectoryStream(rfs, delegate().newDirectoryStream(fixPath(path), options));
+ }
+
+ private Path fixPath(Path p) {
Review Comment:
Should be *protected* in order to allow users to override the behavior if they should wish to do so.
##########
sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedSecureDirectoryStream.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.sshd.common.file.root;
+
+import java.io.IOException;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.LinkOption;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.SecureDirectoryStream;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.FileAttributeView;
+import java.util.Set;
+
+/**
+ * A secure directory stream proxy for a {@link RootedFileSystem}
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ */
+class RootedSecureDirectoryStream extends RootedDirectoryStream implements SecureDirectoryStream<Path> {
Review Comment:
See previous comments regarding *public* visibility...
##########
sshd-common/src/main/java/org/apache/sshd/common/file/util/BaseFileSystem.java:
##########
@@ -121,6 +121,23 @@ protected void appendDedupSep(StringBuilder sb, CharSequence s) {
}
}
+ /**
+ * In case we are running on Windows, accept "\\" as a file separator. Ignore in *nix as "\\" is a valid filename
+ *
+ * @param name the name to fix the separator for if running on Windows
+ * @return the fixed name
+ */
+ private String handleWindowsSeparator(String name) {
Review Comment:
Should be *protected* in order to allow users to override the behavior if they should wish to do so.
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -415,6 +442,19 @@ public static Boolean checkFileExists(Path path, LinkOption... options) {
}
}
+ private static Path getPartialPath(Path path, int partsToExtract) {
Review Comment:
Should be *public* in case users want/need to use it
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
Review Comment:
Should be *public* in case users want/need to use it
##########
sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java:
##########
@@ -2406,6 +2428,31 @@ protected NavigableMap<String, Object> resolveFileAttributes(
});
}
+ /**
+ * A utility function to validate that the directories leading up to a file are not symlinks when they are not
+ * supposed to be.
+ *
+ * @param path the file to check for symlink presence
+ * @param neverFollowSymLinks whether to never follow symlinks in the parent paths
+ * @param options whether the file itself can be a symlink
+ * @return the status of the file
+ */
+ private static Boolean checkSymlinkState(Path path, boolean neverFollowSymLinks, LinkOption[] options) {
+ Boolean status = validateParentExistWithNoSymlinksIfNeverFollowSymlinks(path, neverFollowSymLinks);
+ if (!Boolean.FALSE.equals(status)) {
+ status = IoUtils.checkFileExists(path, options);
+ }
+ return status;
+ }
+
+ private static Boolean validateParentExistWithNoSymlinksIfNeverFollowSymlinks(Path path, boolean neverFollowSymLinks) {
Review Comment:
Should be *public* in case users want/need to use it
##########
sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java:
##########
@@ -2406,6 +2428,31 @@ protected NavigableMap<String, Object> resolveFileAttributes(
});
}
+ /**
+ * A utility function to validate that the directories leading up to a file are not symlinks when they are not
+ * supposed to be.
+ *
+ * @param path the file to check for symlink presence
+ * @param neverFollowSymLinks whether to never follow symlinks in the parent paths
+ * @param options whether the file itself can be a symlink
+ * @return the status of the file
+ */
+ private static Boolean checkSymlinkState(Path path, boolean neverFollowSymLinks, LinkOption[] options) {
Review Comment:
Should be *public* in case users want/need to use it
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
+ if (namesList.isEmpty()) {
+ return root == null ? fs.getPath(".") : root;
+ }
+
+ Path cleanedPathToResolve = buildPath(fs, namesList);
+ return root.resolve(cleanedPathToResolve);
+ }
+
+ private static Path buildPath(FileSystem fs, List<String> namesList) {
Review Comment:
Should be *public* in case users want/need to use it
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a diff in pull request #362: Fix rooted filesystem
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1172872427
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
+ if (namesList.isEmpty()) {
+ return root == null ? fs.getPath(".") : root;
+ }
+
+ Path cleanedPathToResolve = buildPath(fs, namesList);
+ return root.resolve(cleanedPathToResolve);
+ }
+
+ private static Path buildPath(FileSystem fs, List<String> namesList) {
Review Comment:
Does not have to be a *List* - a *Collection* will do just fine.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a diff in pull request #362: Fix rooted filesystem
Posted by "lgoldstein (via GitHub)" <gi...@apache.org>.
lgoldstein commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1174440169
##########
sshd-common/src/test/java/org/apache/sshd/common/file/root/RootedFileSystemProviderTest.java:
##########
@@ -20,60 +20,118 @@
package org.apache.sshd.common.file.root;
import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.nio.channels.Channel;
import java.nio.channels.FileChannel;
+import java.nio.charset.StandardCharsets;
import java.nio.file.DirectoryStream;
import java.nio.file.FileSystem;
+import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
+import java.nio.file.LinkOption;
+import java.nio.file.NoSuchFileException;
import java.nio.file.OpenOption;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
+import java.util.List;
import java.util.Objects;
import java.util.Random;
import java.util.TreeSet;
+import java.util.function.Supplier;
+import com.google.common.jimfs.Configuration;
+import com.google.common.jimfs.Jimfs;
import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.util.test.CommonTestSupportUtils;
import org.apache.sshd.util.test.NoIoTestCase;
+import org.junit.AfterClass;
+import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.FixMethodOrder;
import org.junit.Test;
import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
import org.junit.runners.MethodSorters;
+import org.junit.runners.Parameterized;
/**
* Tests the RootedFileSystemProvider implementation of {@link java.nio.file.spi.FileSystemProvider} checking that
* permissions for generic FS commands are not permitted outside of the root directory.
- *
+ * <p>
* Individual tests are form pairs (e.g. testX, testXInvalid) where testXInvalid is expected to test a parent path of
* {@link RootedFileSystem#getRoot()}
*
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
@Category({ NoIoTestCase.class })
+@RunWith(Parameterized.class)
public class RootedFileSystemProviderTest extends AssertableFile {
- private static RootedFileSystem fileSystem;
- private static Path rootSandbox;
+ private static final String DOESNT_EXIST = "../doesnt_exist/../";
- public RootedFileSystemProviderTest() {
- super();
- }
+ private static FileSystem unixInMemFs;
+ private static FileSystem windowsInMemFs;
+ private static FileSystem osxInMemFs;
+ private final FileSystem hostFilesystem;
+ private final RootedFileSystem fileSystem;
+ private final Path rootSandbox;
+ private final FileHelper fileHelper;
- @BeforeClass
- public static void initializeFileSystem() throws IOException {
+ public RootedFileSystemProviderTest(String fsType, Supplier<FileSystem> hostFilesystem) throws Exception {
+ super();
+ this.hostFilesystem = hostFilesystem.get();
Path targetFolder = Objects.requireNonNull(
CommonTestSupportUtils.detectTargetFolder(RootedFileSystemProviderTest.class),
"Failed to detect target folder");
- rootSandbox = FileHelper.createTestSandbox(targetFolder.resolve(TEMP_SUBFOLDER_NAME));
+ Path targetFolderOnHostFs = getTargetFolderOnHostFs(targetFolder);
+ fileHelper = new FileHelper();
+ rootSandbox = fileHelper.createTestSandbox(targetFolderOnHostFs.resolve(TEMP_SUBFOLDER_NAME));
fileSystem = (RootedFileSystem) new RootedFileSystemProvider().newFileSystem(rootSandbox, Collections.emptyMap());
}
+ @Parameterized.Parameters(name = "{0}FS")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(
+ new Object[] { "Windows", (Supplier<FileSystem>) () -> windowsInMemFs },
+ new Object[] { "Unix", (Supplier<FileSystem>) () -> unixInMemFs },
+ new Object[] { "MacOS", (Supplier<FileSystem>) () -> osxInMemFs },
+ new Object[] { "Native", (Supplier<FileSystem>) FileSystems::getDefault });
+ }
+
+ @BeforeClass
+ public static void initializeFileSystem() {
+ unixInMemFs = Jimfs.newFileSystem(Configuration.unix());
+ windowsInMemFs = Jimfs.newFileSystem(Configuration.windows());
+ osxInMemFs = Jimfs.newFileSystem(Configuration.osX());
+ }
+
+ @AfterClass
+ public static void afterClass() {
+ List<Exception> failures = new ArrayList<>();
+ for (FileSystem fs : Arrays.asList(unixInMemFs, windowsInMemFs, osxInMemFs)) {
+ try {
+ fs.close();
+ } catch (Exception ex) {
+ failures.add(ex);
+ }
+ }
+ Assert.assertEquals(Collections.emptyList(), failures);
+ }
+
+ private Path getTargetFolderOnHostFs(Path targetFolder) {
+ return this.hostFilesystem.getSeparator().equals("\\")
+ ? this.hostFilesystem.getPath("C:", targetFolder.toString())
Review Comment:
I second that - someone else might run on drive `X:` or `Z:` (or any other letter of the alphabet). The code should auto-detect if running on Windows and then also auto detect the drive letter.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] pyckle commented on a diff in pull request #362: Fix rooted filesystem
Posted by "pyckle (via GitHub)" <gi...@apache.org>.
pyckle commented on code in PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#discussion_r1174578418
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
+ if (namesList.isEmpty()) {
+ return root == null ? fs.getPath(".") : root;
+ }
+
+ Path cleanedPathToResolve = buildPath(fs, namesList);
+ return root.resolve(cleanedPathToResolve);
+ }
+
+ private static Path buildPath(FileSystem fs, List<String> namesList) {
Review Comment:
A List is an ordered collection. Why should this API permit an unordered collection?
##########
sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java:
##########
@@ -637,4 +677,75 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint
}
return result;
}
+
+ /**
+ * Chroot a path under the new root
+ *
+ * @param newRoot the new root
+ * @param toSanitize the path to sanitize and chroot
+ * @return the chrooted path under the newRoot filesystem
+ */
+ public static Path chroot(Path newRoot, Path toSanitize) {
+ Objects.requireNonNull(newRoot);
+ Objects.requireNonNull(toSanitize);
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+ }
+
+ /**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @return the sanitized path
+ */
+ public static Path removeCdUpAboveRoot(Path toSanitize) {
+ List<String> sanitized = removeExtraCdUps(toSanitize);
+ return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized);
+ }
+
+ private static List<String> removeExtraCdUps(Path toResolve) {
+ List<String> newNames = new ArrayList<>(toResolve.getNameCount());
+
+ int numCdUps = 0;
+ int numDirParts = 0;
+ for (int i = 0; i < toResolve.getNameCount(); i++) {
+ String name = toResolve.getName(i).toString();
+ if ("..".equals(name)) {
+ // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes
+ if (numDirParts > numCdUps) {
+ ++numCdUps;
+ newNames.add(name);
+ }
+ } else {
+ // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't
+ // add to the number of ".."s that can be present before the root
+ if (!".".equals(name)) {
+ ++numDirParts;
+ }
+ newNames.add(name);
+ }
+ }
+ return newNames;
+ }
+
+ private static Path buildPath(Path root, FileSystem fs, List<String> namesList) {
Review Comment:
A List is an ordered collection. Why should this API permit an unordered collection?
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #362: [SSHD-1324] Rooted file system can leak informations
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #362:
URL: https://github.com/apache/mina-sshd/pull/362#issuecomment-1537180084
@gnodet: seems this needs more work. I suggest we publish a 2.10.0 without this change, and do a 2.10.1 once this is ready. I'd really like to have 2.10.0 in Eclipse 2023-06, but to make that work, I'll need a release next week. Otherwise it'll be too late in the Eclipse release process.
--
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: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org