search

shaka-project / shaka-packager

A media packaging and development framework for VOD and Live DASH and HLS applications, supporting Common Encryption for Widevine and other DRM Systems.
https://shaka-project.github.io/shaka-packager/
Other
1.9k stars 496 forks source link

fix: Proposed fix for #1316 #1317

Closed synologic closed 6 months ago

synologic commented 6 months ago

Fixes check in AesCbcEncryptor::CryptInternal which breaks CBCS encryption

google-cla[bot] commented 6 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

cosmin commented 6 months ago

The right fix is to update the definition of required ciphertext size. Currently it claims it needs to be one block larger than the real size because of implementation details mbedtls. This needs to be verified, and hopefully it's either not accurate, an alternative api could be used with mbedtls or we need to switch away from mbedtls if there is no workaround.

synologic commented 6 months ago

Switching away from mbedtls it's probably not a 5 minute job. Not having Fairplay, currently, is bad.

cosmin commented 6 months ago

Yes, there are some known issues currently with the mbedtls implementation in main, please use latest 2.6.1 release until this is resolved,

synologic commented 6 months ago

For my needs, i will stick to this fix on the latest main. There are notable differences (I am not sure how/when this changed) between 2.6.1 memory bandwidth usage and the latest main, but i'm seeing considerable drops in memory read/writes, as a matter of fact, memory traffic dropped about 3.5x just by using the latest main, which makes high density packaging much more efficient.

No problem to close this PR if it's unnaceptable :)

cosmin commented 6 months ago

@synologic what I believe to be the correct fix for this is in the fix-integration-tests if you want to give that a shot. Unless the current state of main that branch passes all integration tests so I have reasonable confidence that it should work for all scenarios. Due to the holidays it will take some time to get that reviewed and merged (and split into several smaller pull requests) but feel free to report any problems you find on that branch and I'll take a look.