[Kde-games-devel] Review Request: Avoid a crash when knavalbattle receives a valid (but understood) xml message

[Jaime]

More than that...
The server must be able to handle all kind of requests, not only the ones
coming from any version of the client, but all what any script kiddie is
able to throw to it:

This is valid to any input expecting XML (or JSON) coming from files or
network.

The server does not crash now with the following attacks:
XML following the game schema
simple XML non following the game schema
Non XML data only
simple valid XML and simple invalid XML

And, do not forget these:
XML with gigantic node names (a tag name of 1.000.000 'a' will cause the
server to allocate 1Gb of memory?)
XML with gigantic values (a value of 1.000.000 'a' will cause the server to
allocate 1Gb of memory?)
XML with gigantic node hierarchy (a tag inside a tag 1.000.000 times will
cause the server to allocate >1Gb of memory?)

To avoid the last 3, I propose this simple patch: (I'll use review-board,
but will loose this thread)

diff --git a/src/protocol.cpp b/src/protocol.cpp
index 791179b..c5cbacc 100644
--- a/src/protocol.cpp
+++ b/src/protocol.cpp
@@ -124,7 +124,7 @@ Protocol::Protocol(QIODevice* device)

 void Protocol::readMore()
 {
-    QByteArray data = m_device->readAll();
+    QByteArray data = m_device->read(4096);
     m_buffer += QString::fromUtf8(data.constData());

     int pos;



And attached is a simple compressed xml file that I've used to test it:
cat b.xml | nc localhost 54321   (nc is netcat, used to send the file to a
listening knavalbattle)


It will create an opponent name of 9000 b, with the patch, that name is not
processed, and is not allocating an string of length 9000. You are free to
try to create/use a file with more 'b's. My vi karma and konsole does not
allow it to me (in a smooth way).




2012/12/9 Wolfgang Rohdewald <wolfgang at rohdewald.de>

> On Sunday 09 December 2012 18:42:45 Albert Astals Cid wrote:
> > > Albert Astals Cid wrote:
> > >     Actually maybe it would make more sense not emmiting a null
> message from the protocol.cpp file instead of protecting from a null
> message in the networkentity.cpp slot? Anyway any of the two solutions
> work, commit the one you prefer
>
> client and server might not be updated at the same time.
> So if you do both changes, updating either client or server would be
> sufficient
>
> --
> Wolfgang
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20121212/54bcef53/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: b.xml.gz
Type: application/x-gzip
Size: 239 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20121212/54bcef53/attachment.gz>