Date: Fri, 4 Jun 2004 10:58:51 +0200 (CEST)
From: Sebastian Krahmer <krahmer@suse.de>
Reply-To: security-team@suse.de
To: security-team@suse.de
Subject: [security-team] gaim


Ich glaub ich hab nen remote stack smash im MSN parsing:


process_multi_line(MsnServConn *servconn, char *buffer)
{
        char msg_str[MSN_BUF_LEN];
                     ^^^^^^^^^^^
        gboolean result = TRUE;

        if (servconn->multiline_type == MSN_MULTILINE_MSG) {
                MsnMessage *msg;
                size_t header_len;
                g_snprintf(msg_str, sizeof(msg_str),
                                   "MSG %s %s %d\r\n",
                                  servconn->msg_passport,servconn->msg_friendly,
                                   servconn->multiline_len);

                header_len = strlen(msg_str);
                ^^^^^^^^^^

                // AUD?
                memcpy(msg_str + header_len, buffer, servconn->multiline_len);
                               ^^^^^^^^^^^^          ^^^^^^^^^^^^^^^^^^

Selbst wenn multiline_len maximal MSN_BUF_LEN wäre, würde es hier knallen.
Man kann sogar glaube ich die multiline_len per remote request setzen,
und MSN_BUF_LEN byte koennen in msn_servconn_parse_data()
eingelesen werden und kommen in die servconn->rxqueu, die
dann an als buffer argument an process_multi_line() geht.

Müssten jetzt die gaim developer verifizieren, allerdings gibts viele
die sagen 'kein bug' wenn man ihnen versucht bugs in ihrem code klarzumachen :)

CAN-2004-0500

I created a patch for the new security issues the SuSE Security Team found.
Could you please peer-review it and look whether it builds clean,
my machine lacks proper gnome libs.
I will attach patch soon.

Created attachment 21723 [details]
fix for overflows

...

I'll built it for 0.75, the version we have on 9.1, and test it.

Could you have a look then whether the patch applies to older
versions than 9.1? For stable I can have a look whether its still
affected.

So we have 9.0 and 9.1 now, right? Does it apply to 8.2 and 8.1?

9.1, 9.0 -> we've got 'em. 
 
still need to take a look for 8.2,8.1 
 

Yes, could you please do that. The timeline has been extended for
public disclosure, but this will not be possible a second time
I guess :-)

CAN-2004-0500

Created attachment 22258 [details]
the fix for STABLE

...

Ok. I looked at SL 9.0. Looks like the vulnerable code was
introduced after gaim 0.67 which we have on SL 9.0.
So we only need updates for SL 9.1 and STABLE (and derived
manitained products if we have some). The fixes are already attached.
Please go ahead.

I am on it. Did the patches made it upstream to the gaim team? 

At least we informed them and send the patches. I dont know
what they do with it.

/work/src/done/PATCHINFO/gaim.patchinfo-box: 
 
DISTRIBUTION: 9.1-i386,9.1-x86_64 
PACKAGE: gaim 
PACKAGER: hhetter@suse.de 
BUGZILLA: 41630 
CATEGORY: security 
DESCRIPTION: 
This update fixes a security problem with the MSN module in gaim. 
( CAN-2004-0500 ) 
DESCRIPTION_DE: 
Dieses Update behebt eine Sicherheitslücke im MSN Modul von Gaim. 
( CAN-2004-0500 ) 
 
Submitted package for 9.1 
stable to follow 

hmm, it doesn't seem that their latest release 0.80 has these patches :(   at 
least by quick looking in their changelogs... 

Hmm, I don't like the last hunk of the secfix.
1) it might copy too much which may lead to a segfault (i.e. it should be
the other way round:
    left = servconn->multiline_len;
    if (left > sizeof(msg_str) - header_len)
        left = sizeof(msg_str) - header_len;
2) the MSG header still contains the untruncated length, so that
msn_message_new_from_str() will access out of bounds data.

I challenge 1). my fix looks if theres more space left than
the input, and if so, it takes the length of the input so it does
not copy more than necessary from input (which could be the case if
there are 100 bytes left but input is only 10byte long).
Indeed, your proposal in 1) has the same logic than my fix, so
we dont need to change that.
For 2) I have no idea, maybe the internal length in the header has to be updated?

Sorry 'bout 1), you're eight, of course. I don't know what I was thinking
yesterday. Anyway, 2) still stands. Maybe we should just return an error instead
of truncating the buffer?

Would be fine with me but depends on the action gaim takes then.
If it exits upon this error than this is also bad because then you
can shutdown it remotely. Maybe we can make it just ignore the MSG,
but i am not so deep in the code.

Ok, any news here? I'd appreciate a patch for the second thing you mentioned.
Then the packages can be build as tehres only SL 9.1 and STABLE
(maybe maintained products based on it). The deadline os Aug 4th so
we have to start.

As the new version 0.80 of gaim doesn't seem to contain any fix for this, this 
fix will be needed for NLD too. 

gaim 0.80 doesn't appear to contain this function.  It appears to have been
completely replaced.

I will have a look for 0.80 tomorrow. Where are the SL 9.1 (and STABLE?)
builds? Deadline is wednesday, we need at least the SL 9.1 patches
as SL 9.1 is the only one affected so far (not taking 0.80 which i look at tomorrow)

for the 9.1 builts, I was awaiting a new patch for 2) in comment #19 

otherwise, the builts I checked in two weeks ago would have been correct 
 

FYI, these packages (see comment #15) can still be found in 
/work/src/done/9.1/gaim 
with gaim.note marking it as "bad fix". 

I think the fix is correct, for comment #19 : the message is constructed
afterwards so there is no bad length somewhere. The message is constructed
from the string where we took a look whether it all fits inside.
I'd vote for removing the "bad fix" mark and releasing the packages.

Could you please inform suse-dist then? I think the packages and patchinfos
should still be in place. Tomorrow is release date. Fortunally
we dont need to test the packages as it is 9.1 only.

The packages are still missing. :( 
 
Release time is 14:00 UTC/16:00 MEST 

gaim is not part of SLES9, right? 

I dont know. I submitted fix for SL 9.1 and STABLE, so if it
is part of SLES9 the fix should apply. I didnt have the time to look at 0.80
which seems to be in NLD. But we should build the SL 9.1 nevertheless
I think as its not clear whether 0.80 is affected.

The problem still is: 
 
thomas@bragg:~> cat /work/src/done/9.1/gaim.note 
bad fix 
 
Sebastian, Michael, isn't that issue solved now by the discussion above? 

For me it is, see comment #28

I don't understand #28. process_multi_line() constructs a message in msg_str. If
the message doesn't fit in the buffer, it gets truncated, but the g_snprintf
call still used the old length servconn->multiline_len. It then calls
msn_message_new_from_str() which uses atoi() to extract the size from the
header (which will be the untruncated size) and then happily tries to copy the
buffer contents. Why do you think this is safe?

->multiline_len is somewhere negotiated with the peer,
so just truncating it brings other problems. g_snprintf() itself
doesnt overflow, as it has a size argument, msn_message_new_from_str()
indeed extracts the (untruncated) size field, but allocates
multiline_len (e.g. enough) bytes via g_malloc(). So it should not overflow.

I see your point, but I doubt truncating ->multiline_len will solve
more problems than it makes plus I dont know to which size it should
be truncated. Still voting for applying this fix.

The point is that it reads unallocated memory which may lead to a segfault.
But if that's ok with the secteam I'll check it in. (I didn't suggest
that multiline_len should be truncated but that the snprintf should use the
right value. Just doing the snprintf/strlen again should be enough.)

hm, can you add that quickly to the fix or should I attach an additional one?

Hm. Thing is not that easy. problem is that we construct the string first
and then we can decide how much room is left. So we do a first g-snprintf(),
see how much is left, (using fixed 5-digit len parameter) and then do the
g_snprintf() again with the actually right size parameter.
This is my proposal:

--- servconn.c.orig     2003-12-26 17:37:33.000000000 -0800
+++ servconn.c  2004-08-03 00:22:34.000000000 -0700
@@ -146,16 +146,24 @@
 
        if (servconn->multiline_type == MSN_MULTILINE_MSG) {
                MsnMessage *msg;
-               size_t header_len;
+               size_t header_len, left;
 
                g_snprintf(msg_str, sizeof(msg_str),
-                                  "MSG %s %s %d\r\n",
+                                  "MSG %s %s %05d\r\n",
                                   servconn->msg_passport, servconn->msg_friendl
y,
                                   servconn->multiline_len);
 
                header_len = strlen(msg_str);
+               left = sizeof(msg_str) - header_len;
+               if (left > servconn->multiline_len)
+                       left = servconn->multiline_len;
 
-               memcpy(msg_str + header_len, buffer, servconn->multiline_len);
+               g_snprintf(msg_str, sizeof(msg_str),
+                                  "MSG %s %s %05d\r\n",
+                                  servconn->msg_passport, servconn->msg_friendl
y,
+                                  left);
+
+               memcpy(msg_str + header_len, buffer, left);
 
                gaim_debug(GAIM_DEBUG_MISC, "msn",
                                   "Message: {%s}\n", buffer);
@@ -654,3 +662,4 @@
                }
        }
 }

If you agree then this can be substituted in the diff-files.

You don't need the %05d as the resulting integer will only get smaller in size.
And I would only do the second snprintf if left is not multiline_len. (You'll
need a second header_len = strlen(msg_str); in that case.)

But by using %05d its always the same length so we dont need any correction
afterwards. Thats ok , no?
Bitte nicht sinnlos an meinen fixes quengeln :-)

Created attachment 22553 [details]
new patch reflecting comment from Michael

If this fix is ok (it also handles the size-parameter in the header)
please apply it and inform suse-dist. The patchinfo should be the same.
STABLE does not need a new fix, since this only affects SL 9.1

Hey, that's not "sinnlos" maybe the other side treats leading zeros as octal,
so %05d might be dangerous... ;-)

Heh, not really? :-)
Then they would use a sscanf() with %o and then its their own fault, no?
If they extract by hand then a atoi() should give them the correct
length. We cant workaround every possible 3rd vendor client brokeness.
The specification should say which encoding the size field is
(hex, dec, oct) so it should be clear.
Submitting? :-)

I don't like that it behaves differently than before. Please fix it.

Created attachment 22593 [details]
new patch also removing %05d

Behaves exactly as Michaels requirements for the gaim-fix A1 orange book
certification procedure.

Ok, should all be finer than fine now, can it now be submitted???

Thanks a lot, and sorry for the hassle! Please submit the package.

Holger, can you please remove the "bad-fix" and submit it with the recent fix
and inform suse-dist? Patchinfos should be the same thanks.

Hm, holger is away until monday.

While looking for gaim 0.80 as requested (SLD) I found another issue.
Please wait with submitting until news.

Created attachment 22613 [details]
the fix for SLD (gaim-0.80)

..

Created attachment 22614 [details]
The fix for SL 9.1 (gaim-0.75)

...

We should now be complete. We have SL 9.1 and SLD affected.
The poatchinfo for Sl 9.1 should be the same. Can you now do the same procedure
for SLD please Holger?

Submitted for 9.1, I can't delete gaim.note myself, but the dist team is 
informed. Now looking at the SLD package. 

submitted package for SLD 
 

submitted package for stable 
 
packages for 9.1 and SLD have already been checked in by the dist team. 
 
closing this bug. 

<!-- SBZ_reopen -->Reopened by krahmer@suse.de at Mon Aug  9 15:26:25 2004, took initial reporter draht@suse.de to cc

reopened for tracking until advisory is released.

CAN-2004-0500

packages approved, advisory released. 

CVE-2004-0500: CVSS v2 Base Score: 7.5 (AV:N/AC:L/Au:N/C:P/I:P/A:P)