You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Chen <qu...@hkn.eecs.berkeley.edu> on 2003/11/24 07:11:28 UTC
annoyance in error reporting
When Subversion can't access a repository it doesn't give
information as to why not. For example, if a grandparent
directory needs chmod a+rx all you'll get is
svn: Unsupported repository version
svn: PROPFIND request failed on '/repos/quarl/trunk/bookmarks'
svn:
Expected version '2' of repository; found no version at all; is '/home/quarl/REPOS/quarl' a valid repository path?
--
Karl Chen 2003-11-23 23:09
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by kf...@collab.net.
Karl Chen <qu...@hkn.eecs.berkeley.edu> writes:
> When Subversion can't access a repository it doesn't give
> information as to why not. For example, if a grandparent
> directory needs chmod a+rx all you'll get is
>
> svn: Unsupported repository version
> svn: PROPFIND request failed on '/repos/quarl/trunk/bookmarks'
> svn:
> Expected version '2' of repository; found no version at all; is '/home/quarl/REPOS/quarl' a valid repository path?
Could you attach the reproduction recipe for this as an example to
http://subversion.tigris.org/issues/show_bug.cgi?id=1254
Thank you,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by mark benedetto king <mb...@lowlatency.com>.
On Mon, Nov 24, 2003 at 04:13:38PM -0500, Greg Hudson wrote:
> On Mon, 2003-11-24 at 15:13, kfogel@collab.net wrote:
> > Greg Hudson <gh...@MIT.EDU> writes:
> > > So now we're relying on dav_svn_convert_err only converting the
> > > top-level error? That seems like the wrong direction to move in.
> >
> > Sorry, I just misremembered what svn_error_quick_wrap() does. Rather
> > odd, considering its name! Here's a new patch:
>
> I think I'd prefer to just not mention the filename in error text in the
> svn_repos_open() path. But I can see it going either way.
>
I like the fact that the path gets logged to the apache log files; this
means that administrators having trouble configuring their system can
see exactly what path mod_dav_svn is trying to open.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> I think I'd prefer to just not mention the filename in error text in the
> svn_repos_open() path. But I can see it going either way.
I think for an internal interface, it's nice to be more informative
(for example, this way we can have that error in the server logs
conveniently, which could help the administrator).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-11-24 at 15:13, kfogel@collab.net wrote:
> Greg Hudson <gh...@MIT.EDU> writes:
> > So now we're relying on dav_svn_convert_err only converting the
> > top-level error? That seems like the wrong direction to move in.
>
> Sorry, I just misremembered what svn_error_quick_wrap() does. Rather
> odd, considering its name! Here's a new patch:
I think I'd prefer to just not mention the filename in error text in the
svn_repos_open() path. But I can see it going either way.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by mark benedetto king <mb...@lowlatency.com>.
On Mon, Nov 24, 2003 at 02:13:44PM -0600, kfogel@collab.net wrote:
> Greg Hudson <gh...@MIT.EDU> writes:
> > So now we're relying on dav_svn_convert_err only converting the
> > top-level error? That seems like the wrong direction to move in.
>
> Sorry, I just misremembered what svn_error_quick_wrap() does. Rather
> odd, considering its name! Here's a new patch:
>
> -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*-
>
> Fix issue #1501: mod_dav_svn can leak real repository path to client.
>
Committed in r7930. Thanks, and sorry it took me so long to test it.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> So now we're relying on dav_svn_convert_err only converting the
> top-level error? That seems like the wrong direction to move in.
Sorry, I just misremembered what svn_error_quick_wrap() does. Rather
odd, considering its name! Here's a new patch:
-*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*-
Fix issue #1501: mod_dav_svn can leak real repository path to client.
* subversion/mod_dav_svn/repos.c
(dav_svn_get_resource): Use one error for the server logs, another
for dav and thence to the client.
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 7837)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -1194,11 +1194,22 @@
serr = svn_repos_open(&(repos->repos), fs_path, r->connection->pool);
if (serr != NULL)
{
- return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
- apr_psprintf(r->pool,
- "Could not open the SVN "
- "filesystem at %s",
- fs_path));
+ /* The error returned by svn_repos_open might contain the
+ actual path to the failed repository. We don't want to
+ leak that path back to the client, because that would be
+ a security risk, but we do want to log the real error on
+ the server side. */
+ const char *new_msg = "Could not open the requested SVN filesystem";
+ svn_error_t *sanitized_error = svn_error_create(serr->apr_err,
+ NULL, new_msg);
+
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, r,
+ "%s", serr->message);
+
+ /* Return a slightly less informative error to dav. */
+ return dav_svn_convert_err (sanitized_error,
+ HTTP_INTERNAL_SERVER_ERROR,
+ apr_psprintf(r->pool, new_msg));
}
/* Cache the open repos for the next request on this connection */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-11-24 at 14:54, kfogel@collab.net wrote:
> + svn_error_t *sanitized_error = svn_error_quick_wrap(serr, new_msg);
> +
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, r,
> + "%s", serr->message);
> +
> + /* Return a slightly less informative error to dav. */
> + return dav_svn_convert_err (sanitized_error,
> + HTTP_INTERNAL_SERVER_ERROR,
> + apr_psprintf(r->pool, new_msg));
So now we're relying on dav_svn_convert_err only converting the
top-level error? That seems like the wrong direction to move in.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by kf...@collab.net.
mark benedetto king <mb...@lowlatency.com> writes:
> Also, there is issue 1051: "mod_dav_svn displays real path to repository".
>
> This is a real information leak that is likely to be "discovered" by
> third-party auditors and reported on security mailing lists, and will
> give mod_dav_svn an undeserved black eye.
>
> I'd like to make issue 1051 a requirement for 1.0, or document clearly
> in INSTALL that an empty repository (or a misconfigured one) can result
> in this behaviour.
This leak can only happen if one has misconfigured one's server -- you
have to have an SVN location pointing to an empty (uninitialized)
repository, then the leak can happen. So I don't think this needs to
be a 1.0 requirement -- the client can't stimulate this leak from a
well-configured server.
Nevertheless, it sure would be nice get it fixed :-). I have an
untested patch; could you try it out and let me know if it's right?
-*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*- -*-
Fix issue #1501: mod_dav_svn can leak real repository path to client.
* subversion/mod_dav_svn/repos.c
(dav_svn_get_resource): Use one error for the server logs, another
for dav and thence to the client.
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 7837)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -1194,11 +1194,21 @@
serr = svn_repos_open(&(repos->repos), fs_path, r->connection->pool);
if (serr != NULL)
{
- return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
- apr_psprintf(r->pool,
- "Could not open the SVN "
- "filesystem at %s",
- fs_path));
+ /* The error returned by svn_repos_open might contain the
+ actual path to the failed repository. We don't want to
+ leak that path back to the client, because that would be
+ a security risk, but we do want to log the real error on
+ the server side. */
+ const char *new_msg = "Could not open the requested SVN filesystem";
+ svn_error_t *sanitized_error = svn_error_quick_wrap(serr, new_msg);
+
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, r,
+ "%s", serr->message);
+
+ /* Return a slightly less informative error to dav. */
+ return dav_svn_convert_err (sanitized_error,
+ HTTP_INTERNAL_SERVER_ERROR,
+ apr_psprintf(r->pool, new_msg));
}
/* Cache the open repos for the next request on this connection */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by mark benedetto king <mb...@lowlatency.com>.
On Mon, Nov 24, 2003 at 11:49:46AM -0500, Greg Hudson wrote:
> On Mon, 2003-11-24 at 02:11, Karl Chen wrote:
> > When Subversion can't access a repository it doesn't give
> > information as to why not. For example, if a grandparent
> > directory needs chmod a+rx all you'll get is
> >
> > svn: Unsupported repository version
> > svn: PROPFIND request failed on '/repos/quarl/trunk/bookmarks'
> > svn:
> > Expected version '2' of repository; found no version at all; is '/home/quarl/REPOS/quarl' a valid repository path?
>
> I see a few problems here, the first two of which are the most
> important:
>
Also, there is issue 1051: "mod_dav_svn displays real path to repository".
This is a real information leak that is likely to be "discovered" by
third-party auditors and reported on security mailing lists, and will
give mod_dav_svn an undeserved black eye.
I'd like to make issue 1051 a requirement for 1.0, or document clearly
in INSTALL that an empty repository (or a misconfigured one) can result
in this behaviour.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: annoyance in error reporting
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-11-24 at 02:11, Karl Chen wrote:
> When Subversion can't access a repository it doesn't give
> information as to why not. For example, if a grandparent
> directory needs chmod a+rx all you'll get is
>
> svn: Unsupported repository version
> svn: PROPFIND request failed on '/repos/quarl/trunk/bookmarks'
> svn:
> Expected version '2' of repository; found no version at all; is '/home/quarl/REPOS/quarl' a valid repository path?
I see a few problems here, the first two of which are the most
important:
(1) In repos.c:check_repos_version(), if we get any error reading the
format file, we wrap the "found no version at all" error around it,
which is a confusing message when the underlying error is anything other
than file-not-found. (Which, ironically, it will almost never be,
because the URL-splitting algorithm prevents us from trying a path which
doesn't contain a format file.)
(2) dav_svn_convert_err() only converts the topmost error of an error
chain. In this case, the important detail is in a lower-level error.
(3) Telling the user which HTTP request failed is of marginal value,
and it's pretty confusing to the user who isn't familiar with DAV and
DeltaV. The client's error message should stick to explaining the
problem in user's terms, i.e. that the given URL does not appear to have
been successfully configured as a Subversion repository.
(4) The topmost error code in the server's error chain
(SVN_ERR_REPOS_UNSUPPORTED_VERSION) floats to the top of the client's
error chain, and the generic message is displayed, when it has no
bearing on the problem. This case is more evidence that we should not
display generic error messages when we have specific ones.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org