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

Fix broken file padding logic #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

accwebs
Copy link

@accwebs accwebs commented Oct 27, 2024

Bad padding logic for file encryption/decryption was causing updated manifest files to be detected to be invalid by iTunes and other software.

…s to be detected to be invalid by iTunes and other software
@accwebs
Copy link
Author

accwebs commented Oct 28, 2024

I also have a follow-up change to this one prepared which completely eliminates the size-attribute based truncation logic

https://github.com/accwebs/iTunes-Backup-Explorer/tree/feature/remove-size-attribute-truncation

See the commit message for reasoning...

Let me know if you want both these commits lumped together on the same PR.

@MaxiHuHe04
Copy link
Owner

MaxiHuHe04 commented Oct 28, 2024

Ah, this is great. Thank you! I didn't know the padding size is embedded at the end of the files.
I also have been wanting to take a closer look at the change to NIO from the last PR #63 for a while now but haven't gotten around to doing it yet. It's unfortunate that you both worked on the same part, but if I can confirm the better performance I think it makes sense to merge his buffer optimizations and then apply your fix on that.
I hope I will find time in the next couple of days.

@MaxiHuHe04
Copy link
Owner

I just wonder now how this could affect file deletion. As I decrypt the backup database without the size argument, maybe iTunes doesn't like that the padding isn't removed correctly there even though I haven't had a problem with SQLite clients not being able to read it.

@accwebs
Copy link
Author

accwebs commented Oct 28, 2024

I think it's mainly just that any "correct" decryption logic will check the padding and reject the ciphertext if the padding is corrupt/invalid post decrypt. So likely any edit that re-encrypts the metadata sqlite database would 'corrupt' the database in the eyes of other tools (due to the 'old' encryption logic not padding the sqlite file properly).

@accwebs
Copy link
Author

accwebs commented Oct 28, 2024

One realization I had late yesterday about this PR: probably I could completely eliminate much of the spaghetti padding logic I wrote by just changing the Cipher algorithm to AES/CBC/PKCS5Padding. I'm not 100% sure this would work; I'd have to test it.

I considered this approach originally but at the time I was 'maintaining' the Size attribute truncation logic which required me to NOT change it (since the rewritten padding logic was conditional based on whether size was supplied or not. But then I realized I wanted to drop that logic in a subsequent commit.

LMK if you have any opinions/preferences on this.

@accwebs
Copy link
Author

accwebs commented Nov 12, 2024

Still waiting for feedback. LMK if you think I should just rework to use the built-in Java padding mode or stick with how I did things here for now.

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