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

[Albert Astals Cid]

El Dimecres, 12 de desembre de 2012, a les 21:16:44, Jaime va escriure:
> 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;

If we're sure we're not sending anything bigger than 4K makes sense.

Cheers,
  Albert


> 
> 
> 
> 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