This discussion started out on private@ when discussing how far we
should go in fixing the security issue. With Greg's permission I'm
moving the discussion here:
I'd posted the patch which is attached to this email with some
thoughts about implementing input validation. And ended with the
following statement:
On Wed, Mar 6, 2013 at 10:22 PM, Ben Reser <ben_at_reser.org> wrote:
>> This is actually a pretty common pattern for us to not bother to check
>> incoming arguments, so I'm not sure how the project feels about this
>> in general.
Greg responded as such:
On Wed, Mar 6, 2013 at 7:51 PM, Greg Stein <gstein_at_gmail.com> wrote:
> This is my concern. We said long ago, "if the caller screws up, then
> that is there problem. the function is perfectly entitled to core dump
> on malformed params."
Couple of thoughts on this:
1) In this case the caller is us. If we can't get this right all the
time, is it really reasonable to expect others to do so?
2) I don't think there is any case where we'd consider dumping core is
appropriate on the server side. Client side I think is a tad
different. But in my opinion we should harden the APIs that are
needed on the server side as much as is reasonable.
3) I think it's safe to say that the last 15 years has proven out that
this just isn't a good coding practice. If you look at secure coding
guidelines you will always find that functions should validate their
arguments. E.G.
https://www.securecoding.cert.org/confluence/display/seccode/API00-C.+Functions+should+validate+their+parameters
I tend to think there's some middle ground here. Obviously it's
impossible to check everything. You'll note that my patch only
checked things that the function was directly dereferencing.
If we'd had this sort of simple checking in place we'd be talking
about a bug fix to our code and not a security issue. In fact I'd
guess we probably wouldn't be talking about this at all unless someone
asked about the error message in their logs.
> My feeling is that we need to fix the problems in mod_dav_svn rather
> than attempt to paper over them at the lower level.
>
> Your interim solution to deny a PROPFIND on an activity should be just
> fine. We should never need to do that. The activity is just a handle
> on the server-side fs-txn.
Just to be clear I'm 100% in agreement that Philip's patch is correct.
I just think we should go the extra mile. We're going to look really
silly if there are similar security issues with different
URLs/methods. We don't really know how the person found the issue at
hand. They may very well be continuing to look for similar issues and
may find some other URL we aren't properly handling. Issues we could
solve now.
An alternative approach would be sitting down and go through all the
access patterns and checking them. However, I'd expect that to take
time that nobody probably really has right now. On top of that, this
process needs to be repeated whenever we make changes to mod_dav_svn
to make sure things haven't changed.
Quite frankly as a project I don't think we have the bandwidth to do that.
Received on 2013-03-08 00:23:39 CET