On Tue, 08 Aug 2017 14:23:02 +1000
Joel Sing <j...@sing.id.au> wrote:
> On Saturday 29 July 2017 20:49:18 Jan Klemkow wrote:
> > Hi Jack,
> >
> > On Fri, Jul 28, 2017 at 02:05:34AM +0930, Jack Burton wrote:
> > > On Thu, 27 Jul 2017 13:10:14 +0200
> > >
> > > > But, I found a bug in the part of the FastCGI variables. The
> > > > following condition is always false.
> > > >
> > > > > Index: usr.sbin/httpd/server_fcgi.c
> > > > > ===================================================================
> > > > > RCS file: /cvs/src/usr.sbin/httpd/server_fcgi.c,v
> > > > > retrieving revision 1.74
> > > > > diff -u -p -r1.74 server_fcgi.c
> > > > > --- usr.sbin/httpd/server_fcgi.c 21 Jan 2017 11:32:04
> > > > > -0000 1.74 +++ usr.sbin/httpd/server_fcgi.c 21
> > > > > Jul 2017 08:25:57 -0000 @@ -282,11 +283,57 @@
> > > > > server_fcgi(struct httpd *env, struct cl
> > > >
> > > > ...
> > > >
> > > > > + if (srv_conf->tls_ca != NULL) {
> > > >
> > > > ...
> > >
> > > That's odd -- I'm not seeing that behaviour here -- in my tests
> > > srv_conf->tls_ca always behaved just as expected (it's NULL iff
> > > the "tls client ca" directive is not given for that server).
> >
> > In the End, I found and fixed the real bug here:
> >
> > @@ -430,7 +438,11 @@ config_getserver_config(struct httpd *en
> > }
> >
> > f = SRVFLAG_TLS;
> > - srv_conf->flags |= parent->flags & f;
> > + if ((srv_conf->flags & f) == 0) {
> > + srv_conf->flags |= parent->flags & f;
> > + srv_conf->tls_ca = parent->tls_ca;
> > + srv_conf->tls_crl = parent->tls_crl;
>
> I'd have to double check, however I'm pretty sure that this will
> result in a double-free since you're copying the pointer (without a
> reference count) across server config structs. Both will likely be
> passed to serverconfig_free(), which means free() will then be passed
> the same pointer twice.
You're absolutely right. That happens when purging config on reload,
if the old config for a server doing tls client verify has two or more
location blocks...
...and my 30 July diff suffers from the same problem.
Didn't show up in my testing, as initially I used no location blocks
and after Jan's problem report only used one location block per
server ... and server_purge() already makes sure it doesn't free the
default location twice.
I don't think it makes much sense to copy the contents of tls_ca &
tls_crl for each location, since they'll always be identical within
their common parent server.
So that leaves as obvious alternatives either reference counting (as you
alluded to above) or dereferencing only via the parent struct (as Jan's
earlier diff did).
I'm not a fan of the latter, as it's likely to cause confusion
later on (won't be obvious why we're not using the seemingly identical
member of the struct we're handed) ... and the former probably adds
more complexity than we really need.
Perhaps a better alternative would be to free the CA & CRL mem at the
end of server_tls_init() -- like we do already for the key pair -- then
add flags for CA & CRL use instead of just checking for null pointers
everywhere (as both my & Jan's diffs have been doing so far).
It seems I have a couple of other changes to make too, so will roll
something like that in with them & post a revised diff later today or
tomorrow.
Thanks for spotting this Joel.
