Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-4682

C++ TBinaryProtocol crashes on port scan

    XMLWordPrintableJSON

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.10.0, 0.11.0
    • 0.14.0
    • C++ - Library
    • None
    • Patch Available
    • Patch, Important

    Description

      I'm afraid I don't have the expertise or time to submit a formal Git pull request and test cases, but I feel it's important for the Thrift community to be aware of a serious Denial of Service (DOS) attack vulnerability involving TBinaryProtocol. 

      A commercial port scanner application crashed all C++ Thrift server processes we had listening for TBinaryProcol connections. Investigation showed it was due to an assert verifying the initial packet length.  Crashing the server was as easy as sending an initial 4 bytes of zeroes after the client opens the socket.  I'm attaching a "TBinaryProtocolKiller.py" script that  will kill all C++ ThriftBinaryProtocol servers on an arbitrary IPv4 address within a provided port range. The script can be used to verify the fix.   This problem  does not occur in the  Python BinaryProtocol server;  I don't know about other languages.  The problem is present in 0.10.0 and 0.11.0, and I don't know about earlier releases. 

       My recommended patch is to simply change the assert to a regular error check, as shown below. 

      Index: lib/cpp/src/thrift/server/TNonblockingServer.cpp
      ===================================================================
      --- lib/cpp/src/thrift/server/TNonblockingServer.cpp	(revision 75217)
      +++ lib/cpp/src/thrift/server/TNonblockingServer.cpp	(working copy)
      @@ -473,8 +473,13 @@
      return;
      
      case SOCKET_RECV:
      - // It is an error to be in this state if we already have all the data
      - assert(readBufferPos_ < readWant_);
      + if (!(readBufferPos_ < readWant_)) {
      + GlobalOutput.printf("TNonblockingServer: frame size too short from client %s",
      + tSocket_->getSocketInfo().c_str());
      + close();
      + return;
      + }
      
      try {
      // Read from the socket

      Attachments

        1. THRIFT-4682.diff
          0.7 kB
          Michael Patrick
        2. TBinaryProtocolKiller.py
          2 kB
          Michael Patrick

        Issue Links

          Activity

            People

              jensg Jens Geyer
              mpatrick02702 Michael Patrick
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: