Remotely-triggerable crash in oscar xstatus code
mark at kingant.net
Wed Jul 14 03:46:18 EDT 2010
Great, thanks guys! I'll email packagers in a few minutes. See two
notes inline below.
On Tue, Jul 13, 2010 at 11:08 PM, Paul Aurich <paul at darkrain42.org> wrote:
> On 2010-06-18 18:18, Mark Doliner wrote:
>> Problem #1 (the remotely-triggerable crash):
>> The crash happens when a buddy sets an xstatus message containing <desc>
>> but no closing </desc>, or <title> but no closing </title>. The fix
>> is to check the result of strstr(closing_tag_name) and do nothing if it
>> is NULL.
> Yep. Jan Kaluza (HanzZ) has a backtrace of this happening in the wild
> (dump of the bytestream at the bottom of this email). Actually, I have
> a question about the bytestream there:
> It looks like there are two xstatus updates back-to-back. I can't
> recognize SNAC-framing-in-octal, so I can't tell if that's two SNAC
> packets smooshed together or just two xstatus updates, but if it's the
> latter, the code doesn't handle it (and if it's the former, I think
> that's a bug somewhere else in the prpl)
Yeah I don't know what's up with that dump. The two xstatus strings
look mostly the same, right, but the first one is cut off? I wonder
if that variable just isn't null terminated or something? Or maybe
it's already been freed, and that memory just happens to contain the
xstatus message twice for some reason?
>> Problem #2:
>> Fixes potential incorrect parsing of the xstatus string that could result
>> in an incorrect message being displayed to the libpurple user. Happens if
>> an xstatus message contains </desc> before <desc>, or </title> before
>> <title>. The fix is to start looking for the closing tag at the end
>> of the beginning tag rather than at the beginning of the xstatus xml.
>> Probably not a security problem, but definitely a bug.
> Yeah, I noticed that while following the logic looking at HanzZ's crash
> (I really should have read this email a month ago! I'm sorry!) and your
> patch seems to fix it almost correctly.
> I think the two instances of "tmp1 += 12" and "tmp1 += 13" are backwards
> ("title" is one character more than "desc"), although I'd personally
> prefer strlen("stuff") just for readability.
Yikes, you're totally right. I think when I swapped the order of
title and desc I neglected to swap the +=12 and the +=13. I'll fix
More information about the security