-
Notifications
You must be signed in to change notification settings - Fork 466
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
CC_MD5 deprecation migration #415
Comments
What does "a cryptographically correct MD5 hash package" mean? My understanding is that MD5 itself is considered broken and not collision-resistant, which is the reason for the deprecation. |
I talked to @krizzu and our current plan is to migrate to a SHA256 hash |
@jonthanon an alternative I found was using https://github.com/onmyway133/SwiftHash but it would be introducing another dependency. |
Right, but the MD5 algorithm itself is compromised, regardless of the implementation. (Sorry, I think I'm communicating this poorly.) Either way, SHA256 sounds good! |
Ah makes sense! So MD5 in general is wrong, the only option is to migrate to SHA256. |
@safaiyeh Thanks for raising this up. Yes, we agreed that SHA256 is better, among those two.
|
Just a quick update, my focus is first finishing off this PR react-native-device-info/react-native-device-info#1057, it will take me a week. Then I'll fully focus on finishing this task. |
This issue has been marked as stale due to inactivity. Please respond or otherwise resolve the issue within 7 days or it will be closed. |
Current behavior
Currently Async Storage iOS relies on
RCTMD5Hash
which usesCC_MD5
to create a file hash https://github.com/react-native-community/async-storage/blob/af2664e5334175a180d71e22fe10e184904d63ff/ios/RNCAsyncStorage.m#L365CC_MD5 was deprecated in iOS 13 due to not being cryptographically correct.
This initially brought up in React Native core, facebook/react-native#29590
Expected behavior
Either migrate to a SHA256 encryption or use a cryptographically correct MD5 hash package.
I'm assuming a SHA256 encryption would be better as it won't introduce a new dependency. This might be just a change in React Native core, unless it would be better for Async Storage to own that functionality.
I'll make the PR for these, just want to confirm which direction is best to go.
Repro steps
Build React Native app with async-storage & targeting iOS 13.
Warning should show up.
Environment
The text was updated successfully, but these errors were encountered: