Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement multiframe encryption correctly #1644

Merged
merged 4 commits into from
Mar 25, 2021
Merged

Conversation

joeygrover
Copy link
Member

@joeygrover joeygrover commented Mar 23, 2021

Fixes #1642

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).
  • I have tested Android

Summary

The library is now setup to correctly encrypt multi frame packets by encrypting each frames payload rather than the entire payload first. The first frame will also no longer be marked as encrypted because it is not. Also fixed the logic for single frame messages when they were approaching the max size of a TLS record but still under the MTU. Receiving encrypted multiframe packets will no longer crash the library as the data size is reset to the decrypted data size before parsing further.

Changelog

Bug Fixes
  • Fixed incorrect encryption flag being set for first frame
  • Fixed encryption flow to match what is expected and frankly possible with TLS
  • Fixed incoming encrypted multiframe packets from crashing the app

CLA

Fixes both multiframe and single frame where single frame was actually over TLS max data size
@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #1644 (f12bffb) into release/5.1.0_RC (c480dd4) will decrease coverage by 0.02%.
The diff coverage is 5.40%.

❗ Current head f12bffb differs from pull request most recent head 385f314. Consider uploading reports for the commit 385f314 to get more accurate results
Impacted file tree graph

@@                  Coverage Diff                   @@
##             release/5.1.0_RC    #1644      +/-   ##
======================================================
- Coverage               54.06%   54.03%   -0.03%     
+ Complexity               5305     5304       -1     
======================================================
  Files                     555      555              
  Lines                   24504    24530      +26     
  Branches                 3088     3093       +5     
======================================================
+ Hits                    13248    13255       +7     
- Misses                  10107    10124      +17     
- Partials                 1149     1151       +2     
Impacted Files Coverage Δ Complexity Δ
...tdevicelink/managers/video/VideoStreamManager.java 22.33% <0.00%> (-0.42%) 21.00 <0.00> (ø)
.../com/smartdevicelink/protocol/SdlProtocolBase.java 15.79% <0.00%> (+0.20%) 22.00 <0.00> (ø)
...ain/java/com/smartdevicelink/transport/SdlPsm.java 20.65% <0.00%> (ø) 5.00 <0.00> (ø)
...tdevicelink/managers/screen/BaseScreenManager.java 68.78% <100.00%> (+0.33%) 56.00 <8.00> (ø)
...m/smartdevicelink/util/CorrelationIdGenerator.java 44.44% <0.00%> (-22.23%) 2.00% <0.00%> (-1.00%)
...nk/managers/audio/AudioDecoderCompatOperation.java 75.00% <0.00%> (-4.55%) 9.00% <0.00%> (-1.00%)
...rtdevicelink/streaming/video/SdlRemoteDisplay.java 51.85% <0.00%> (+1.23%) 5.00% <0.00%> (ø%)
.../main/java/com/smartdevicelink/util/DebugTool.java 17.20% <0.00%> (+2.15%) 12.00% <0.00%> (+1.00%)

This is added to deal with the case where Core would incorrectly encrypt the first frame which would make the payload length greater than 8 which is a protocol spec violation.
In the clean up effort this was removed but needs to be added back
@RHenigan RHenigan merged commit 33d2fbb into release/5.1.0_RC Mar 25, 2021
@RHenigan RHenigan deleted the bugfix/issue_1642 branch March 25, 2021 18:51
joeljfischer added a commit to smartdevicelink/sdl_ios that referenced this pull request Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants