[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

[PATCH] Disallow going below root in svnserve - take two

From: Nuutti Kotivuori <naked_at_iki.fi>
Date: 2002-12-17 18:23:22 CET

So, now this patch seems ready to go in. It does three things, all
related to each other pretty much:

 - Prevent going below root with '..'.
 - Correctly UTF-8 translate root paths.
 - Makes root path absolute before using.

Currently 'svn_repos_open' isn't expecting an UTF-8 path, but that is
another bug to be fixed. And 'svnadmin' is already giving it UTF-8
paths anyway.

Do take a peek at the INT_ERR() macro there - it's stolen almost
verbatim from 'svnadmin', but I don't know if that's still the way to
go or if it should be handled differently.

If this seems fine, I'll commit it.

-- Naked

Log message:
Prevent using '..' in URLs to go below given root path in
svnserve. Also fix UTF-8 translation for root path. Also make given
root path absolute before using since apr_proc_detach changes the
current directory.

* subversion/svnserve/serve.c: Include svn_utf.h.
  (find_repos): Use apr_filepath_merge with APR_FILEPATH_SECUREROOT to
  combine root path with client path.
  (find_repos): Do UTF-8 translation before and after apr_filepath_merge.
  
* subversion/svnserve/main.c: Include svn_utf.h.
  (main): Translate root path to UTF-8.
  (main): Make root path absolute before detaching.

* subversion/libsvn_ra_svn/todo: Removed note in security about '..'.

Patch:
Index: subversion/libsvn_ra_svn/todo
===================================================================
--- subversion/libsvn_ra_svn/todo (revision 4141)
+++ subversion/libsvn_ra_svn/todo (working copy)
@@ -63,9 +63,6 @@
 There is no way to disable anonymous authentication. Fixing that
 will probably wait for a server configuration file.
 
-The now the client URL can escape from the logical root area using
-".." path elements. The server should check for that.
-
 Port number
 -----------
 
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 4141)
+++ subversion/svnserve/serve.c (working copy)
@@ -25,6 +25,7 @@
 #include <apr_strings.h>
 #include <apr_network_io.h>
 #include <apr_user.h>
+#include <apr_file_info.h>
 
 #include <svn_types.h>
 #include <svn_string.h>
@@ -35,6 +36,7 @@
 #include <svn_repos.h>
 #include <svn_path.h>
 #include <svn_time.h>
+#include <svn_utf.h>
 
 #include "server.h"
 
@@ -841,7 +843,10 @@
                                const char **fs_path, apr_pool_t *pool)
 {
   svn_error_t *err;
+ apr_status_t apr_err;
   const char *client_path, *full_path, *candidate;
+ const char *client_path_native, *root_native;
+ char *buffer;
 
   /* Decode any escaped characters in the URL. */
   url = svn_path_uri_decode(url, pool);
@@ -855,10 +860,25 @@
   client_path = strchr(url + 6, '/');
   client_path = (client_path == NULL) ? "" : client_path + 1;
 
+ SVN_ERR(svn_utf_cstring_from_utf8(&client_path_native,
+ svn_path_canonicalize(client_path, pool),
+ pool));
+
+ SVN_ERR(svn_utf_cstring_from_utf8(&root_native,
+ svn_path_canonicalize(root, pool),
+ pool));
+
   /* Join the server-configured root with the client path. */
- full_path = svn_path_join(svn_path_canonicalize(root, pool),
- svn_path_canonicalize(client_path, pool),
- pool);
+ apr_err = apr_filepath_merge(&buffer, root_native, client_path_native,
+ APR_FILEPATH_SECUREROOT, pool);
+
+ if(apr_err)
+ return svn_error_create(SVN_ERR_BAD_FILENAME, 0, NULL,
+ "Couldn't determine repository path.");
+
+ SVN_ERR(svn_utf_cstring_to_utf8(&full_path,
+ svn_path_canonicalize (buffer, pool),
+ NULL, pool));
 
   /* Search for a repository in the full path. */
   candidate = full_path;
Index: subversion/svnserve/main.c
===================================================================
--- subversion/svnserve/main.c (revision 4141)
+++ subversion/svnserve/main.c (working copy)
@@ -33,6 +33,8 @@
 #include "svn_pools.h"
 #include "svn_error.h"
 #include "svn_ra_svn.h"
+#include "svn_utf.h"
+#include "svn_path.h"
 
 #include "server.h"
 
@@ -49,6 +51,14 @@
   /* Nothing to do; we just need to interrupt the accept(). */
 }
 
+#define INT_ERR(expr) \
+ do { \
+ svn_error_t *svnserve_err__temp = (expr); \
+ if (svnserve_err__temp) { \
+ svn_handle_error (svnserve_err__temp, stderr, FALSE); \
+ return EXIT_FAILURE; } \
+ } while (0)
+
 int main(int argc, const char *const *argv)
 {
   svn_boolean_t listen_once = FALSE, daemon_mode = FALSE, tunnel_mode = FALSE;
@@ -94,7 +104,8 @@
           break;
 
         case 'r':
- root = arg;
+ INT_ERR(svn_utf_cstring_to_utf8(&root, arg, NULL, pool));
+ INT_ERR(svn_path_get_absolute(&root, root, pool));
           break;
         }
     }

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 17 18:24:12 2002

This is an archived mail posted to the Subversion Dev mailing list.