security review and patches for libpurple

[Ethan Blanton]

Mark Doliner spake unto us the following wisdom:
> On Sun, Jul 17, 2011 at 6:58 PM, Ethan Blanton <elb at pidgin.im> wrote:
> > With that in mind, I'd like to ask again if there are any objections
> > to my committing these patches to ipp without embargo or a coordinated
> > release.  If not, I will land them some time tomorrow.  If anyone even
> > simply thinks we should wait a few days or get additional input before
> > landing them, that's fine, too.
> 
> I have no objections to this plan.  Embargoing stuff is a pain, so it
> makes sense to only embargo things that actually need to be embargoed.
>  Changes that we believe have no security implications should totally
> be committed whenever.

Unfortunately, I haven't had time to get back to this, so fortunately
the fact that nobody bothered to weigh in wasn't holding anything
back.  Thank you for taking the time.  :-)  Obviously "landing
tomorrow" didn't happen, partly because I got no feedback, but partly
because real life stepped in.

> That being said, I'm wondering how people feel about these changes?  I
> feel like the EFF guys keep referring to strlcpy as "standard" and
> "secure" and that they think of strcpy as deprecated and insecure.

Yes.  I am under the impression that many of the changes were not
really something they understood, their automated checker simply told
them to make them, and they did.  I have changed almost all of their
patches to *actually* improve things, instead of just change things.
They had stuff like this:

    strlcpy(dest, src, strlen(src));

... which is clearly nonsensical, and no better than an unchecked
strcpy.  (I don't know if they had precisely that, but they had
semantically similar changes.)

> But I don't know, I'm not feeling it.  Maybe I just haven't thought
> about it enough.  I've only looked at maybe a third of their changes,
> but do any of them actually deal with untrusted data?  I feel like
> most of them deal with user-supplied data or stuff from the local
> host.  Aka stuff that isn't remotely exploitable.
>
> I feel like using strlcpy will hide problems that we would otherwise
> fix.  Rather than crash with a helpful backtrace,  strings will
> silently be truncated unless we check and verify the return value from
> strlcpy.  That's certainly feasible... but if we're going to go to all
> that work, couldn't we just check the code to make sure our buffers
> are big enough?

In places, yeah.  I think you'll find my changes less offensive than
their original patches, but certainly there are still checks which are
not really *right*, but will simply prevent crashes.  As to whether
overrunning the string is better ... I can't say.  Overrunning buffers
doesn't always lead to crashing, sometimes it leads to much more
subtle and difficult-to-identify bugs.  When stacks get smashed, even
if there's a crash, it may not be easy to find from the backtrace.

> I don't doubt that there are places where it makes sense to use
> strlcpy, but I worry that it will make our code more complicated and
> therefore more error prone.

I have tried to avoid this.  Where possible, I am using lengths which
we already know, sizeof(), and actual allocation values for bounds.

I'll push my changes here before long, and we can take a collective
look at them.  If there's agreement that some of them are bad, or
could be fixed in a different, better way, or simply shouldn't be
touched, we can always revert.

I haven't had time to address all of their patches, but I've processed
about 2/3 of them and either applied or marked them for further
inspection.

Ethan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: Digital signature
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20110809/18a9893c/attachment.pgp>