Input Validation in Functions

[Ben Reser <be...@reser.org>]

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 <be...@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 <gs...@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.

Re: Input Validation in Functions

[Branko Čibej <br...@wandisco.com>]

On 08.03.2013 09:23, Ben Reser wrote:
> On Thu, Mar 7, 2013 at 11:47 PM, Branko Čibej <br...@wandisco.com> wrote:
>> 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.
> I'd like to agree but the way our APIs are layered and actually used
> is not conducive to this.
>
> Case in point...
>
>> 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).
> Yes the servers are supposed to be using svn_repos APIs.  However,
> they end up needing to use svn_fs APIs because the repos layer
> provides an svn_fs_t and some of the features of the the libsvn_fs
> layer are not provided via the repos layer.  E.G. it's impossible to
> retrieve the text of a file with libsvn_repos.

Yes, I know. :(
I keep thinking we never should have invented a separate svn_repos layer
with wrappers for 80% of svn_fs. But that's crying over spilt milk.

> We could go through and figure out which bits of the various layers
> are used by the servers.  But I'm not sure how much work that would
> actually be.

A lot.

So current candidates are svn_repos and svn_fs; but what about the
varioius svn_subr API groups? Retrofitting parameter checks all over the
place there seems like quite a bit of extra work, too.

-- Brane

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com

Re: Input Validation in Functions

[Ben Reser <be...@reser.org>]

On Thu, Mar 7, 2013 at 11:47 PM, Branko Čibej <br...@wandisco.com> wrote:
> 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.

I'd like to agree but the way our APIs are layered and actually used
is not conducive to this.

Case in point...

> 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).

Yes the servers are supposed to be using svn_repos APIs.  However,
they end up needing to use svn_fs APIs because the repos layer
provides an svn_fs_t and some of the features of the the libsvn_fs
layer are not provided via the repos layer.  E.G. it's impossible to
retrieve the text of a file with libsvn_repos.

We could go through and figure out which bits of the various layers
are used by the servers.  But I'm not sure how much work that would
actually be.

Re: Input Validation in Functions

[Branko Čibej <br...@wandisco.com>]

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 <be...@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 <gs...@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