Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.9
    • Fix Version/s: 2.0.10
    • Component/s: SSL
    • Labels:
      None
    • Environment:
      Android

      Description

      I've discovered an issue with the SslHandler class when the unwrap method is called on the local SSLEngine member (SslHandler.sslEngine).

      If the returned status is SSLEngineResult.Status.BUFFER_OVERFLOW, the capacity of the output buffer (SslHandler.appBuffer) can be increased to a size which still may is not large enough for the result.

      I have reproduced this issue consistently by sending a 4k frame over TLS1.2 to an android device. The frame gets heavily fragmented, sometimes into 6 frames, and the SSLEngine does not unwrap the frame until all the bytes have been received (since the hash is based on the entire frame).

      Since the frame gets heavily fragmented, the last segment of the frame can be lower than 2048 bytes. Hence by increasing the capacity by << 1, the output buffer will still be under the required size. (Have a look through SslHandler source for "appBuffer.capacity(appBuffer.capacity() << 1);")

      Anyway, the fix is really easy. Change the line:
      appBuffer.capacity(appBuffer.capacity() << 1);

      to:
      appBuffer.capacity(sslEngine.getSession().getApplicationBufferSize());

      This is actually in the java docs (http://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html) for the overflow buffer case.

      Hope this helps,
      Terence

        Activity

        Hide
        elecharny Emmanuel Lecharny added a comment -

        Not sure I get it : is it crashing for you, or is it just that we loop until we have a buffer big enough for the unwrap to be done ?

        In any case, the suggested modification sounds better (still have to review it carefully). I already ran the tests with it, all green.

        Thanks !

        Show
        elecharny Emmanuel Lecharny added a comment - Not sure I get it : is it crashing for you, or is it just that we loop until we have a buffer big enough for the unwrap to be done ? In any case, the suggested modification sounds better (still have to review it carefully). I already ran the tests with it, all green. Thanks !
        Hide
        tezm Terence Marks added a comment -

        Hey, sorry I skipped a few details.

        Ok, I did a tonne of debugging trying to work this out. Essentially, the SSLEngine has an internal buffer. So whenever you pass in bytes for it to unwrap, there is a chance that it can't unwrap it as the hash has not yet been received for the data, so instead it stores it locally and waits for more bytes to be passed in.

        There is one issue though - when it finally receives the hash and attempts to unwrap the entire frame, it will only return BUFFER_OVERFLOW once if the output buffer is too small. So essentially this behaviour is the actual problem. If you give it a buffer which can't fit in the bytes on the second unwrap call, it will just push whatever bytes it can onto the output buffer and discard the rest.

        Summary: SSLEngine.unwrap() will return BUFFER_OVERFLOW once, but not again if you give it a buffer which is too small when calling it the next time. This leads to bytes getting lost (root cause for me debugging this stuff in the first place).

        Please tell me that sort of makes sense lol,
        Terence

        Show
        tezm Terence Marks added a comment - Hey, sorry I skipped a few details. Ok, I did a tonne of debugging trying to work this out. Essentially, the SSLEngine has an internal buffer. So whenever you pass in bytes for it to unwrap, there is a chance that it can't unwrap it as the hash has not yet been received for the data, so instead it stores it locally and waits for more bytes to be passed in. There is one issue though - when it finally receives the hash and attempts to unwrap the entire frame, it will only return BUFFER_OVERFLOW once if the output buffer is too small. So essentially this behaviour is the actual problem. If you give it a buffer which can't fit in the bytes on the second unwrap call, it will just push whatever bytes it can onto the output buffer and discard the rest. Summary: SSLEngine.unwrap() will return BUFFER_OVERFLOW once, but not again if you give it a buffer which is too small when calling it the next time. This leads to bytes getting lost (root cause for me debugging this stuff in the first place). Please tell me that sort of makes sense lol, Terence
        Hide
        jeffmaury Jeff MAURY added a comment -

        I agree with you patch but, in my opinion, there is something broken with the Android SSL/TLS implementation. Can you confirm the MINA code is running on Android ?

        Show
        jeffmaury Jeff MAURY added a comment - I agree with you patch but, in my opinion, there is something broken with the Android SSL/TLS implementation. Can you confirm the MINA code is running on Android ?
        Hide
        tezm Terence Marks added a comment -

        Yes, I agree.

        I can't from outside of mina fix this issue. That's why I'm suggesting the change is needed because it allocates a buffer which is the actual size that the SSLEngine requires when BUFFER_OVERFLOW is returned.

        This fix is more of a hack because I don't see a reason why SSLEngine can't accept a buffer which is too small after consecutive tries. Maybe I'll pass this on to the google devs as well.

        Thanks,
        Terence

        Show
        tezm Terence Marks added a comment - Yes, I agree. I can't from outside of mina fix this issue. That's why I'm suggesting the change is needed because it allocates a buffer which is the actual size that the SSLEngine requires when BUFFER_OVERFLOW is returned. This fix is more of a hack because I don't see a reason why SSLEngine can't accept a buffer which is too small after consecutive tries. Maybe I'll pass this on to the google devs as well. Thanks, Terence
        Hide
        tezm Terence Marks added a comment -

        And yes it is definitely running on android and causing this issue.

        This was tested on a nexus 5 (completely vanilla android) version 5.1.1.

        Show
        tezm Terence Marks added a comment - And yes it is definitely running on android and causing this issue. This was tested on a nexus 5 (completely vanilla android) version 5.1.1.
        Hide
        jeffmaury Jeff MAURY added a comment -

        OK no problem this was just for clarification. We'll publish the code as it is an optimization (less loops)

        Show
        jeffmaury Jeff MAURY added a comment - OK no problem this was just for clarification. We'll publish the code as it is an optimization (less loops)
        Hide
        tezm Terence Marks added a comment -

        Thankyou. Will it just be a commit or an actual release?

        Show
        tezm Terence Marks added a comment - Thankyou. Will it just be a commit or an actual release?
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Regardless of the Android error, I think Terence is right about using getApplicationBufferSize(), instead of our own buffer which is grown on demand :

        Quoted from the Java API (http://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLSession.html#getApplicationBufferSize%28%29):

        All SSLEngine network buffers should be sized at least this large to avoid insufficient space problems when performing wrap and unwrap calls.
        

        I think teh pb is that the standard Java implementation is more permissive than the Android one here.

        I'll apply the suggested patch.

        Thanks !

        Show
        elecharny Emmanuel Lecharny added a comment - Regardless of the Android error, I think Terence is right about using getApplicationBufferSize(), instead of our own buffer which is grown on demand : Quoted from the Java API ( http://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLSession.html#getApplicationBufferSize%28%29): All SSLEngine network buffers should be sized at least this large to avoid insufficient space problems when performing wrap and unwrap calls. I think teh pb is that the standard Java implementation is more permissive than the Android one here. I'll apply the suggested patch. Thanks !
        Hide
        elecharny Emmanuel Lecharny added a comment -

        I just committed the patch in 2.0.

        We should check if it applies in 3.0.

        Show
        elecharny Emmanuel Lecharny added a comment - I just committed the patch in 2.0. We should check if it applies in 3.0.
        Show
        elecharny Emmanuel Lecharny added a comment - Fixed with http://git-wip-us.apache.org/repos/asf/mina/commit/e81c0eb0
        Hide
        tezm Terence Marks added a comment -

        Sweeettt, thankyou.

        Terence

        Show
        tezm Terence Marks added a comment - Sweeettt, thankyou. Terence
        Hide
        dlmiles Darryl L. Miles added a comment - - edited

        On what basis do you think something is broken in the Android SSL/TLS implementation ?

        Maybe you are not aware but SSL/TLS allows upto 64kb frame, a frame is an encapsulation of application plain text what has been encrypted and possibly padded and then had HMAC applied and then some SSL/TLS protocol bytes added to describe it.

        What appears to happen here is that Java SSLEngine implementation during decryption will not break plain text part into smaller chunks for you, it will only give you an entire frame decoded.

        If anything this is a failing in the java SSLEngine API, by silently truncating the data on the 2nd attempt is has effectively introduced a potential security attack vector that could have been better mitigated by making the error return stick. Nothing good can result from their current choice, it errs on the side of causing a security problem no preventing one for a security conscious feature. This point is moot.

        Note this is a feature of all SSL/TLS implementations (frame sizes upto 64kb), it just so happens that Android is capable of generating larger frames than you are seeing from your other SSL client. This is actually good for mobile because a frame has additional overhead so the larger the frames (more application plain text per frame) the lower the % of overhead from the SSL/TLS protocol adds to it (because the HMAC and frame type-id are a constant extra bytes added per frame, while the padding is usually upto 15 bytes but varies with padding and cipher choice). Less data sent means a cheaper mobile data usage bill.

        But most application uses smaller application buffers to feed their SSL encryption stream, usually <= 4k.

        Show
        dlmiles Darryl L. Miles added a comment - - edited On what basis do you think something is broken in the Android SSL/TLS implementation ? Maybe you are not aware but SSL/TLS allows upto 64kb frame, a frame is an encapsulation of application plain text what has been encrypted and possibly padded and then had HMAC applied and then some SSL/TLS protocol bytes added to describe it. What appears to happen here is that Java SSLEngine implementation during decryption will not break plain text part into smaller chunks for you, it will only give you an entire frame decoded. If anything this is a failing in the java SSLEngine API, by silently truncating the data on the 2nd attempt is has effectively introduced a potential security attack vector that could have been better mitigated by making the error return stick. Nothing good can result from their current choice, it errs on the side of causing a security problem no preventing one for a security conscious feature. This point is moot. Note this is a feature of all SSL/TLS implementations (frame sizes upto 64kb), it just so happens that Android is capable of generating larger frames than you are seeing from your other SSL client. This is actually good for mobile because a frame has additional overhead so the larger the frames (more application plain text per frame) the lower the % of overhead from the SSL/TLS protocol adds to it (because the HMAC and frame type-id are a constant extra bytes added per frame, while the padding is usually upto 15 bytes but varies with padding and cipher choice). Less data sent means a cheaper mobile data usage bill. But most application uses smaller application buffers to feed their SSL encryption stream, usually <= 4k.
        Hide
        tezm Terence Marks added a comment -
        Show
        tezm Terence Marks added a comment - SSLEngine is platform dependent. See https://android.googlesource.com/platform/external/conscrypt/+/lollipop-release/src/main/java/org/conscrypt/OpenSSLEngineImpl.java for Android's implementation of the SSLEngine.

          People

          • Assignee:
            Unassigned
            Reporter:
            tezm Terence Marks
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 24h
              24h
              Remaining:
              Remaining Estimate - 24h
              24h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development