Re: Svnadmin verify question

2021-11-03 Thread Luke Mauldin
Does the Svnadmin verify do checksum checks?  How does it verify validity?  

> On Nov 3, 2021, at 1:44 PM, Nathan Hartman  wrote:
> 
> On Tue, Nov 2, 2021 at 5:09 PM Luke Mauldin  wrote:
>> 
>> Can anyone tell me the details of the svnadmin verify command?  Does 
>> subversion store any internal hash of each commit to verify its correctness 
>> and that it has not changed over time due to disk/memory errors or software 
>> bugs?
>> 
>> Luke Mauldin
> 
> Yes. The purpose of 'svnadmin verify' is to check for exactly those
> kinds of errors.
> 
> By default it runs through each revision one-by-one and stops/reports
> if it finds a problem. There's also a --keep-going option since SVN
> 1.9.
> 
> It is a good idea to run 'svnadmin verify' periodically on
> repositories to ensure their integrity.
> 
> It is also crucial to have a good backup process in place, i.e., keep
> multiple good up-to-date backups of repositories in different physical
> locations and make sure you can restore from them before disaster
> strikes. Obviously it's of no use to detect corruption if you don't
> have backups to restore from.
> 
> Hope this helps,
> Nathan



Re: Svnadmin verify question

2021-11-03 Thread Nathan Hartman
On Tue, Nov 2, 2021 at 5:09 PM Luke Mauldin  wrote:
>
> Can anyone tell me the details of the svnadmin verify command?  Does 
> subversion store any internal hash of each commit to verify its correctness 
> and that it has not changed over time due to disk/memory errors or software 
> bugs?
>
> Luke Mauldin

Yes. The purpose of 'svnadmin verify' is to check for exactly those
kinds of errors.

By default it runs through each revision one-by-one and stops/reports
if it finds a problem. There's also a --keep-going option since SVN
1.9.

It is a good idea to run 'svnadmin verify' periodically on
repositories to ensure their integrity.

It is also crucial to have a good backup process in place, i.e., keep
multiple good up-to-date backups of repositories in different physical
locations and make sure you can restore from them before disaster
strikes. Obviously it's of no use to detect corruption if you don't
have backups to restore from.

Hope this helps,
Nathan


Re: Use-after-free of object-pools in subversion/libsvn_repos/authz.c

2021-11-03 Thread Thomas Weißschuh
On 2021-11-03 17:14+0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2021 at 04:47:39PM +0100, Thomas Weißschuh wrote:
> > The svn_atomic__init_once() inside svn_repos_authz_initialize() seems to 
> > work
> > correctly. synchronized_authz_initialize() is only executed once.
> > 
> > I am arguing that it should *not* be executed only once because the pools 
> > that
> > it gets called with the first time will be invalid when it is called the 
> > second
> > time. So it should rebuild authz_pool with the new parent pool.
> > 
> > Replacing the call through svn_atomic__init_once() with an unconditional 
> > call
> > makes the issue go away.
> 
> I see. So for some reason, under httpd's control, the SVN module's
> post_config hook is invoked several times, each time with a different
> pool? Do we know why this is happening?
> 
> If this is valid httpd behaviour then the code in authz.c is making
> an invalid assumption. And there could similar problems elsewhere,
> in svn_fs_initialize() for example.

Looking at server/main.c from httpd:
( 
https://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/server/main.c?view=markup&pathrev=1874286
 )

svn_repos_authz_initialize() is called from the post_config hook
(the function called "init" in mod_dav_svn.c) of mod_dav_svn using the pool
passed as the first argument to the hook.

That hook is invoked from httpds src/main.c in lines 740 and 807, both times
with the same pool "pconf" as the first argument.
But in line 750, between those hooks, the function reset_process_pconf() is
invoked.
This clears the pconf pool of the process which is also the same pool that is
passed to the hooks and where mod_dav_svn has instantiated its subpools from.
(See line 490)

I'll open a ticket for that tomorrow.


Re: Use-after-free of object-pools in subversion/libsvn_repos/authz.c

2021-11-03 Thread Stefan Sperling
On Wed, Nov 03, 2021 at 05:21:19PM +0100, Thomas Weißschuh wrote:
> On 2021-11-03 17:14+0100, Stefan Sperling wrote:
> > On Wed, Nov 03, 2021 at 04:47:39PM +0100, Thomas Weißschuh wrote:
> > > The svn_atomic__init_once() inside svn_repos_authz_initialize() seems to 
> > > work
> > > correctly. synchronized_authz_initialize() is only executed once.
> > > 
> > > I am arguing that it should *not* be executed only once because the pools 
> > > that
> > > it gets called with the first time will be invalid when it is called the 
> > > second
> > > time. So it should rebuild authz_pool with the new parent pool.
> > > 
> > > Replacing the call through svn_atomic__init_once() with an unconditional 
> > > call
> > > makes the issue go away.
> > 
> > I see. So for some reason, under httpd's control, the SVN module's
> > post_config hook is invoked several times, each time with a different
> > pool? Do we know why this is happening?
> 
> I'll investigate that a bit more.
> 
> > If this is valid httpd behaviour then the code in authz.c is making
> > an invalid assumption. And there could similar problems elsewhere,
> > in svn_fs_initialize() for example.
> 
> Should we keep this discussion on the ML or should I open a ticket for it?

Both :)

I agree that this warrants a new ticket. Once the problem is fully
understood we can still decide to close this ticket in case the real
issue turns out to be somewhere else. Please feel free to create a
new ticket for this in our tracker. Thanks!


Re: Use-after-free of object-pools in subversion/libsvn_repos/authz.c

2021-11-03 Thread Thomas Weißschuh
On 2021-11-03 17:14+0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2021 at 04:47:39PM +0100, Thomas Weißschuh wrote:
> > The svn_atomic__init_once() inside svn_repos_authz_initialize() seems to 
> > work
> > correctly. synchronized_authz_initialize() is only executed once.
> > 
> > I am arguing that it should *not* be executed only once because the pools 
> > that
> > it gets called with the first time will be invalid when it is called the 
> > second
> > time. So it should rebuild authz_pool with the new parent pool.
> > 
> > Replacing the call through svn_atomic__init_once() with an unconditional 
> > call
> > makes the issue go away.
> 
> I see. So for some reason, under httpd's control, the SVN module's
> post_config hook is invoked several times, each time with a different
> pool? Do we know why this is happening?

I'll investigate that a bit more.

> If this is valid httpd behaviour then the code in authz.c is making
> an invalid assumption. And there could similar problems elsewhere,
> in svn_fs_initialize() for example.

Should we keep this discussion on the ML or should I open a ticket for it?


Re: Use-after-free of object-pools in subversion/libsvn_repos/authz.c

2021-11-03 Thread Stefan Sperling
On Wed, Nov 03, 2021 at 04:47:39PM +0100, Thomas Weißschuh wrote:
> The svn_atomic__init_once() inside svn_repos_authz_initialize() seems to work
> correctly. synchronized_authz_initialize() is only executed once.
> 
> I am arguing that it should *not* be executed only once because the pools that
> it gets called with the first time will be invalid when it is called the 
> second
> time. So it should rebuild authz_pool with the new parent pool.
> 
> Replacing the call through svn_atomic__init_once() with an unconditional call
> makes the issue go away.

I see. So for some reason, under httpd's control, the SVN module's
post_config hook is invoked several times, each time with a different
pool? Do we know why this is happening?

If this is valid httpd behaviour then the code in authz.c is making
an invalid assumption. And there could similar problems elsewhere,
in svn_fs_initialize() for example.


Re: Use-after-free of object-pools in subversion/libsvn_repos/authz.c

2021-11-03 Thread Thomas Weißschuh
On 2021-11-03 16:34+0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2021 at 04:21:34PM +0100, Thomas Weißschuh wrote:
> > Hi everybody,
> > 
> > While investigating persistent segmentation faults in mod_dav_svn I found
> > invalid uses of objectpools in subversion/libsnv_repos/authz.c.
> > 
> > In svn_repos_authz_initialize() the objectpools passed in during the
> > configuration phase are stored in static variables.
> > For some reason the configuration phase runs multiple times and the 
> > previously
> > used objectpools are freed.
> > 
> > Because cached references to these freed objectpools are still used inside
> > authz.c accesses to that memory will read invalid data from other parts of
> > Apache, leading to segmentation faults.
> > 
> > Maybe the issue happens especially on Alpine Linux because they are using 
> > musl
> > libc with its own memory allocator which may behave differently than glibc 
> > and
> > more directly reuse freed memory.
> > 
> > I was able to work around the issue by removing the caching logic in
> > svn_repos_authz_initialize() and always call 
> > synchronized_authz_initialize().
> > 
> > Thanks,
> > Thomas
> 
> I guess this means the implemention of apr_atomic_cas32() is not working
> correctly on your system? svn_atomic__init_once() relies on this.

The svn_atomic__init_once() inside svn_repos_authz_initialize() seems to work
correctly. synchronized_authz_initialize() is only executed once.

I am arguing that it should *not* be executed only once because the pools that
it gets called with the first time will be invalid when it is called the second
time. So it should rebuild authz_pool with the new parent pool.

Replacing the call through svn_atomic__init_once() with an unconditional call
makes the issue go away.

> There are regression tests for atomics in APR.
> You could try to run these tests to see if there are any failures.
> If broken atomics turn out to be the root cause of this problem
> then please file an issue with the APR project instead.

I'll try that, too. But as explained above it does not seem likely to me.

Thanks,
Thomas


Re: Use-after-free of object-pools in subversion/libsvn_repos/authz.c

2021-11-03 Thread Stefan Sperling
On Wed, Nov 03, 2021 at 04:21:34PM +0100, Thomas Weißschuh wrote:
> Hi everybody,
> 
> While investigating persistent segmentation faults in mod_dav_svn I found
> invalid uses of objectpools in subversion/libsnv_repos/authz.c.
> 
> In svn_repos_authz_initialize() the objectpools passed in during the
> configuration phase are stored in static variables.
> For some reason the configuration phase runs multiple times and the previously
> used objectpools are freed.
> 
> Because cached references to these freed objectpools are still used inside
> authz.c accesses to that memory will read invalid data from other parts of
> Apache, leading to segmentation faults.
> 
> Maybe the issue happens especially on Alpine Linux because they are using musl
> libc with its own memory allocator which may behave differently than glibc and
> more directly reuse freed memory.
> 
> I was able to work around the issue by removing the caching logic in
> svn_repos_authz_initialize() and always call synchronized_authz_initialize().
> 
> Thanks,
> Thomas

I guess this means the implemention of apr_atomic_cas32() is not working
correctly on your system? svn_atomic__init_once() relies on this.

There are regression tests for atomics in APR.
You could try to run these tests to see if there are any failures.
If broken atomics turn out to be the root cause of this problem
then please file an issue with the APR project instead.

Hope this helps,
Stefan


Use-after-free of object-pools in subversion/libsvn_repos/authz.c

2021-11-03 Thread Thomas Weißschuh
Hi everybody,

While investigating persistent segmentation faults in mod_dav_svn I found
invalid uses of objectpools in subversion/libsnv_repos/authz.c.

In svn_repos_authz_initialize() the objectpools passed in during the
configuration phase are stored in static variables.
For some reason the configuration phase runs multiple times and the previously
used objectpools are freed.

Because cached references to these freed objectpools are still used inside
authz.c accesses to that memory will read invalid data from other parts of
Apache, leading to segmentation faults.

Maybe the issue happens especially on Alpine Linux because they are using musl
libc with its own memory allocator which may behave differently than glibc and
more directly reuse freed memory.

I was able to work around the issue by removing the caching logic in
svn_repos_authz_initialize() and always call synchronized_authz_initialize().

Thanks,
Thomas


Also reported before at 
https://gitlab.alpinelinux.org/alpine/aports/-/issues/10116


Environment:
  SVN: 1.14.1
  Apache: 2.4.51
  APR: 1.7.0


Reproduction steps:

Save the two files "Dockerfile" and "svn.conf" from below in a directory.
Execute from that directory:

$ docker build -t svn-repro .
$ docker run --rm -ti -p 8080:80 --name svn-repro svn-repro

Execute from another terminal:

$ curl localhost:8080/foo

The running container should have stopped with a segmentation fault.


Reproduction files:

```Dockerfile
FROM alpine:3.14
USER root

RUN apk --no-cache add \
 apache2 apache2-webdav mod_dav_svn subversion

COPY svn.conf /etc/apache2/conf.d/svn.conf
RUN mkdir -p /srv/svn/repositories
RUN echo -e "[/]\n* = r" | tee /srv/svn/acl
RUN svnadmin create /srv/svn/repositories/foo

EXPOSE 80
CMD ["/usr/sbin/httpd", "-X"]
```

```svn.conf
LoadModule dav_svn_module /usr/lib/apache2/mod_dav_svn.so
LoadModule authz_svn_module /usr/lib/apache2/mod_authz_svn.so

LogLevel trace6
CoreDumpDirectory /tmp/
MaxRequestWorkers 1


DAV svn
SVNParentPath /srv/svn/repositories

AuthzSVNAccessFile /srv/svn/acl

```