Thrift
  1. Thrift
  2. THRIFT-1205

port server unduly fragile with arbitrary input

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.6.1
    • Fix Version/s: 0.8
    • Component/s: Java - Library
    • Labels:
    • Environment:

      javac 1.5.0_19, OS X 10.4.11

    • Patch Info:
      Patch Available

      Description

      Telnetting to the port and type a couple of arbitrary characters crashes the server almost immediately as follows. I haven't glanced at the relevant code. Is this reproducible on other platforms?

      $ ./run-server.sh 
      Starting the simple server...
      Exception in thread "Thread-0" java.lang.OutOfMemoryError: Java heap space
              at org.apache.thrift.protocol.TBinaryProtocol.readStringBody(TBinaryProtocol.java:353)
              at org.apache.thrift.protocol.TBinaryProtocol.readMessageBegin(TBinaryProtocol.java:215)
              at SimonSays$Processor.process(Unknown Source)
              at org.apache.thrift.server.TSimpleServer.serve(TSimpleServer.java:70)
              at JavaServer.simple(Unknown Source)
              at JavaServer$1.run(Unknown Source)
              at java.lang.Thread.run(Thread.java:613)
      

        Activity

        Toby Thain created issue -
        Hide
        Toby Thain added a comment -

        I should have linked to my program source:
        https://github.com/qu1j0t3/thriftwork

        Show
        Toby Thain added a comment - I should have linked to my program source: https://github.com/qu1j0t3/thriftwork
        Hide
        Bryan Duxbury added a comment -

        You should make use of the max frame size parameter to protect yourself from situations like these.

        Show
        Bryan Duxbury added a comment - You should make use of the max frame size parameter to protect yourself from situations like these.
        Bryan Duxbury made changes -
        Field Original Value New Value
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Duplicate [ 3 ]
        Hide
        Toby Thain added a comment -

        So you're saying that it's acceptable for the server to crash if anyone merely telnets into a public facing Thrift server (0.6.1) and types a couple of characters? And you don't consider that a security bug?

        Show
        Toby Thain added a comment - So you're saying that it's acceptable for the server to crash if anyone merely telnets into a public facing Thrift server (0.6.1) and types a couple of characters? And you don't consider that a security bug?
        Hide
        Ryan King added a comment -

        Thrift is not design for public facing services.

        Bryan- perhaps there should be a (large) default max frame size?

        Show
        Ryan King added a comment - Thrift is not design for public facing services. Bryan- perhaps there should be a (large) default max frame size?
        Hide
        Toby Thain added a comment -

        Fair enough, but:

        • it will inevitably be used in this way, unless (even if) there is a big disclaimer somewhere (did I miss it?)
        • doesn't Facebook use it for public facing services? What about their puzzle server?
        Show
        Toby Thain added a comment - Fair enough, but: it will inevitably be used in this way, unless (even if) there is a big disclaimer somewhere (did I miss it?) doesn't Facebook use it for public facing services? What about their puzzle server?
        Hide
        Bryan Duxbury added a comment -

        @Toby - The problem is that "typing a bunch of characters" is indistinguishable from the first few bytes of a really long message. I'd really like to have a good solution to this problem so I can point to that in the future, but so far no one has managed to propose a workable solution.

        @Ryan - I'd accept a patch for decreasing the default size to something reasonably large, if you spun a good argument for it.

        Show
        Bryan Duxbury added a comment - @Toby - The problem is that "typing a bunch of characters" is indistinguishable from the first few bytes of a really long message. I'd really like to have a good solution to this problem so I can point to that in the future, but so far no one has managed to propose a workable solution. @Ryan - I'd accept a patch for decreasing the default size to something reasonably large, if you spun a good argument for it.
        Hide
        Toby Thain added a comment -

        @Bryan - yes, I inferred that from the nature of the crash.

        I agree that (apart from a default limit, which should be a decent workaround), solving this is tricky. I haven't looked at the code yet, but it might be possible to guard against this by simply not allocating the entire buffer immediately, but rather a smaller interim buffer, and waiting to see if a valid message is forthcoming. This would work best if the rest of the unmarshalling code had thorough validity checks. This should avoid the attempt to allocate a meaninglessly large buffer in the case that the message can be rejected on other grounds. Of course, an attacker who's willing to craft a valid, extremely large Thrift message can still eat up server resources

        I can look at this approach if you think it has merit.

        Show
        Toby Thain added a comment - @Bryan - yes, I inferred that from the nature of the crash. I agree that (apart from a default limit, which should be a decent workaround), solving this is tricky. I haven't looked at the code yet, but it might be possible to guard against this by simply not allocating the entire buffer immediately, but rather a smaller interim buffer, and waiting to see if a valid message is forthcoming. This would work best if the rest of the unmarshalling code had thorough validity checks. This should avoid the attempt to allocate a meaninglessly large buffer in the case that the message can be rejected on other grounds. Of course, an attacker who's willing to craft a valid, extremely large Thrift message can still eat up server resources I can look at this approach if you think it has merit.
        Hide
        Bryan Duxbury added a comment -

        The "partial allocation" approach actually isn't workable. The Thrift deserialization code depends on the call stack for keeping state, so if it runs out of data, there's no nonblocking way for it to give up control and wait for more data to arrive. The whole premise of the nonblocking server is that we can know when all the bytes for a message have arrived without doing any actual deserialization, hence the requirement for framed transport.

        Show
        Bryan Duxbury added a comment - The "partial allocation" approach actually isn't workable. The Thrift deserialization code depends on the call stack for keeping state, so if it runs out of data, there's no nonblocking way for it to give up control and wait for more data to arrive. The whole premise of the nonblocking server is that we can know when all the bytes for a message have arrived without doing any actual deserialization, hence the requirement for framed transport.
        Hide
        Ryan King added a comment -

        Bryan-

        The default maxLength is now 2GB. I propose we set it to something like 64MB (or even less). I think that's still significantly bigger than the common use case. We should make the normal case (relatively small frames) safer and put the burden of customization on the rare cases.

        Show
        Ryan King added a comment - Bryan- The default maxLength is now 2GB. I propose we set it to something like 64MB (or even less). I think that's still significantly bigger than the common use case. We should make the normal case (relatively small frames) safer and put the burden of customization on the rare cases.
        Hide
        Toby Thain added a comment -

        @Ryan - I'd recommend less, maybe 1MB, and make this limit (and how to lift it) clear in the doc, along with the recommendation that Thrift not be used public-facing, due to these DoS risks. Or, better, float the proposal on the mailing list; I bet that even 1MB covers the vast majority of use cases.

        Show
        Toby Thain added a comment - @Ryan - I'd recommend less, maybe 1MB, and make this limit (and how to lift it) clear in the doc, along with the recommendation that Thrift not be used public-facing, due to these DoS risks. Or, better, float the proposal on the mailing list; I bet that even 1MB covers the vast majority of use cases.
        Hide
        Bryan Duxbury added a comment -

        I think 1MB is too small. This is a really tricky issue, because the bottom line is that people are definitely going to bump their heads into it before they go looking for the option, if they ever do.

        Show
        Bryan Duxbury added a comment - I think 1MB is too small. This is a really tricky issue, because the bottom line is that people are definitely going to bump their heads into it before they go looking for the option, if they ever do.
        Hide
        Toby Thain added a comment -

        But the exception in this situation can clearly state why it's being thrown, what the option is, and where to find it.

        Show
        Toby Thain added a comment - But the exception in this situation can clearly state why it's being thrown, what the option is, and where to find it.
        Hide
        Toby Thain added a comment -

        Actually, readFrame() is free to read the frame in chunks of any size, avoiding the need for upfront allocation, though when (if) fully read they would need to be assembled into a single byte[] given the current deserializer interface.

        It seems to me that this can be solved for all transports by having the deserializer ask for chunks instead of walking a whole frame: it's trivial to pretend a complete buffer is a nonblocking stream but pretending a blocking stream is a buffer leads to difficulties like those discussed here.

        Show
        Toby Thain added a comment - Actually, readFrame() is free to read the frame in chunks of any size, avoiding the need for upfront allocation, though when (if) fully read they would need to be assembled into a single byte[] given the current deserializer interface. It seems to me that this can be solved for all transports by having the deserializer ask for chunks instead of walking a whole frame: it's trivial to pretend a complete buffer is a nonblocking stream but pretending a blocking stream is a buffer leads to difficulties like those discussed here.
        Hide
        Bryan Duxbury added a comment -

        Actually, readFrame() is free to read the frame in chunks of any size, avoiding the need for upfront allocation

        This is true for FramedTransport, but not for the nonblocking servers themselves. We make an important distinction between the IO part of the request and the deserialization part of the request.

        this can be solved for all transports by having the deserializer ask for chunks instead of walking a whole frame

        I don't think you should underestimate the amount of work this would be. It's a completely different scheme for deserialization.

        Show
        Bryan Duxbury added a comment - Actually, readFrame() is free to read the frame in chunks of any size, avoiding the need for upfront allocation This is true for FramedTransport, but not for the nonblocking servers themselves. We make an important distinction between the IO part of the request and the deserialization part of the request. this can be solved for all transports by having the deserializer ask for chunks instead of walking a whole frame I don't think you should underestimate the amount of work this would be. It's a completely different scheme for deserialization.
        Hide
        Ryan King added a comment -

        reopening so i can attach a patch

        Show
        Ryan King added a comment - reopening so i can attach a patch
        Ryan King made changes -
        Resolution Duplicate [ 3 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Assignee Ryan King [ kingryan ]
        Hide
        Ryan King added a comment -

        Reduces the default frame limit to 16MB.

        Show
        Ryan King added a comment - Reduces the default frame limit to 16MB.
        Ryan King made changes -
        Ryan King made changes -
        Status Reopened [ 4 ] In Progress [ 3 ]
        Ryan King made changes -
        Patch Info [Patch Available]
        Hide
        Bryan Duxbury added a comment -

        I just committed this. Thanks for the patch.

        Show
        Bryan Duxbury added a comment - I just committed this. Thanks for the patch.
        Bryan Duxbury made changes -
        Status In Progress [ 3 ] Closed [ 6 ]
        Fix Version/s 0.8 [ 12316293 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in Thrift #218 (See https://builds.apache.org/job/Thrift/218/)
        THRIFT-1205. java: port server unduly fragile with arbitrary input

        Increase the default max frame size to 16MB.

        Patch: Ryan King

        bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1156731
        Files :

        • /thrift/trunk/lib/java/src/org/apache/thrift/transport/TFramedTransport.java
        • /thrift/trunk/lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
        Show
        Hudson added a comment - Integrated in Thrift #218 (See https://builds.apache.org/job/Thrift/218/ ) THRIFT-1205 . java: port server unduly fragile with arbitrary input Increase the default max frame size to 16MB. Patch: Ryan King bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1156731 Files : /thrift/trunk/lib/java/src/org/apache/thrift/transport/TFramedTransport.java /thrift/trunk/lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java

          People

          • Assignee:
            Ryan King
            Reporter:
            Toby Thain
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development