[GitHub] [mina-sshd] pyckle commented on a diff in pull request #362: Fix rooted filesystem
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 readAllLines(BufferedReader
reader, int lineCountHint
}
return result;
}
+
+/**
+ * Chroot a path under the new root
+ *
+ * @param newRootthe new root
+ * @param toSanitize the path to sanitize and chroot
+ * @returnthe chrooted path under the newRoot filesystem
+ */
+public static Path chroot(Path newRoot, Path toSanitize) {
+Objects.requireNonNull(newRoot);
+Objects.requireNonNull(toSanitize);
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+}
+
+/**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @returnthe sanitized path
+ */
+public static Path removeCdUpAboveRoot(Path toSanitize) {
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(),
sanitized);
+}
+
+private static List removeExtraCdUps(Path toResolve) {
+List 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
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-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] pyckle commented on a diff in pull request #362: Fix rooted filesystem
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 readAllLines(BufferedReader
reader, int lineCountHint
}
return result;
}
+
+/**
+ * Chroot a path under the new root
+ *
+ * @param newRootthe new root
+ * @param toSanitize the path to sanitize and chroot
+ * @returnthe chrooted path under the newRoot filesystem
+ */
+public static Path chroot(Path newRoot, Path toSanitize) {
+Objects.requireNonNull(newRoot);
+Objects.requireNonNull(toSanitize);
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+}
+
+/**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @returnthe sanitized path
+ */
+public static Path removeCdUpAboveRoot(Path toSanitize) {
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(),
sanitized);
+}
+
+private static List removeExtraCdUps(Path toResolve) {
+List 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
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-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] pyckle commented on a diff in pull request #362: Fix rooted filesystem
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 readAllLines(BufferedReader
reader, int lineCountHint
}
return result;
}
+
+/**
+ * Chroot a path under the new root
+ *
+ * @param newRootthe new root
+ * @param toSanitize the path to sanitize and chroot
+ * @returnthe chrooted path under the newRoot filesystem
+ */
+public static Path chroot(Path newRoot, Path toSanitize) {
+Objects.requireNonNull(newRoot);
+Objects.requireNonNull(toSanitize);
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+}
+
+/**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @returnthe sanitized path
+ */
+public static Path removeCdUpAboveRoot(Path toSanitize) {
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(),
sanitized);
+}
+
+private static List removeExtraCdUps(Path toResolve) {
+List 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
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 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 readAllLines(BufferedReader
reader, int lineCountHint
}
return result;
}
+
+/**
+ * Chroot a path under the new root
+ *
+ * @param newRootthe new root
+ * @param toSanitize the path to sanitize and chroot
+ * @returnthe chrooted path under the newRoot filesystem
+ */
+public static Path chroot(Path newRoot, Path toSanitize) {
+Objects.requireNonNull(newRoot);
+Objects.requireNonNull(toSanitize);
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(newRoot, newRoot.getFileSystem(), sanitized);
+}
+
+/**
+ * Remove any extra directory ups from the Path
+ *
+ * @param toSanitize the path to sanitize
+ * @returnthe sanitized path
+ */
+public static Path removeCdUpAboveRoot(Path toSanitize) {
+List sanitized = removeExtraCdUps(toSanitize);
+return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(),
sanitized);
+}
+
+private static List removeExtraCdUps(Path toResolve) {
+List 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
