You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Nuutti Kotivuori <na...@iki.fi> on 2002/12/17 17:23:22 UTC
[PATCH] Disallow going below root in svnserve - take two
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
Re: [PATCH] Disallow going below root in svnserve - take two
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Nuutti Kotivuori <na...@iki.fi> writes:
> Now when I took a peek, actually we use it in three places already -
> 'svnadmin', 'svnlook' and now 'svnserve'. So, yes, that should be in
> svn_error.h. But what should it be called, then? SVN_INT_ERR? I'm not
> sure I understand the original intent in the naming.
It handles an svn error then returns an integer error, right? So
SVN_INT_ERR is fine.
> No define - and the behaviour is almost the same. Error to stdout
> instead of stderr it seems, then the destruction of the pool and then
> return instead of an exit() call.
If you can convert that too, without changing the meaning, go for it.
> If there's a nice way to handle this all, someone could chime in and
> suggest it. But for the meantime, I'll put something like SVN_INT_ERR
> in svn_error.h and convert 'svnadmin' and 'svnlook' to using that.
Sounds good. Thanks for doing this!
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Disallow going below root in svnserve - take two
Posted by Nuutti Kotivuori <na...@iki.fi>.
Karl Fogel wrote:
> Nuutti Kotivuori <na...@iki.fi> writes:
>> 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 we're using the INT_ERR() in two places, and it's the same
> definition except for the internal symbol name, then it's probably
> time to abstract it into svn_error.h, what do you think?
Heh. Should've guessed that :-)
Now when I took a peek, actually we use it in three places already -
'svnadmin', 'svnlook' and now 'svnserve'. So, yes, that should be in
svn_error.h. But what should it be called, then? SVN_INT_ERR? I'm not
sure I understand the original intent in the naming.
Oh, and a peek to the commandline client reveals this:
,----
| err = svn_utf_cstring_to_utf8 (&utf8_opt_arg, opt_arg, NULL, pool);
|
| if (! err)
| err = svn_stringbuf_from_file (&buffer, utf8_opt_arg, pool);
| if (! err)
| err = svn_utf_stringbuf_to_utf8 (&buffer_utf8, buffer, pool);
| if (err)
| {
| svn_handle_error (err, stdout, FALSE);
| svn_pool_destroy (pool);
| return EXIT_FAILURE;
| }
`----
No define - and the behaviour is almost the same. Error to stdout
instead of stderr it seems, then the destruction of the pool and then
return instead of an exit() call.
If there's a nice way to handle this all, someone could chime in and
suggest it. But for the meantime, I'll put something like SVN_INT_ERR
in svn_error.h and convert 'svnadmin' and 'svnlook' to using that.
-- Naked
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Disallow going below root in svnserve - take two
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Nuutti Kotivuori <na...@iki.fi> writes:
> 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 we're using the INT_ERR() in two places, and it's the same
definition except for the internal symbol name, then it's probably
time to abstract it into svn_error.h, what do you think?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org