Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-70694

QWebsocket incorrect frame parsing

    XMLWordPrintable

Details

    • Bug
    • Resolution: Done
    • P1: Critical
    • 5.14.0
    • 5.11.2, 5.12.0, 5.12.1, 5.12.2, 5.12.3, 5.12.4, 5.13.0 Alpha 1, 5.13.0 RC 1, 5.13.0 RC 2, 5.13.0 RC 3, 5.13.0
    • WebSockets
    • None
    • All
    • b14f5f59a3ae96949e6a33302385a751d6448182

    Description

       

      The qt websocket implements the frame parser in the QWebSocketDataProcessor/QWebSocketFrame classes.

      Every time that the tcpsocket emits a readyRead() signal, the function QWebsocketDataProcess:: process(...) is called, which calls itself the QWebSocketFrame::readFrame(..), here's an extract:

      QWebSocketFrame QWebSocketFrame::readFrame(QIODevice *pIoDevice)
      {
          bool isDone = false;
          qint64 bytesRead = 0;
          QWebSocketFrame frame;
          quint64 dataWaitSize = 0;
          Q_UNUSED(dataWaitSize); // value is used in MACRO, Q_UNUSED to avoid compiler warnings
          ProcessingState processingState = PS_READ_HEADER;
          ProcessingState returnState = PS_READ_HEADER;
          bool hasMask = false;
          quint64 payloadLength = 0;
      
          while (!isDone)
          {
              switch (processingState) {
              case PS_WAIT_FOR_MORE_DATA:
                  //TODO: waitForReadyRead should really be changed
                  //now, when a WebSocket is used in a GUI thread
                  //the GUI will hang for at most 5 seconds
                  //maybe, a QStateMachine should be used
                  if (!pIoDevice->waitForReadyRead(5000)) {
                      frame.setError(QWebSocketProtocol::CloseCodeGoingAway,
                                     tr("Timeout when reading data from socket."));
                      processingState = PS_DISPATCH_RESULT;
                  } else {
                      processingState = returnState;
                  }
                  break;
      

      The person who wrote the code apparently knew himself, that this was incorrect seeing the comment "TODO".

      Indeed, the code here, is blocking (for maximum 5sec). As a result:
      1) This will block the event loop, therefore we can't use qwebsocket in gui thread unlike other socket class for Qt.
      We also can't use it in thread where the event loop cannot be blocked (for example thread where we have qt socket classes (they rely on event loop)).
      2) Waiting for only 5sec might be not enough, as the data from tcp network could come later, this would result in a incorrect websocket connection close.
      3) An attacker could use this to block the thread of the qwebsocket (which is the same as the qwebsocketserver). Indeed he could send small chunks of data continuously. This would cause a denial of service: as the event loop wouldn't be run, the qwebsocketserver wouldn't be able to process other clients.

      This class should be asynchronous, as the other qt socket classes (qtcpsocket, qudpsocket, qnetworkaccessmanager, etc)

       

       

      Attachments

        Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            People

              kurt.pattyn Kurt Pattyn
              enstone Franck Dude
              Votes:
              3 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Gerrit Reviews

                  There are no open Gerrit changes