Fix IOS keychain critical bug ⚠️

[micheal1hany]

add archiver on data writing/reading to hide data from being attacked and retrieved from malicious tools

[micheal1hany]

Hello @juliansteenbakker, @mogol told me that you can help and support with me this changes so i'll explain the case.

The case is when i made dumping attack on the ios keychain over any app used flutter secure storage, i able to see what is the data hade and the screenshots i uploaded demonstrating the difference before and after my changes and it's very important as i assume .

• this is the data showed after dumping

• and this after i made my changes to hide the sensitive user data

[micheal1hany]

@juliansteenbakker if there's any thing i can help with in the tests don't hesitate to send me.

[juliansteenbakker]

Thanks for your work @micheal1hany. it seems like tests are failing due to an unrelated issue right now, which im going to solve first.

[micheal1hany]

Thanks for your work @micheal1hany. it seems like tests are failing due to an unrelated issue right now, which im going to solve first.

Ok fine, i just saw the logs and yes it's related to Gradle. Tyt

[juliansteenbakker]

Can you please merge the latest changes from develop into your fork? @micheal1hany

[micheal1hany]

Can you please merge the latest changes from develop into your fork? @micheal1hany

Ok no problem but maybe in the morning because it's late here in my time, I'll inform you once done.

[juliansteenbakker]

Can you please merge the latest changes from develop into your fork? @micheal1hany

Ok no problem but maybe in the morning because it's late here in my time, I'll inform you once done.

No problem, here in my time it's also way to late to be working 😉

[micheal1hany]

Can you please merge the latest changes from develop into your fork? @micheal1hany

Ok no problem but maybe in the morning because it's late here in my time, I'll inform you once done.

No problem, here in my time it's also way to late to be working 😉

Hello @juliansteenbakker , i just pulled develop to my fix-keychain forked branch. please inform me if you need anything else.

[bierbaumtim]

Is this change backward compatible in the sense that data stored with the current version of this plugin can be read after this PR has landed. If not, there should be a migration, as the stored data could be one-time generated data such as private keys or similar. If this data can no longer be read, it could break apps and possibly lead to data loss.

[micheal1hany]

Is this change backward compatible in the sense that data stored with the current version of this plugin can be read after this PR has landed. If not, there should be a migration, as the stored data could be one-time generated data such as private keys or similar. If this data can no longer be read, it could break apps and possibly lead to data loss.

i think it will not work directly and need to store those data from the beginning to achieve the new way, as a proposal you can make a patch for the app to delete all the keychain with your app/service name and store the data from the beginning if it possible for your case. If we need a migration function just tell me the case and I'll try to handle it

[bierbaumtim]

Your proposal for a patch doesn't make sense. If this PR landed I can't read the data from the keychain to save them in the new way and without this PR I can read the data but can not save them in the new way. So either way it's not working. Also I don't have a special case in mind, but rather the general case that all currently stored data must be readable after this PR landed.

So I think there should be a migration function. I propose the following function

Future<List<(String key, bool success)>> migrateKeychainValues(List<(String, iOSOptions)> keys);

And on the native side(pseudo code):

func migrate(_ args: [(key: String, options: Options)], _ result: @escaping FlutterResult) {
    let res: [(key: String, success: bool)] = []

    for t in args {
        read(...) // using the old way
        if (readSuccessfull) {
            write(...) // new way
            res.add((t.key, writeSuccessfull))
        } else {
            res.add((t.key, false))
        }
    }

    result(res)
}

I consider this PR as a breaking change since every developer has to run the migration at least once before accessing data in the keychain. Also, if the migration partially fails readAll doesn't work properly.

@juliansteenbakker What do you think of my concerns and how should we address them?

[juliansteenbakker]

I share the thoughts of @bierbaumtim. This would otherwise break all current implementation. Also, if we want to implement this on iOS we should also implement it on macOS, since that is almost the same codebase.