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 backup MAC checking. #8327

Closed
wants to merge 1 commit into from

Conversation

elkhadiy
Copy link
Contributor

@elkhadiy elkhadiy commented Oct 30, 2018

First time contributor checklist

Contributor checklist

  • Virtual device Pixel XL, Android 6.0
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

if(MessageDigest.isEqual(ourMac, theirMac) was always returning false since ourMac was of length 32 and theirMac was of length 10.

10-30 21:20:02.045 6562-6609/org.thoughtcrime.securesms D/MAC_CHECK: theirMac = C16A82AB25ACD6292668
10-30 21:20:02.046 6562-6609/org.thoughtcrime.securesms D/MAC_CHECK: ourMac   = C16A82AB25ACD6292668C5C4DFE3C8B3405EC4E87DC87E5B67B22536A68F8248

This issue most likely went under the radar since a ! was forgotten in the condition.

I'm also not sure of what happens when an exception is thrown in case of a MAC missmatch to the output stream in readAttachmentTo(OutputStream out, int length). There is a out.close() before the check, the file is written even if an exception is raised ?

@elkhadiy elkhadiy force-pushed the fix_backup_mac_check branch 2 times, most recently from a584249 to ec98046 Compare October 31, 2018 10:10
if(MessageDigest.isEqual(ourMac, theirMac) was always returning false
since ourMac was of length 32 and theirMac was of length 10.
@elkhadiy elkhadiy force-pushed the fix_backup_mac_check branch from ec98046 to 08892f2 Compare October 31, 2018 10:11
@greyson-signal
Copy link
Contributor

Tweaked slighty and merged in 787bcf7. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants