[JGIT PATCH] Permit a wider range of repository names in jgit daemon requests

[JGIT PATCH] Permit a wider range of repository names in jgit daemon requests

[Shawn O. Pearce]

The earlier restriction was too narrow for some applications, for
example repositories named "jgit.dev" and "jgit.test" are perfectly
valid Git repositories and should still be able to be served by
the daemon.

By blocking out only uses of ".." as a path component and Windows
UNC paths (by blocking "\") we can reasonably prevent the client
from escaping the base dirctories configured in the daemon.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 I knew I wrote this patch, but I couldn't find it in tree, or
 on the mailing list.  So I'm sending/resending it.  :-)

 .../src/org/spearce/jgit/transport/Daemon.java     |   42 +++++++++----------
 1 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java
index 646c88d..d39fd04 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java
@@ -51,7 +51,6 @@
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.regex.Pattern;
 
 import org.spearce.jgit.lib.Repository;
 
@@ -62,9 +61,6 @@
 
 	private static final int BACKLOG = 5;
 
-	private static final Pattern SAFE_REPOSITORY_NAME = Pattern
-			.compile("^[A-Za-z][A-Za-z0-9/_ -]+(\\.git)?$");
-
 	private InetSocketAddress myAddress;
 
 	private final DaemonService[] services;
@@ -292,24 +288,26 @@ synchronized (exports) {
 				return db;
 		}
 
-		if (SAFE_REPOSITORY_NAME.matcher(name).matches()) {
-			final File[] search;
-			synchronized (exportBase) {
-				search = exportBase.toArray(new File[exportBase.size()]);
-			}
-			for (final File f : search) {
-				db = openRepository(new File(f, name));
-				if (db != null)
-					return db;
-
-				db = openRepository(new File(f, name + ".git"));
-				if (db != null)
-					return db;
-
-				db = openRepository(new File(f, name + "/.git"));
-				if (db != null)
-					return db;
-			}
+		if (name.startsWith("../") || name.contains("/../")
+				|| name.contains("\\"))
+			return null;
+
+		final File[] search;
+		synchronized (exportBase) {
+			search = exportBase.toArray(new File[exportBase.size()]);
+		}
+		for (final File f : search) {
+			db = openRepository(new File(f, name));
+			if (db != null)
+				return db;
+
+			db = openRepository(new File(f, name + ".git"));
+			if (db != null)
+				return db;
+
+			db = openRepository(new File(f, name + "/.git"));
+			if (db != null)
+				return db;
 		}
 		return null;
 	}
-- 
1.6.1.422.g7298

Re: [JGIT PATCH] Permit a wider range of repository names in jgit daemon requests

[Robin Rosenberg]

måndag 09 februari 2009 04:58:03 skrev du:
> The earlier restriction was too narrow for some applications, for
> example repositories named "jgit.dev" and "jgit.test" are perfectly
> valid Git repositories and should still be able to be served by
> the daemon.
> 
> By blocking out only uses of ".." as a path component and Windows
> UNC paths (by blocking "\") we can reasonably prevent the client
> from escaping the base dirctories configured in the daemon.

Didn't I tell you // is also UNC-prefix? Windows treats / == \ at the API
level. Also why test for contains one "\"? And why in the middle? The
UNC prefix can only occur at the beginning of a path. You can use
File.isAbsolute to see if a name represents an absolute path. It is
platform-depdendent, so new File("//foo/bar").isAbsolute() yield
different results on Windows and unix platforms.

-- robin

[JGIT PATCH v2] Permit a wider range of repository names in jgit daemon requests

[Shawn O. Pearce]

The earlier restriction was too narrow for some applications, for
example repositories named "jgit.dev" and "jgit.test" are perfectly
valid Git repositories and should still be able to be served by
the daemon.

By blocking out only uses of ".." as a path component and Windows
UNC paths (by blocking "//") we can reasonably prevent the client
from escaping the base directories configured in the daemon.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
  Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
  > Didn't I tell you // is also UNC-prefix? Windows treats / == \ at the API
  > level. Also why test for contains one "\"? And why in the middle? The
  > UNC prefix can only occur at the beginning of a path. You can use
  > File.isAbsolute to see if a name represents an absolute path. It is
  > platform-depdendent, so new File("//foo/bar").isAbsolute() yield
  > different results on Windows and unix platforms.

  *sigh*... so *that's* what happened to this patch.  I posted it,
  you sent me comments, and I forgot to correct it.  Doh.

  Sorry for blasting you with the same patch twice.  I just really
  forgot that I even posted it, let alone that you had sent me back
  comments and that was why its not in tree yet.

  I think I've addressed your concerns above in this version.
  
 .../src/org/spearce/jgit/transport/Daemon.java     |   56 ++++++++++++--------
 1 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java
index 5087533..e5b446c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java
@@ -52,7 +52,6 @@
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.regex.Pattern;
 
 import org.spearce.jgit.lib.PersonIdent;
 import org.spearce.jgit.lib.Repository;
@@ -64,9 +63,6 @@
 
 	private static final int BACKLOG = 5;
 
-	private static final Pattern SAFE_REPOSITORY_NAME = Pattern
-			.compile("^[A-Za-z][A-Za-z0-9/_ -]+(\\.git)?$");
-
 	private InetSocketAddress myAddress;
 
 	private final DaemonService[] services;
@@ -286,8 +282,26 @@ synchronized DaemonService matchService(final String cmd) {
 	}
 
 	Repository openRepository(String name) {
+		// Assume any attempt to use \ was by a Windows client
+		// and correct to the more typical / used in Git URIs.
+		//
+		name = name.replace('\\', '/');
+
+		// git://thishost/path should always be name="/path" here
+		//
 		if (!name.startsWith("/"))
 			return null;
+
+		// Forbid Windows UNC paths as they might escape the base
+		//
+		if (name.startsWith("//"))
+			return null;
+
+		// Forbid funny paths which contain an up-reference, they
+		// might be trying to escape and read /../etc/password.
+		//
+		if (name.contains("/../"))
+			return null;
 		name = name.substring(1);
 
 		Repository db;
@@ -301,24 +315,22 @@ synchronized (exports) {
 				return db;
 		}
 
-		if (SAFE_REPOSITORY_NAME.matcher(name).matches()) {
-			final File[] search;
-			synchronized (exportBase) {
-				search = exportBase.toArray(new File[exportBase.size()]);
-			}
-			for (final File f : search) {
-				db = openRepository(new File(f, name));
-				if (db != null)
-					return db;
-
-				db = openRepository(new File(f, name + ".git"));
-				if (db != null)
-					return db;
-
-				db = openRepository(new File(f, name + "/.git"));
-				if (db != null)
-					return db;
-			}
+		final File[] search;
+		synchronized (exportBase) {
+			search = exportBase.toArray(new File[exportBase.size()]);
+		}
+		for (final File f : search) {
+			db = openRepository(new File(f, name));
+			if (db != null)
+				return db;
+
+			db = openRepository(new File(f, name + ".git"));
+			if (db != null)
+				return db;
+
+			db = openRepository(new File(f, name + "/.git"));
+			if (db != null)
+				return db;
 		}
 		return null;
 	}
-- 
1.6.2.rc0.173.g5e148