security review and patches for libpurple
elb at pidgin.im
Tue Aug 9 12:26:14 EDT 2011
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
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 482 bytes
Desc: Digital signature
More information about the security