I like this solution; it seems the cleanest way to go. Comments below:
Daniel Rall <dlr@finemaltcoding.com> writes:
> Issues: 1051, 2271
>
> Problem:
>
> o Possibly sensitive server information is currently revealed to
> clients by mod_dav_svn and svnserve (e.g. path to repository).
>
> $ svn log -r3209683
> svn: PROPFIND request failed on '/repos/svn/!svn/vcc/default'
> svn:
> Reference to non-existent revision 3209683 in filesystem
> '/usr/www/repositories/svn/db'
>
> o There is no way to tell from looking at an error object whether it
> contains security-sensitive information.
>
>
> Requirements of any solution:
>
> o Low-level libraries should continue to propogate all contextual
> information available (including security-sensitive info). For
> example, httpd error logs should contain file system paths to the
> repository, as should error messages from file:// operations.
>
> o Secure versions of error messages must still contain relevant
> security-insensitive information (e.g. "Reference to non-existent
> revision 3209683", even though the path is omitted).
>
>
> Solution:
>
> o Add field, "safe_message" to svn_error_t which contains a safe
> version of the error message. Callers -- mod_dav_svn, svnserve,
> etc. -- may choose the secure or insecure error message depending on
> their needs (e.g. talking to the client or the httpd error logs).
>
> o Constructors of error messages are responsible for knowing when they
> are including security-sensitive data, and should provide an alternate
> "safe" version of the message too.
>
> svn_error_t *err = svn_error_createf (apr_err, child,
> "Reference to non-existent revision %d"
> "in filesystem '%s'", rev, path);
> /* ### Use helper function to avoid pool usage mistakes? */
> err->safe_message = apr_psprintf(err->pool,
> "Reference to non-existent revision %d",
> rev);
Yeah, I think we'll want a helper function for this.
> o Helper function to acquire appropriate error message from svn_error_t:
>
> /** Return the least security-sensitive available custom message in @a
> * err.
> */
> char *
> svn_error_safest_message(svn_error_t *err)
> {
> return (err->safe_message ? err->safe_message : err->message);
> }
>
> o A macro which logs the security-insensitive version, and returns the
> secure version (for usage in mod_dav_svn, svnserve, etc.)?
Well, that macro would be very different in svnserve and mod_dav_svn.
Not sure if it's worth having two, we'll have to see when we get to
implementing.
> Questions about solution:
>
> o Do we need svn_error2_t for safe_message (for binary compatibility)?
I would _think_ so. It's a public, open structure. Even if we add to
the end of the structure, it won't be source-compatible with older SVN
releases. It kind of depends how we interpret clause (2) in HACKING:
2) Upgrading to a new minor release in the same major line may
cause new APIs to appear, but not remove any APIs. Any code
written to the old minor number will work with any later minor
number in that line. However, downgrading afterwards may not
work, if new code has been written that takes advantage of the
new APIs.
Here, we're not causing a new API to appear so much as adding a field
to an existing API. But now that I think about it, that's not so
convincing. Maybe it is okay according to the guidelines. I'd like a
second opinion.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Apr 12 22:51:47 2005