Bug 24138

Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/pymsn/msnp/base.py", line 106, in _dispatch_command
    handler(command)
  File "/usr/lib/python2.6/dist-packages/pymsn/msnp/notification.py", line 347, in _handle_ILN
    self._handle_NLN(command)
  File "/usr/lib/python2.6/dist-packages/pymsn/msnp/notification.py", line 386, in _handle_NLN
    urllib.unquote(command.arguments[5]))
  File "/usr/lib/python2.6/dist-packages/pymsn/p2p.py", line 160, in parse
    shac = base64.b64decode(shac)
  File "/usr/lib/python2.6/base64.py", line 76, in b64decode
    raise TypeError(msg)
TypeError: Incorrect padding

This bug is actually a DoS vulnerability in the way papyon handles MSNObject data in p2p.py.

There is no exception handling for malformed fields, such as the SHA1C in the bug filer's case. It tries to base 64 decode whether or not the string is properly encoded in base 64. If it isn't, it will crash silently with a traceback in the logs. The user will continue thinking he is talking to the person with the malformed checksum, but in reality, no messages will be sent or received.

More importantly, while going over this code, it appears that the checksum isn't even used when sending or receiving, yet it is stored in the object. If this is the intent, then malformed field exceptions should be ignored and a warning logged instead of crashing. However, it would be best if papyon verified the integrity of the fields and then compared checksums (just like what is done for the SHA1D). If the checksums do not match or the fields are malformed, then notify the user and drop the object from further processing.

I can kind of see the methodology behind what is being done now. The data hash is being checked and compared for integrity, and that's all you care about - the data. If this is the case and you see the checksum as redundant, then at least handle malformed ones by ignoring them instead of crashing on a bash 64 decode.

Created attachment 31049 [details] [review]
Quilt patch

The attached quilt patch ignores malformed SHA1Cs so that conversations don't crash. As mentioned earlier, though, there are a lot more places where checking needs to be done to make this framework secure. Check for KeyErrors in the xml, TypeErrors in the decodings, etc. I've managed to cause a crash remotely using several different methods.

*** Bug 25526 has been marked as a duplicate of this bug. ***

Thanks for you patch, I have pushed it into master. Could you please open bugs for all the area where such fallback are needed? Thanks in advance

*** Bug 26135 has been marked as a duplicate of this bug. ***

Still happening with version (0.4.5)

Traceback:
 Traceback (most recent call last):
   File "/usr/lib/pymodules/python2.6/papyon/msnp/base.py", line 107, in _dispatch_command
     handler(command)
   File "/usr/lib/pymodules/python2.6/papyon/msnp/notification.py", line 453, in _handle_NLN
     urllib.unquote(command.arguments[idx]))
   File "/usr/lib/pymodules/python2.6/papyon/p2p.py", line 159, in parse
     shad = base64.b64decode(shad)
   File "/usr/lib/python2.6/base64.py", line 76, in b64decode
     raise TypeError(msg)
 TypeError: Incorrect padding

Created attachment 34733 [details] [review]
Proposed patch

Like I said on irc, I'm pretty sure that's a dup of https://bugs.freedesktop.org/show_bug.cgi?id=26804 so I would like to see if it can be reproduced with the patch or some logs.

(In reply to comment #9)
> Like I said on irc, I'm pretty sure that's a dup of
> https://bugs.freedesktop.org/show_bug.cgi?id=26804 so I would like to see if it
> can be reproduced with the patch or some logs.

Unfortunately, it's not a duplicate as Laurent is running papyon with the said fix.

Olivier, do you have any objections against Laurent's patch? I personally don't know how else to go about fixing this.

(In reply to comment #10)
> (In reply to comment #9)
> > Like I said on irc, I'm pretty sure that's a dup of
> > https://bugs.freedesktop.org/show_bug.cgi?id=26804 so I would like to see if it
> > can be reproduced with the patch or some logs.
> 
> Unfortunately, it's not a duplicate as Laurent is running papyon with the said
> fix.
> 
> Olivier, do you have any objections against Laurent's patch? I personally don't
> know how else to go about fixing this.

I'm ok with it, but perhaps we should add more debug info in each exception it may proves helpfuls

(In reply to comment #11)
> perhaps we should add more debug info in each exception it
> may proves helpfuls

What kind of extra info do you mean? Check out my branch to fix this bug; I added another commit.

I mean having the failed sha[cd] in the exceptions, since most people don't provide the log but only the traceback that could be useful

(In reply to comment #13)
> I mean having the failed sha[cd] in the exceptions, since most people don't
> provide the log but only the traceback that could be useful

Yeah can't hurt. Check out my branch for all this then.