Another g_markup_escape_text Vulnerability

[Elliott Sales de Andrade]

On Sat, May 5, 2012 at 6:36 PM, Mark Doliner <mark at kingant.net> wrote:

> On Fri, May 4, 2012 at 3:05 PM, Elliott Sales de Andrade
> <qulogic at pidgin.im> wrote:
> > On Fri, May 4, 2012 at 3:53 AM, Mark Doliner <mark at kingant.net> wrote:
> >>
> >> Hey Elliott, from looking at your patch it seems like there might
> >> still be a few places where we don't validate the string as UTF-8.
> >> For example:
> >> - If no content-type is provided
> >
> > I'm actually not sure why that conversion is there, now that I look
> again.
> > If there is no content-type, then `msn_cmdproc_process_msg` (which is
> called
> > on the message immediately after, and leads to the crash) should ignore
> the
> > message entirely.
>
> Good point.  I think you're right.  So it looks like we can ignore this
> case.
>
>
We could probably limit this to (content_type && g_str_equal(content_type,
"text/plain") && msg->body) then.


>  >> - If content-type is not text/plain
> >
> > If it's not text/plain, then it may not necessarily be UTF-8 anyway. It
> > could, for example, be P2P data. They definitely shouldn't be displayed,
> but
> > there might be some other type of parsing going on, but I'd have to check
> > the other content types to be sure.
>
> Oh, I see what's happening.  msn_plain_msg() is only called when
> content-type is text/plain, and so you're only ensuring UTF-8 when
> content-type is text/plain.  And yeah, attempting to convert to UTF-8
> when content-type is not text/plain would break binary data.  So we're
> good here, too.
>
>
I've been looking over the other content-types, and most should be okay.
However, to be safe, it may be a good idea to use
purple_str_has_prefix(content_type, "text/") instead? I'm thinking it may
be better to assume UTF-8 instead of ISO-8859-1 then. (For example,
emoticons are text/x-mms-emoticon with no charset, but you should be able
to use UTF-8 emoticon shortcuts.)


> >> - If msg->charset is NULL and g_convert(from ISO-8859-1 to UTF-8) fails
> >> - If msg->charset is set to something other than UTF-8 and
> >> g_convert(msg->charset to UTF-8) fails and g_convert(ISO-8859-1 to
> >> UTF-8) fails
> >
> > Is it even possible for ISO-8859-1->UTF-8 conversion to fail? I thought
> all
> > bytes are valid ISO-8859-1 and there are no multi-byte characters. I
> tried
> > converting a buffer of 255-0 which did not induce an error.
>
> Oh, good point, I'd forgotten about that.  I went ahead and added a
> check for it, just to be polite to people with broken systems.
>
> I've attached an updated patch.  Changes from yours:
> - re-ordered the if-else statements a little.  I think I removed one
> level of depth from one place.
> - Don't bother trying to convert anything if msg->body if NULL.  I'm
> not sure this is needed... but if seems like it might be.
>

Probably a good idea anyway.


> - I noticed that msn_oim_report_to_user() calls
> msn_message_parse_payload() and it looks like it assumes msg->body is
> UTF-8 without actually validating it or checking that content-type is
> text/plain.  So I added a call to g_utf8_validate().
>
>
That message is created by the server and then wrapped by an XML document.
It really *should* be UTF-8. The msg->body is base64-encoded by the server
and so is definitely UTF-8. The base64-decoded message may not be, but that
check is already there.


> One remaining question I have... are we sure the headers will be valid
> UTF-8?  I think we assume they're UTF-8 in a lot of places.
>
>
I'm not too sure it's guaranteed, but I'm also not sure whether we make
that assumption either. There are strsplits and strcmps, but I don't think
anything should be displayed from the headers (i.e., it's /just/ a
"string"), but don't quote me on that.


> I'll be out of town for three weeks starting Tuesday, so I'm hoping to
> release this tomorrow.
>

I've attached an updated patch with the changes I suggested.

-- 
Elliott aka QuLogic
Pidgin developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20120505/b26601bd/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pidgin-msn-utf8-validation-v3.diff
Type: application/octet-stream
Size: 2241 bytes
Desc: not available
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20120505/b26601bd/attachment-0001.obj>