fix: BAD_DECRYPT Exception on invalid cipher #1730

M123-dev · 2022-05-02T10:20:23Z

What

Fixes: PlatformException: PlatformException(Exception encountered, read, javax.crypto.BadPaddingException: error:1e000065:C... #1121
Fixes: Infinite account loading spinner because the app can't access the credentials anymore #1311

codecov-commenter · 2022-05-02T10:25:19Z

Codecov Report

Merging #1730 (1a27153) into develop (2ea0da3) will decrease coverage by 0.59%.
The diff coverage is 0.64%.

@@            Coverage Diff             @@
##           develop   #1730      +/-   ##
==========================================
- Coverage     8.86%   8.26%   -0.60%     
==========================================
  Files          161     165       +4     
  Lines         6623    7148     +525     
==========================================
+ Hits           587     591       +4     
- Misses        6036    6557     +521

Impacted Files	Coverage Δ
...h_app/lib/cards/category_cards/abstract_cache.dart	`0.00% <0.00%> (ø)`
...p/lib/cards/category_cards/asset_cache_helper.dart	`0.00% <0.00%> (ø)`
...p/lib/cards/category_cards/raster_async_asset.dart	`0.00% <0.00%> (ø)`
...oth_app/lib/cards/category_cards/raster_cache.dart	`0.00% <0.00%> (ø)`
..._app/lib/cards/category_cards/svg_async_asset.dart	`0.00% <0.00%> (ø)`
...smooth_app/lib/cards/category_cards/svg_cache.dart	`0.00% <0.00%> (ø)`
...t_cards/knowledge_panels/knowledge_panel_card.dart	`0.00% <0.00%> (ø)`
...t_cards/knowledge_panels/knowledge_panel_page.dart	`0.00% <0.00%> (ø)`
...knowledge_panels/knowledge_panel_summary_card.dart	`0.00% <ø> (ø)`
...s/knowledge_panels/knowledge_panel_table_card.dart	`0.00% <0.00%> (ø)`
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19e3560...1a27153. Read the comment docs.

teolemon · 2022-05-02T11:37:39Z

packages/smooth_app/lib/data_models/user_management_provider.dart

+    try {
+      userId = await DaoSecuredString.get(_USER_ID);
+      password = await DaoSecuredString.get(_PASSWORD);
+    } on PlatformException {


Could you add a comment here on the kind of exception it was (SSL related, possibly on old devices ?)

Added a comment

monsieurtanuki

Hi @M123-dev!
I think it's not fair to just bypass the exception. What are we supposed to do with "less fortunate" users? Shouldn't we store the user/password in DaoString instead, as a fallback? Shouldn't we have detected that issue previously when the user stored the user/password: it probably didn't work either at that moment.
Any idea of the root cause of this crash? (there's no "issue" by the way).

M123-dev · 2022-05-03T15:09:28Z

Heyyy @monsieurtanuki, the API used in the secure storage package was introduced in android 4.3 (Now around 10 years old), I think we have a lot more other problems on these version. The error occured to me when I had the app installed via the PlayStore and then installed it via USB Debugging from my PC (for running in debug mode). After that it fails what's good in terms of security. But if we know that such a case meight happen we don't need to throw a exeption but just handle it. But because it's a security and not feature related bug there is no way of knowing it in advance.

#1121 #1311

monsieurtanuki

Hi @M123-dev!

I didn't know what you mentioned was possible; when I develop in Android/java the app refuses to install a debug apk on a release apk and so on. That doesn't work that way with flutter, fair enough.

The general idea is good: in those strange cases just display "you have been logged out", remove the credentials and that's it.

The timing is not good though: adding a Scaffold (for the Snackbar) is not a good solution, as we've already experienced bugs because of embedded Scaffolds. Please call the Snackbar somewhere else.

packages/smooth_app/lib/main.dart

monsieurtanuki

@M123-dev Not convinced either by your latest change. That's tricky...
Let's say that only naughty boys like you may experience that "logged out" exception: what about just printing a log? And moving the code back to main.dart?

M123-dev · 2022-05-03T16:38:47Z

Sure @monsieurtanuki

monsieurtanuki · 2022-05-03T16:45:26Z

packages/smooth_app/lib/main.dart

@@ -227,6 +227,8 @@ class SmoothAppGetLanguage extends StatelessWidget {
    final LocalDatabase _localDatabase = context.read<LocalDatabase>();
    AnalyticsHelper.trackStart(_localDatabase, context);

+    context.read<UserManagementProvider>().mountCredentials();


Not await? Btw as it's in the init part and we do that only once, do we have to notify the listeners in mountCredentials? I don't think so, as it's dangerous to notify while building a Widget.

no await intended, since the credentials are not needed directly after start, this can run when there is time for that. Because of that that little more efford with the provider

Not convinced. All we do in mountCredentials is get the credentials from the dao and put them in static. Quick and easy.
OK, technically you are allowed not to wait until it finishes and use the provider for the refresh, but:

hey, it's init time, let's init once and for all

the notifylistener will trigger a display refresh just because you didn't want to spend 20 milliseconds awaiting

not everything we do is strictly needed at init time - we also set the query country, which is useless as we haven't queried anything yet

btw mountCredentials could be called much earlier, in _init1 or _init2

Just applied your suggestions, my first approach was to completely remove the provider, but looking into it again we should keep it to properly reflect logins from for example the login card.

packages/smooth_app/lib/data_models/user_management_provider.dart

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

M123-dev · 2022-05-05T16:28:41Z

@monsieurtanuki just saw the stats on Sentry, looks like I'm really the only one with the problem 😆

fix: BAD_DECRYPT Exception on invalid cipher

83fbbcb

M123-dev added the security label May 2, 2022

M123-dev requested a review from monsieurtanuki May 2, 2022 10:20

M123-dev requested a review from a team as a code owner May 2, 2022 10:20

github-actions bot assigned M123-dev May 2, 2022

Update user_management_provider.dart

4dbd53d

teolemon reviewed May 2, 2022

View reviewed changes

monsieurtanuki reviewed May 2, 2022

View reviewed changes

More comments + showing SnackBar

bc84460

github-actions bot added the 🌐 l10n label May 3, 2022

monsieurtanuki reviewed May 3, 2022

View reviewed changes

packages/smooth_app/lib/main.dart Outdated Show resolved Hide resolved

Moving cred movment lower in the tree

d6d63f3

monsieurtanuki reviewed May 3, 2022

View reviewed changes

Moved back

af0d5bd

github-actions bot removed the 🌐 l10n label May 3, 2022

monsieurtanuki reviewed May 3, 2022

View reviewed changes

teolemon added this to the V1 milestone May 4, 2022

Await user credentials on app start

6eaf3ec

github-actions bot added the 👥 User management Account login, signup, signout label May 4, 2022

monsieurtanuki approved these changes May 4, 2022

View reviewed changes

packages/smooth_app/lib/data_models/user_management_provider.dart Outdated Show resolved Hide resolved

Update packages/smooth_app/lib/data_models/user_management_provider.dart

1a27153

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

monsieurtanuki approved these changes May 4, 2022

View reviewed changes

M123-dev merged commit 955895d into develop May 4, 2022

M123-dev deleted the fix-BAD_DECRYPT-error branch May 4, 2022 13:38

github-actions bot mentioned this pull request May 4, 2022

chore(develop): release 1.0.0 #962

Closed

teolemon added the SSL label Nov 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: BAD_DECRYPT Exception on invalid cipher #1730

fix: BAD_DECRYPT Exception on invalid cipher #1730

M123-dev commented May 2, 2022

codecov-commenter commented May 2, 2022 •

edited

Loading

teolemon May 2, 2022

M123-dev May 3, 2022

monsieurtanuki left a comment

M123-dev commented May 3, 2022

monsieurtanuki left a comment

monsieurtanuki left a comment

M123-dev commented May 3, 2022

monsieurtanuki May 3, 2022

M123-dev May 3, 2022

monsieurtanuki May 3, 2022

M123-dev May 4, 2022

M123-dev commented May 5, 2022 •

edited

Loading

fix: BAD_DECRYPT Exception on invalid cipher #1730

fix: BAD_DECRYPT Exception on invalid cipher #1730

Conversation

M123-dev commented May 2, 2022

What

codecov-commenter commented May 2, 2022 • edited Loading

Codecov Report

teolemon May 2, 2022

Choose a reason for hiding this comment

M123-dev May 3, 2022

Choose a reason for hiding this comment

monsieurtanuki left a comment

Choose a reason for hiding this comment

M123-dev commented May 3, 2022

monsieurtanuki left a comment

Choose a reason for hiding this comment

monsieurtanuki left a comment

Choose a reason for hiding this comment

M123-dev commented May 3, 2022

monsieurtanuki May 3, 2022

Choose a reason for hiding this comment

M123-dev May 3, 2022

Choose a reason for hiding this comment

monsieurtanuki May 3, 2022

Choose a reason for hiding this comment

M123-dev May 4, 2022

Choose a reason for hiding this comment

M123-dev commented May 5, 2022 • edited Loading

codecov-commenter commented May 2, 2022 •

edited

Loading

M123-dev commented May 5, 2022 •

edited

Loading