Include libmsn in main

MIR looks good, thanks, a couple of points worry me though.

= Security =

# Does not directly process binary or structured data such as video, sound, or pdf
this directly contradicts:
# Would have network activity inasmuch as it handles network traffic for MSN chats, which includes receiving incoming files over chat.

I think this is a typical security sensitive lib, exposed to network data, with buffers, string parsing, marshalling / unmarshalling of network data into objects etc.

This risk is probably largely alleviated by the fact that it should communicate mostly with MSN servers, but msn/p2p.cpp let's me think there are also user to user connections.

I propose that we ask at least for a quick look from a security person; perhaps we can also enable some stronger hardening flags for this particular package?

= IP =

I don't think the MSN protocol is an open standard; I understand it was reverse engineered. I guess this is ok for interoperability, but deserves a mention in the MIR.

I also wonder about usage of the name libmsn; gaim at to be renamed because of TM issues. I guess this is an upstream problem and we will rename if we get asked to.

Would someone from ubuntu-security be so kind to take a quick look at libmsn and see if any obvious bad practice is in use? It looks like decent C++ code to me, but then it has a XML parser and talks to the network! ;-)

Any room for improvement in terms of e.g. build flags?

Any update? It would be nice to have this done sooner than later.
I've also updated the MIR, if that was an issue impeding progress.

Jonathan,

no, there's a pending request to ubuntu-security@ to review this. Sorry for the delay, Christmas/New Year holidays..

Security team, please set to "in progress" if ack, or ask for changes if nack. Thanks!

Sorry for the delay. Will get to this soon.

I'm running intrepid and had libmsn included in the kubuntu-expermintal 4.2 beta. I just updated to the RC and it's gone now. That's probably related to this issue. Just wanted to let you know that intrepid will need this package as well.

Nope, kdenetwork got backported to the ppa without somebody re-adding msn support.

I've moved this to main for now, I'm afraid we can't wait indefinitely for reviews.

I (finally) finished this up today. I spot checked various static buffers and functions that could have potential problems and overall things seem good. Some areas of note:

1. unsafe use of wcscpy() and strcpy():

msn/xmlParser.cpp has:
#ifdef _XMLWIDECHAR
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)wcscpy(c1,c2); }
        ...
#else
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)strcpy(c1,c2); }
        ...
#endif

This is potentially dangerous if the input is not sanitized. A better, but untested, implementation might be something like:
#ifdef _XMLWIDECHAR
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
           XMLSTR *dest = (XMLSTR)wcsncpy(c1,c2,sizeof(c1));
           dest[sizeof(dest)-1] = L'\0';
           return dest;
        }
        ...
#else
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
            XMLSTR *dest = (XMLSTR)strncpy(c1,c2,sizeof(c1));
            dest[sizeof(dest)-1] = '\0';
            return dest;
        }
        ...
#endif

2. Use of sprintf with a static buffer and no bounds checking:

At line 356 of msn/xmlParser.cpp in XMLNode::openFileHelper():
        sprintf(message,
#ifdef _XMLWIDECHAR
            "XML Parsing error inside file '%S'.\n%S\nAt line %i, column %i.\n%s%S%s"
#else
            "XML Parsing error inside file '%s'.\n%s\nAt line %i, column %i.\n%s%s%s"
#endif
            ,filename,XMLNode::getError(pResults.error),pResults.nLine,pResults.nColumn,s1,s2,s3);

where 'filename' is an unsanitized parameter to XMLNode::openFileHelper(). The good news is that openFileHelper() doesn't appear to be used within libmsn. It would be best to fix this to future-proof its use.

3. Does not check the return value of fread()

In XMLNode::parseFile() from xmlParser.cpp. A short read would result in buf containing uncleared memory. This function appears to be only used in XMLNode::openFileHelper(), which, as mentioned, doesn't appear to be used by libmsn. As before, it would be best to check the return value for possible future use of XMLNode::openFileHelper().

4. Contains an embedded md5 implementation. Noteworthy, but not a security concern.

CONCLUSION: Please fix the use of strcpy() and wcscpy() as discussed in point 2. The XML file parsing could be skipped, but it would be nice if it were fixed also.

sorry, "as discussed in point 2" should have read "as discussed in point 1".

Thanks, Jamie! Jonathan, can you please fix those and give it some testing?

I'm really not all that great with C++
I can apply patches just fine, but otherwise I'm not much for it. (Plus I don't have MSN)
Bouncing to Riddell, who should be able to do/delegate this sort of thing.

Hi,
the xmlParser code is not originally part of libmsn project, but it is good to know that only a few issues were spotted.

to use strncpy() we need to know exactly the length of the string, and using for example strlen() (as sizeof() will return the size of the pointer in this case) it will behave exactly the same way. IIRC strcpy() will copy the string until it finds a '\0', which is the same as using strncpy() and a strlen() to find out the size of the string.
am I missing something?

Anyway thanks for the report.

The use of strncpy and knowing the length of the string is the whole point of the change. strlen is dangerous if the string is not null-terminated and strcpy will happily overflow the target buffer if the source string is too long. The example code I gave is not intended to be a patch, but a starting point for correcting the problem.

Here is a patch which changes the prototype of _tcscpy() to accept a length parameter and adjust the code accordingly.

On Fri, Mar 20, 2009 at 02:09:33PM -0000, Aurélien Gâteau wrote:
> http://launchpadlibrarian.net/24149888/02-avoid_potential_buffer_overrun.diff

"n" should be verified to be !=0, so that result[-1] isn't used.

See the notes from "man strcpy":

       If there is no terminating null byte in the first n characters of src,
       strncpy() produces an unterminated string in dest. Programmers often
       prevent this mistake by forcing termination as follows:

           strncpy(buf, str, n);
           if (n > 0)
               buf[n - 1]= '\0';

This change does not have the same functionality (in toXMLStringUnSafe):

- case 4: *(dest++)=*(source++);
- case 3: *(dest++)=*(source++);
- case 2: *(dest++)=*(source++);
- case 1: *(dest++)=*(source++);
+ case 4:
+ case 3:
+ case 2:
+ case 1:
+ *(dest++)=*(source++);
+ length--;
+ break;

the original can copy up to 4 characters (notice the lack of breaks).

Also, nothing in the loop is testing that "length" is remaining positive.
This should be checked in toXMLStringUnSafe, CreateXMLStringR (if nResult
is ever >= length, abort), and in _tcscpy (where it should abort if n<0).
(Note that strncpy takes size_t, not int, so a signed to unsigned
conversion would take place -- best to check the int before getting to
the strncpy call.)

--
Kees Cook
Ubuntu Security Team

Kees Cook wrote:
> On Fri, Mar 20, 2009 at 02:09:33PM -0000, Aurélien Gâteau wrote:
>> http://launchpadlibrarian.net/24149888/02-avoid_potential_buffer_overrun.diff
>
> "n" should be verified to be !=0, so that result[-1] isn't used.
[snip]

> This change does not have the same functionality (in toXMLStringUnSafe):
[snip]
> the original can copy up to 4 characters (notice the lack of breaks).
>
> Also, nothing in the loop is testing that "length" is remaining positive.
> This should be checked in toXMLStringUnSafe, CreateXMLStringR (if nResult
> is ever >= length, abort), and in _tcscpy (where it should abort if n<0).
[snip]

I agree with these remarks (shame on me for the switch() error), I went
to fix those bugs, but after further inspection, I believe the changes
are not necessary or at least not at the right place:

_tcspy() is called from CreateXMLStringR() and toXMLStringUnSafe() only.
- CreateXMLStringR() can be called with a null pointer, in this case it
compute the necessary buffer size. This is what CreateXMLString(), the
only caller, does.

- toXMLStringUnSafe() is called from CreateXMLStringR(), which is fine
and from toXML(), which compute the source string length before.

All in all I think the code is fine, or that the fix should be on the
malloc()/realloc() calls which are done after computing the buffer lengths.

Aurélien, can you clarify your position? Are you saying no patch is needed at all or that your patch is fine as is? The main point Kees was making was to make sure that we don't have a negative length, where you put that check doesn't really matter so long as it ensures that the length is > 0.

My patch is not fine (because of the switch case error I introduced). What I meant is that _tcscpy() is only dangerous if either the source buffer is not 0 terminated, or the source buffer is 0 terminated but the destination buffer is too small.

After further inspection of the original code, I believe it only runs _tcscpy() on 0 terminated source buffers, and correctly takes care of allocating destination buffers big enough so that they cannot be overrun.

That's why I believe the original code is ok and does not need to be patched.

More on this. I wrote a simple test program to exercise the parser. I am able to make toXMLStringUnSafe read past the source buffer end, but strangely valgrind does not complain.

I also started to test the whole parser with strange strings. So far I can't get it to crash or valgrind to complain. I will continue a bit further on this. I attach my test program to this message, place it inside the msn/ dir and run "make test" to build it.

This bug was fixed in the package libmsn - 4.0~beta4-0ubuntu3

---------------
libmsn (4.0~beta4-0ubuntu3) jaunty; urgency=low

  * Add 02-avoid_potential_buffer_overrun.diff from Aurélien Gâteau
    Closes LP: #308060

 -- Jonathan Riddell <email address hidden> Fri, 27 Mar 2009 12:38:50 +0000

so, can I add this to the upstream libmsn?
http://launchpadlibrarian.net/24149888/02-avoid_potential_buffer_overrun.diff

thanks!

Please revert! As pointed out by Kees Cook, the patch is not correct.

As I said, I believe the patch is not needed, but if you are to include this, then at least fix the switch() bug I introduced.

... or use this updated patch.

Thanks Aurélien! I adjusted the switch statement to check length on each case (so we couldn't overflow in the middle of the cascade through the statements).

I also added patch tagging to the header, for Ubuntu's own tracking. If this passes your tests, then, yes, it should go upstream.

This bug was fixed in the package libmsn - 4.0~beta4-0ubuntu4

---------------
libmsn (4.0~beta4-0ubuntu4) jaunty; urgency=low

  * Update 02-avoid_potential_buffer_overrun.diff with more correct
    correct version, thanks to Aurélien Gâteau (LP: #308060).

 -- Kees Cook <email address hidden> Fri, 27 Mar 2009 16:23:21 -0700