MaikuB / flutter_appauth

A Flutter wrapper for AppAuth iOS and Android SDKs
269 stars 238 forks source link

[flutter_appauth] - Update AppDelegate.swift in the example App #463

Closed davideravasi closed 5 months ago

davideravasi commented 7 months ago

I discover this potential data leak while playing with an IOS implementation of your library. You can double check..

MicrosoftTeams-image (7)
MaikuB commented 6 months ago

Thanks for the PR. On looking more into the details, this appears to be more relevant to "real apps". The example here points to a demo server and is only intended to be an example on how to use the plugin. Therefore it would seem like there isn't really value on applying the change and using the plugin is predominantly based on writing Dart code and then making changes to some resources on the native side so redirects are processed properly. The latter doesn't involve writing native code that developers wouldn't expect to look in an AppDelegate file

davideravasi commented 6 months ago

I'm ok with what you said, should we at least leave a comment somewhere (in the readme? Idk) to make people who are going to use this library aware of this? I'm saying this because, even if this library is not supposed to be 100% security oriented, I imagine who is using it has this intention.

Let me know if this makes sense.

MaikuB commented 5 months ago

It does though it's not to do with the library per se and from what I read, that data is only accessible to the app itself. Mentioning this as I'm questioning if this is a legitimate issue as I would've thought it would be more of a widespread concern to be conscious. I would expect other libraries to include warnings on this were that to be the case. Note I'm not saying this to say it shouldn't go in as a note. Just thinking out loud and that if a note were to be made, it may need to say that this is potentially an issue and developers will need to decide for themselves if it's needed i.e. do it at their own risk. The solution also seems to be predicated on making sure responses are not cached at all so may have other unintended side effects

davideravasi commented 5 months ago

Hey thanks for the answer. So a bit of background info, we discover this issue after a pentest of our app (I work for an highly regulated company). So yes, you are right, only the app itself can access this file but the problem is, if someone is able to debug locally your app with tools like Frida, the malicious actor can potentially retrieve and store the access token, which is definitely a security issue if you ask me personally.

And for the solution you are right, luckily we had only that webview in the app so not caching anything solved our issue but as you said, depending on your business scenario there might be potential side effects.

Overall I don't see any issue in mentioning this especially because this library is "sold" as a secure implementation of OAuth2/Open Id Connect and would be appreciated by a potential user being aware of those possible issue.

I would just mention in the readme that, the information after the login is stored the the .db file and if you are scared about leaving those traces in the app you can look online for possible remediations.

Let me know if this makes sense!

MaikuB commented 5 months ago

That makes sense though I initially thought perhaps iOS would be harder to crack that on Android when it comes to getting access to application data where it's known that a shared preferences is a plain text XML file that could be seen by rooting the device. Either way that is just based on my knowledge so like I was trying say earlier, should be fine to mention it but need to caution developers to be mindful of any unintended side effects

davideravasi commented 5 months ago

Yes to be fair it's perhaps possible that they used a jailbroken phone (so with a jailbreak detection mechanism enabled you should be fine... [not really tbh because with Frida they can bypass it especially if you use the 2-3 most famous libraries]).

Anyway, do you want me to open a PR on the .readme? Then you can review it.

I will try to keep it short an just mention this as a note.

Let me know :)

MaikuB commented 5 months ago

Yep a PR on a note is fine :)

davideravasi commented 5 months ago

Updated the .readme in another PR. As soon as you check that one we can close this one :)

davideravasi commented 5 months ago

Solved in the PR > https://github.com/MaikuB/flutter_appauth/pull/474