Remotely-triggerable crash in oscar xstatus code

[Mark Doliner]

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
that.

--Mark