{"thread":{"id":"17668","subject":"[JGIT PATCH] Permit a wider range of repository names in jgit daemon requests","startedAt":"2009-02-09T03:58:03Z","lastAt":"2009-02-09T15:23:37Z","messageCount":3,"participants":["Shawn O. Pearce","Robin Rosenberg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"103811","messageId":"1234151883-30479-1-git-send-email-spearce@spearce.org","threadId":"17668","inReplyTo":null,"subject":"[JGIT PATCH] Permit a wider range of repository names in jgit daemon requests","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-02-09T03:58:03Z","receivedAt":"2009-02-09T03:58:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The earlier restriction was too narrow for some applications, for\nexample repositories named \"jgit.dev\" and \"jgit.test\" are perfectly\nvalid Git repositories and should still be able to be served by\nthe daemon.\n\nBy blocking out only uses of \"..\" as a path component and Windows\nUNC paths (by blocking \"\\\") we can reasonably prevent the client\nfrom escaping the base dirctories configured in the daemon.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n I knew I wrote this patch, but I couldn't find it in tree, or\n on the mailing list. So I'm sending/resending it. :-)\n\n .../src/org/spearce/jgit/transport/Daemon.java | 42 +++++++++----------\n 1 files changed, 20 insertions(+), 22 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java\nindex 646c88d..d39fd04 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java\n@@ -51,7 +51,6 @@\n import java.util.Collection;\n import java.util.HashMap;\n import java.util.Map;\n-import java.util.regex.Pattern;\n \n import org.spearce.jgit.lib.Repository;\n \n@@ -62,9 +61,6 @@\n \n \tprivate static final int BACKLOG = 5;\n \n-\tprivate static final Pattern SAFE_REPOSITORY_NAME = Pattern\n-\t\t\t.compile(\"^[A-Za-z][A-Za-z0-9/_ -]+(\\\\.git)?$\");\n-\n \tprivate InetSocketAddress myAddress;\n \n \tprivate final DaemonService[] services;\n@@ -292,24 +288,26 @@ synchronized (exports) {\n \t\t\t\treturn db;\n \t\t}\n \n-\t\tif (SAFE_REPOSITORY_NAME.matcher(name).matches()) {\n-\t\t\tfinal File[] search;\n-\t\t\tsynchronized (exportBase) {\n-\t\t\t\tsearch = exportBase.toArray(new File[exportBase.size()]);\n-\t\t\t}\n-\t\t\tfor (final File f : search) {\n-\t\t\t\tdb = openRepository(new File(f, name));\n-\t\t\t\tif (db != null)\n-\t\t\t\t\treturn db;\n-\n-\t\t\t\tdb = openRepository(new File(f, name + \".git\"));\n-\t\t\t\tif (db != null)\n-\t\t\t\t\treturn db;\n-\n-\t\t\t\tdb = openRepository(new File(f, name + \"/.git\"));\n-\t\t\t\tif (db != null)\n-\t\t\t\t\treturn db;\n-\t\t\t}\n+\t\tif (name.startsWith(\"../\") || name.contains(\"/../\")\n+\t\t\t\t|| name.contains(\"\\\\\"))\n+\t\t\treturn null;\n+\n+\t\tfinal File[] search;\n+\t\tsynchronized (exportBase) {\n+\t\t\tsearch = exportBase.toArray(new File[exportBase.size()]);\n+\t\t}\n+\t\tfor (final File f : search) {\n+\t\t\tdb = openRepository(new File(f, name));\n+\t\t\tif (db != null)\n+\t\t\t\treturn db;\n+\n+\t\t\tdb = openRepository(new File(f, name + \".git\"));\n+\t\t\tif (db != null)\n+\t\t\t\treturn db;\n+\n+\t\t\tdb = openRepository(new File(f, name + \"/.git\"));\n+\t\t\tif (db != null)\n+\t\t\t\treturn db;\n \t\t}\n \t\treturn null;\n \t}\n-- \n1.6.1.422.g7298\n"},{"id":"103819","messageId":"200902090847.40905.robin.rosenberg@dewire.com","threadId":"17668","inReplyTo":"1234151883-30479-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH] Permit a wider range of repository names in jgit daemon requests","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2009-02-09T07:47:40Z","receivedAt":"2009-02-09T07:47:40Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"måndag 09 februari 2009 04:58:03 skrev du:\n> The earlier restriction was too narrow for some applications, for\n> example repositories named \"jgit.dev\" and \"jgit.test\" are perfectly\n> valid Git repositories and should still be able to be served by\n> the daemon.\n> \n> By blocking out only uses of \"..\" as a path component and Windows\n> UNC paths (by blocking \"\\\") we can reasonably prevent the client\n> from escaping the base dirctories configured in the daemon.\n\nDidn't I tell you // is also UNC-prefix? Windows treats / == \\ at the API\nlevel. Also why test for contains one \"\\\"? And why in the middle? The\nUNC prefix can only occur at the beginning of a path. You can use\nFile.isAbsolute to see if a name represents an absolute path. It is\nplatform-depdendent, so new File(\"//foo/bar\").isAbsolute() yield\ndifferent results on Windows and unix platforms.\n\n-- robin\n"},{"id":"103848","messageId":"20090209152337.GH30949@spearce.org","threadId":"17668","inReplyTo":"200902090847.40905.robin.rosenberg@dewire.com","subject":"[JGIT PATCH v2] Permit a wider range of repository names in jgit daemon requests","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-02-09T15:23:37Z","receivedAt":"2009-02-09T15:23:37Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The earlier restriction was too narrow for some applications, for\nexample repositories named \"jgit.dev\" and \"jgit.test\" are perfectly\nvalid Git repositories and should still be able to be served by\nthe daemon.\n\nBy blocking out only uses of \"..\" as a path component and Windows\nUNC paths (by blocking \"//\") we can reasonably prevent the client\nfrom escaping the base directories configured in the daemon.\n\nSigned-off-by: Shawn O. Pearce \n---\n Robin Rosenberg wrote:\n > Didn't I tell you // is also UNC-prefix? Windows treats / == \\ at the API\n > level. Also why test for contains one \"\\\"? And why in the middle? The\n > UNC prefix can only occur at the beginning of a path. You can use\n > File.isAbsolute to see if a name represents an absolute path. It is\n > platform-depdendent, so new File(\"//foo/bar\").isAbsolute() yield\n > different results on Windows and unix platforms.\n\n *sigh*... so *that's* what happened to this patch. I posted it,\n you sent me comments, and I forgot to correct it. Doh.\n\n Sorry for blasting you with the same patch twice. I just really\n forgot that I even posted it, let alone that you had sent me back\n comments and that was why its not in tree yet.\n\n I think I've addressed your concerns above in this version.\n \n .../src/org/spearce/jgit/transport/Daemon.java | 56 ++++++++++++--------\n 1 files changed, 34 insertions(+), 22 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java\nindex 5087533..e5b446c 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Daemon.java\n@@ -52,7 +52,6 @@\n import java.util.Collection;\n import java.util.HashMap;\n import java.util.Map;\n-import java.util.regex.Pattern;\n \n import org.spearce.jgit.lib.PersonIdent;\n import org.spearce.jgit.lib.Repository;\n@@ -64,9 +63,6 @@\n \n \tprivate static final int BACKLOG = 5;\n \n-\tprivate static final Pattern SAFE_REPOSITORY_NAME = Pattern\n-\t\t\t.compile(\"^[A-Za-z][A-Za-z0-9/_ -]+(\\\\.git)?$\");\n-\n \tprivate InetSocketAddress myAddress;\n \n \tprivate final DaemonService[] services;\n@@ -286,8 +282,26 @@ synchronized DaemonService matchService(final String cmd) {\n \t}\n \n \tRepository openRepository(String name) {\n+\t\t// Assume any attempt to use \\ was by a Windows client\n+\t\t// and correct to the more typical / used in Git URIs.\n+\t\t//\n+\t\tname = name.replace('\\\\', '/');\n+\n+\t\t// git://thishost/path should always be name=\"/path\" here\n+\t\t//\n \t\tif (!name.startsWith(\"/\"))\n \t\t\treturn null;\n+\n+\t\t// Forbid Windows UNC paths as they might escape the base\n+\t\t//\n+\t\tif (name.startsWith(\"//\"))\n+\t\t\treturn null;\n+\n+\t\t// Forbid funny paths which contain an up-reference, they\n+\t\t// might be trying to escape and read /../etc/password.\n+\t\t//\n+\t\tif (name.contains(\"/../\"))\n+\t\t\treturn null;\n \t\tname = name.substring(1);\n \n \t\tRepository db;\n@@ -301,24 +315,22 @@ synchronized (exports) {\n \t\t\t\treturn db;\n \t\t}\n \n-\t\tif (SAFE_REPOSITORY_NAME.matcher(name).matches()) {\n-\t\t\tfinal File[] search;\n-\t\t\tsynchronized (exportBase) {\n-\t\t\t\tsearch = exportBase.toArray(new File[exportBase.size()]);\n-\t\t\t}\n-\t\t\tfor (final File f : search) {\n-\t\t\t\tdb = openRepository(new File(f, name));\n-\t\t\t\tif (db != null)\n-\t\t\t\t\treturn db;\n-\n-\t\t\t\tdb = openRepository(new File(f, name + \".git\"));\n-\t\t\t\tif (db != null)\n-\t\t\t\t\treturn db;\n-\n-\t\t\t\tdb = openRepository(new File(f, name + \"/.git\"));\n-\t\t\t\tif (db != null)\n-\t\t\t\t\treturn db;\n-\t\t\t}\n+\t\tfinal File[] search;\n+\t\tsynchronized (exportBase) {\n+\t\t\tsearch = exportBase.toArray(new File[exportBase.size()]);\n+\t\t}\n+\t\tfor (final File f : search) {\n+\t\t\tdb = openRepository(new File(f, name));\n+\t\t\tif (db != null)\n+\t\t\t\treturn db;\n+\n+\t\t\tdb = openRepository(new File(f, name + \".git\"));\n+\t\t\tif (db != null)\n+\t\t\t\treturn db;\n+\n+\t\t\tdb = openRepository(new File(f, name + \"/.git\"));\n+\t\t\tif (db != null)\n+\t\t\t\treturn db;\n \t\t}\n \t\treturn null;\n \t}\n-- \n1.6.2.rc0.173.g5e148\n"}]}