On 08.03.2013 00:23, Ben Reser wrote:
> 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.
Tend to agree but I'd restrict such checking to the APIs we consider
"public" -- regardless of whether or not they're exposed in the public
headers or not. Doing such checks in every layer is definitely overkill.
Furthermore, while your patch proposes checks on the FS vtable level, I
believe servers are supposed to use the svn_repos APIs and it would
therefore make sense to make those bullet-proof (svn_fs should only be
used directly by the admin utilities).
-- Brane
P.S.: Our API in general exposes too many knobs ... but it's rather late
in the day to change that.
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Received on 2013-03-08 08:48:33 CET