Multiframe encryption is not implemented correctly

[joeygrover]

Bug Report

MultiFrame Issues

There is currently a discrepancy between how multi-frame packets are encrypted and decrypted. It appears that at least the Java Suite and Core do them in opposite ways. For reference the two methods are Packet Payload Encryption vs Frame Payload Encryption.

Packet Payload

This method encrypts the entire payload of a packet first, then it chunks up that payload into multiple frames. This requires the receiver to buffer the entire payload of the large packet from all of the multi-frames before it decrypts anything. This leads to the issue that no block should be larger than the max TLS size, however if multi-frames are being sent it can be guaranteed that the encrypted payload would be larger than that.

Frame Payload

This method takes the large packet and chunks it up first using the TLS max block size. Then each frame has its payload encrypted individually. This request the receiver to decrypt the data prior to adding it to a buffer to rebuild the larger packet's complete payload.

Multi-Frame fails on Java Suite

The current library assumes that the data size received in the first frame describes the entire payload of what is to come. However this is with the assumption that it is supposed to perform Packet Payload encryption so it buffers the entire multi-frame message prior to decrypting. SDL Core is using Frame Payload encryption though and the data size included in the first frame describes the total number of decrypted bytes to buffer.

First Frame incorrect encryption flag

According to the protocol spec the first frame's payload must always be 8 bytes and therefore can't be encrypted. The library currently marks the packet as encrypted but does not actually encrypt the payload.

Incorrect Single Frame Encryption

The Java Suite will allow any packet that is less than the MTU size through as a single frame. This includes attempting to encrypt the payload of that same size. However, the max TLS record is usually much less than that of the MTU. This causes the encryption library to crash because it can't encrypt that much data. Even if the encryption library handled it, the java suite library only reads out the max TLS record size of bytes so anything larger than that will be lost.

The Java suite will encrypt up to the ~16k number of bytes correctly, but anything over that will crash the app.

Reproduction Steps

Receiving

• Start the RPC service with encryption enabled.
• Have Core send an encrypted OnSystemRequest
• Library crashes

Sending

• Start the RPC service with encryption enabled.
• Send an PutFile with a size >16k
• Notice the library fails

Expected Behavior

Encrypted RPCs are sent and received correctly regardless of if they are single or multi-frame

Observed Behavior

RPCs are not sent, library crashes etc.

[mrapitis]

I have also investigated this issue, and I have created some preliminary changes that align processing of multi-frame encrypted packet processing in the mobile library with core. The changes are not too extensive and can be found at the link below. Hopefully they can be taken into consideration while this issue is being worked on with the team.
mrapitis@e9af921

[joeygrover]

@mrapitis thank you!! I was set to learn some of things you already discovered today so that helped tremendously. I used some of the branch you created, but tried to avoid a lot of the byte array copies if possible. We have been testing today and so far everything looks good with the PR. If you have a chance to test it too that would be great. Pull request is number #1644

[mrapitis]

@joeygrover glad to help. I have tested your changes and they are working well with my setup. I tried encryption with onsystemrequest, systemrequest and audiopassthru. The updated coded looks cleaner also. Would it be possible for the team to investigate if similar changes are also required on iOS. Thanks again for your fixes.

[joeygrover]

@mrapitis That's awesome to hear!! We also tested against Core without the fix to not encrypt the first frame (so still sending an encrypted first frame) and that did cause an issue at first but I added another commit that took care of it. Previous versions of Core should also work now.

We are currently trying to work in the investigation/fixing for the iOS library. At first glance I believe we did confirm a similar issue exists. Timing is tough right now (testing & release), but it is a high priority issue to fix. With the confirmation that the linked PR solves the issue, I think that confirms an easier path to get iOS fixed as well. If in the worst case scenario we can't get it into this upcoming release, the issue on iOS is worthy of a hotfix right after.

[mrapitis]

It looks like there is just one small issue that existed in my branch as well, we need to reset the frameSequenceNumber back to 0x01 when we roll over 0xFF according to the protocol spec.

If the sequence reaches 0xFF with more frames to create, it shall rollover to 0x01 not 0x00 as it is reserved.

Something similar to below would probably work:

                            if (frameSequenceNumber ==
                                    SdlPacket.FRAME_INFO_FINAL_CONNESCUTIVE_FRAME) {
                                // we can't use 0x00 as frameSequenceNumber, because
                                // it's reserved for the last frame
                                ++frameSequenceNumber;
                            }

                            if (i == frameCount - 1) { //we are on the last frame
                                frameSequenceNumber = SdlPacket.FRAME_INFO_FINAL_CONNESCUTIVE_FRAME

[joeygrover]

@mrapitis good catch. That did get removed while I was cleaning up. Added it back in and pushed.