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

Validate the bytes received in TSaslTransport

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.9
    • 0.9.2
    • Java - Library
    • None
    • Patch Available

    Description

      In TSaslTransport#receiveSaslMessage, we are doing two things incorrectly:

      • Not validating the status byte code.
      • Not validating the decoded payload size integer before allocating a whole array with it.

      The latter especially is bad when a network security software sends a thrift server port some garbage data, causing it to receive failures like:

      java.lang.OutOfMemoryError: Java heap space
      	at org.apache.thrift.transport.TSaslTransport.receiveSaslMessage(TSaslTransport.java:181)
      	at org.apache.thrift.transport.TSaslServerTransport.handleSaslStartMessage(TSaslServerTransport.java:125)
      	at org.apache.thrift.transport.TSaslTransport.open(TSaslTransport.java:253)
      

      Or even,

      ERROR org.apache.thrift.server.TThreadPoolServer: Error occurred during processing of message.
      java.lang.NegativeArraySizeException
              at org.apache.thrift.transport.TSaslTransport.receiveSaslMessage(TSaslTransport.java:181)
              at org.apache.thrift.transport.TSaslServerTransport.handleSaslStartMessage(TSaslServerTransport.java:125)
              at org.apache.thrift.transport.TSaslTransport.open(TSaslTransport.java:253)
      

      Attachments

        1. THRIFT-2660.patch
          4 kB
          Harsh J
        2. THRIFT-2660.patch
          4 kB
          Harsh J

        Issue Links

          Activity

            qwertymaniac Harsh J added a comment -

            Improved test and fixed a bug with earlier fix that disallowed 0-len payloads (which are legit).

            qwertymaniac Harsh J added a comment - Improved test and fixed a bug with earlier fix that disallowed 0-len payloads (which are legit).
            roger Roger Meier added a comment -

            committed

            roger Roger Meier added a comment - committed
            roger Roger Meier added a comment -

            + committed changes from new patch

            roger Roger Meier added a comment - + committed changes from new patch
            hudson Hudson added a comment -

            FAILURE: Integrated in Thrift #1258 (See https://builds.apache.org/job/Thrift/1258/)
            THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev c983250355bc5fd572a6b56fc5637461ef868ec8)

            • lib/java/src/org/apache/thrift/transport/TSaslTransport.java
            • lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java
              THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev 5f841dff8b264708fe961186bd26c1016acdfca4)
            • lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java
            • lib/java/src/org/apache/thrift/transport/TSaslTransport.java
            hudson Hudson added a comment - FAILURE: Integrated in Thrift #1258 (See https://builds.apache.org/job/Thrift/1258/ ) THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev c983250355bc5fd572a6b56fc5637461ef868ec8) lib/java/src/org/apache/thrift/transport/TSaslTransport.java lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev 5f841dff8b264708fe961186bd26c1016acdfca4) lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java lib/java/src/org/apache/thrift/transport/TSaslTransport.java
            qwertymaniac Harsh J added a comment -

            Thanks roger.meier! Is there any chance this can be backported to make it appear in the upcoming 0.9.2 release? I did see an RC0 float up, so the answer may be no, but thought I'd ask.

            qwertymaniac Harsh J added a comment - Thanks roger.meier ! Is there any chance this can be backported to make it appear in the upcoming 0.9.2 release? I did see an RC0 float up, so the answer may be no, but thought I'd ask.
            roger Roger Meier added a comment -

            Yes, why not. jfarrell what do you think?

            roger Roger Meier added a comment - Yes, why not. jfarrell what do you think?
            jfarrell Jake Farrell added a comment -

            if its in the current trunk then it will get into the next rc, i've been working on the cross compile issues for arm based projects and was hoping to get that in as well. i might just push on that for the now and get this release candidate out

            jfarrell Jake Farrell added a comment - if its in the current trunk then it will get into the next rc, i've been working on the cross compile issues for arm based projects and was hoping to get that in as well. i might just push on that for the now and get this release candidate out
            qwertymaniac Harsh J added a comment -

            Thank you again, jfarrell and roger.meier!

            qwertymaniac Harsh J added a comment - Thank you again, jfarrell and roger.meier !
            brocknoland Brock Noland added a comment -

            Nice work!! I think the 100MB max message size will work for the vast majority of use cases, but I do think this should be configurable. I've created THRIFT-2678 to track that.

            brocknoland Brock Noland added a comment - Nice work!! I think the 100MB max message size will work for the vast majority of use cases, but I do think this should be configurable. I've created THRIFT-2678 to track that.
            hudson Hudson added a comment -

            SUCCESS: Integrated in Thrift #1331 (See https://builds.apache.org/job/Thrift/1331/)
            THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev c983250355bc5fd572a6b56fc5637461ef868ec8)

            • lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java
            • lib/java/src/org/apache/thrift/transport/TSaslTransport.java
              THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev 5f841dff8b264708fe961186bd26c1016acdfca4)
            • lib/java/src/org/apache/thrift/transport/TSaslTransport.java
            • lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java
            hudson Hudson added a comment - SUCCESS: Integrated in Thrift #1331 (See https://builds.apache.org/job/Thrift/1331/ ) THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev c983250355bc5fd572a6b56fc5637461ef868ec8) lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java lib/java/src/org/apache/thrift/transport/TSaslTransport.java THRIFT-2660 Validate the bytes received in TSaslTransport (roger: rev 5f841dff8b264708fe961186bd26c1016acdfca4) lib/java/src/org/apache/thrift/transport/TSaslTransport.java lib/java/test/org/apache/thrift/transport/TestTSaslTransports.java

            People

              roger Roger Meier
              qwertymaniac Harsh J
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: