Re: [pptp-devel] patch: fix call structure initialization

[James C. <qu...@la...>]

+CC C. Scott Ananian, original author.

On Wed, Jan 21, 2015 at 05:45:53AM -0500, Jaroslav Skarvada wrote:
> Hi James,
> 
> thanks for your analysis.
> 
> ----- Original Message -----
> > On Tue, Jan 20, 2015 at 10:22:50AM -0500, Jaroslav Skarvada wrote:
> > > I received the following crash report [1]. I think the problem
> > > is in improper initialization of the call structure,
> > > i.e. there can be random number in the call vector size.
> > 
> > Thanks.  But I'm not so sure.
> > 
> Well, I was too fast with patching :)
> 
> > Yes, you have evidence of random number in vector struct member size.
> > Your patch only affects the malloc of a PPTP_CALL, not the malloc of a
> > vector.  The malloc of a vector is in vector_create.  size is
> > explicitly initialised.  size is a count.
> > 
> > I've looked at the backtrace:
> > https://bugzilla.redhat.com/attachment.cgi?id=981451
> > 
> > In binary_search the local variable r is corrupt, the value is not
> > reasonable as a count, and this will have come from conn->call.
> > 
> > conn->call is set to return value of vector_create().
> > 
> > conn->call is not changed once set.
> > 
> > Either conn->call or vector->size have been corrupted.
> > 
> All agree.

A possible cause is identified.

Running vector_test under valgrind showed heap corruption in
vector_remove, for which I've pushed a patch to git.  Now, valgrind
reports no problems with vector_test.

Please include the patch in your packaging.  It is attached.

> > Both conn and call at pptp_call_destroy are addresses in [heap]
> > according to the maps:
> > https://bugzilla.redhat.com/attachment.cgi?id=981458
> > 
> > You have evidence of heap corruption, cause unknown.
> >
> > Suggest adding code to detect corrupt size, dump the vector, the call,
> > and the connection.
> > 
> All suggestions are welcome. No problem to add anything to debug
> the problem in Fedora testing branch.

I've no suggestion for this now.

> > Suggest looking for correlating evidence of remote attack.
> > 
> Maybe, but it seems to be quite common for undetected attack:
> https://retrace.fedoraproject.org/faf/reports/4805/
> 
> Fedora 20: 74 reports
> Fedora 19: 47 reports
> Fedora 18: 40 reports

No, I would call that not common enough.  Where heap corruption is
certain, the problem should be reported far more.

Looking at the uses of vector_remove in pptpctrl.c and pptp_callmgr.c,
the heap corruption is unlikely to occur unless;

1.  a memory allocation fails causing a retry, or

2.  a connection fails causing a retry.

So this probably explains the low frequency of problem.

> > > The attached patch should fix the problem
> > 
> > Given above, nak.  But thanks for looking into it.
> > 
> > I'm curious; does your patch actually fix the problem?
> > 
> I don't know, but probably not. I wasn't able to reproduce the problem,
> reopened the bug report

If you'd like to reproduce, you might try setting up for a malloc fail
or connection fail.

-- 
James Cameron
http://quozl.linux.org.au/