Issues decrypting LCP resources (CBCDRMInputStream)

[mickael-menu-mantano]

CBCDRMInputStream needs to be reviewed because there are a number of issues:

• @aferditamuriqi found out last July an issue with extra padding in non-deflated resources (which are decrypted with CBCDRMInputStream).
• When calculating the positionList of a LCPDF, we have to read the CBCDRMInputStream entirely into a Data buffer, otherwise random access doesn't work:
  https://github.com/readium/r2-streamer-swift/blob/1de6db2cf5c72e88384d238454a6fd8b9746022f/r2-streamer-swift/Parser/PDF/PDFParser.swift#L193-L197

A few clues:

• @danielweck left a comment on a code review that might be related to this issue.
• @aferditamuriqi made a quick fix that only applies to HTML resources to remove the padding. However, this should not depend on the content-type, but unfortunately she encountered issues with fonts if applied all the time: https://github.com/readium/r2-streamer-swift/blob/1de6db2cf5c72e88384d238454a6fd8b9746022f/r2-streamer-swift/Fetcher/DRM/CBCDRMInputStream.swift#L120-L124
• I jotted a few notes down, regarding the fonts issue when removing the padding:
  • This function is called for each range of data, so when removing the last byte, it is removed from each chunk instead of only at the end of the resource.
  • If a resource is bigger than a certain buffer size, then we split the read in several chunks. This usually doesn't happen for HTML because it's so small, but I noticed that fonts are usually read in 2 or more chunks.
  • It should have been enough to reduce the reported stream size in plainTextSize instead, but it's supposed to be done already.
• CBCDRMInputStream was directly translated from this C++ implementation. The only difference I can see is that the C++ version provides the type of padding. The private LCP framework doesn't take any padding scheme as a parameter though, so I'm not sure we're getting the proper outSize, which might explain why the padding could not be removed properly. Any thoughts on the padding scheme @danielweck?

[aferditamuriqi]

@mickael-menu can this issue be closed? can I assume it is fixed by this PR readium/r2-streamer-swift#140

[mickael-menu-mantano]

@aferditamuriqi No I didn't do anything to fix this issue so far

[danielweck]

I will take a look at the padding rules.

…

On Sat, 28 Dec 2019, 13:18 Mickaël Menu, ***@***.***> wrote: @aferditamuriqi <https://github.com/aferditamuriqi> No I didn't do anything to fix this issue so far — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/readium/r2-streamer-swift/issues/141?email_source=notifications&email_token=AAEYUMPSME3DBGIHSFOA2STQ25G4FA5CNFSM4JV2H4P2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYJXJI#issuecomment-569416613>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYUMI3RC75C5T7F2UMQM3Q25G4FANCNFSM4JV2H4PQ> .

[aferditamuriqi]

@mickael-menu ok, great, just wanted to make sure, since it was connected to a PR

[danielweck]

* [`CBCDRMInputStream` was directly translated from this C++ implementation](https://github.com/readium/readium-lcp-client/blob/764b28f5f4d879f771be7e727cf4a79ac0d62d9f/src/lcp-client-lib/AesCbcSymmetricAlgorithm.cpp#L125). The only difference I can see is that [the C++ version provides the type of padding](https://github.com/readium/readium-lcp-client/blob/764b28f5f4d879f771be7e727cf4a79ac0d62d9f/src/lcp-client-lib/AesCbcSymmetricAlgorithm.cpp#L122). The private LCP framework doesn't take any padding scheme as a parameter though, so I'm not sure we're getting the proper `outSize`, which might explain why the padding could not be removed properly. Any thoughts on the padding scheme @danielweck?

See this recent bugfix:

readium/readium-lcp-client@25d2e37

[mickael-menu]

@danielweck Thanks, this looks promising!

Do we need to provide the padding scheme as well? There's no way to do that with the R2 LCP lib.

// Figure out what padding scheme to use
BlockPaddingSchemeDef::BlockPaddingScheme padding = BlockPaddingSchemeDef::NO_PADDING;
if (total > (ssize - CryptoPP::AES::BLOCKSIZE))
{
    padding = BlockPaddingSchemeDef::W3C_PADDING; // Note that handling of W3C padding scheme during decryption also handles PKCS#7 (which is BlockPaddingSchemeDef::PKCS_PADDING in CryptoPP, with AES CBC Block Size > 8 (not PKCS#5))
}

[danielweck]

This is a CryptoPP implementation detail, strictly-speaking we do not need to ask the underlying crypto lib to automatically remove padding, instead we can just trim the padding suffix "manually".

[mickael-menu]

Equivalent issue on r2-lcp-kotlin: readium/kotlin-toolkit#207

And a Kotlin PR fixing it: readium/r2-lcp-kotlin#84