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

Do not overwrite or generate new Wallet Payment ID when wallet info cannot be decrypted #16373

Closed
Miyayes opened this issue Jun 11, 2021 · 6 comments · Fixed by brave/brave-core#9504

Comments

@Miyayes
Copy link
Collaborator

Miyayes commented Jun 11, 2021

Description

One effect of the current logic is that if the wallet info cannot be decrypted (e.g., OS encryption permissions are not granted to the Brave app), then the Rewards engine may try to generate a new Wallet Payment ID, which may overwrite the previous one.

Solution

  1. Prevent this from happening.
  2. Have proper error handling and messaging to the user in case wallet info cannot be decrypted.

Notes

Related to:

@Miyayes Miyayes added feature/rewards priority/P2 A bad problem. We might uplift this to the next planned release. OS/Android Fixes related to Android browser functionality OS/Desktop labels Jun 11, 2021
@zenparsing zenparsing added this to the 1.29.x - Nightly milestone Jul 23, 2021
@stephendonner
Copy link

stephendonner commented Aug 3, 2021

Verified PASSED using the testplan at brave/brave-core#9504 with

Brave 1.29.46 Chromium: 92.0.4515.115 (Official Build) nightly (x86_64)
Revision 48cb2f4029b84b003719740a6cf9ca73f374a857-refs/branch-heads/4515_105@{#4}
OS macOS Version 11.5.1 (Build 20G80)

Scenario: App loses access to system password store

Steps:

  1. Opened the browser with a clean profile on staging.
  2. Opened the brave://rewards panel and clicked "Start using Brave Rewards" to generate a rewards wallet.
  3. Accepted the 30.000 BAT UGP grant.
  4. Closed the browser.
  5. Opened the Keychain Access app and removed Brave Browser Nightly.app from the Brave Safe Storage list.
  6. Started the browser and denied the app's request to access the system password store.
  7. Navigated to brave://rewards.
  8. Toggled Ads to off.
  9. Navigated to brave://rewards-internals and viewed the logs.
  10. Verified a "Decryption failed" message was logged.
  11. Restarted the browser, and granted access to the system password store.
  12. Confirmed that Rewards tipping and auto-contributions were working.
example example example example example example example example example
Screen Shot 2021-08-03 at 10 32 26 AM Screen Shot 2021-08-03 at 10 33 28 AM Screen Shot 2021-08-03 at 10 33 50 AM Screen Shot 2021-08-03 at 10 34 39 AM Screen Shot 2021-08-03 at 10 35 10 AM Screen Shot 2021-08-03 at 10 35 48 AM Screen Shot 2021-08-03 at 10 36 26 AM Screen Shot 2021-08-03 at 10 39 01 AM Screen Shot 2021-08-03 at 10 39 31 AM

Scenario: Rewards wallet data is corrupted

Steps:

  1. Opened the browser with a clean profile on staging.
  2. Opened the brave://rewards panel and clicked "Start using Brave Rewards" to generate a rewards wallet.
  3. Accepted the 30.000 BAT UGP grant.
  4. Closed the browser.
  5. Added junk characters to the value in brave.rewards.wallets.brave
  6. Launched Brave
  7. Opened brave://rewards-internals
  8. Confirmed I saw the following in the logfile:
[Aug 17, 2021, 12:27:22.0 PM:ERROR:state.cc(387)] Base64 decoding failed for wallets.brave
[Aug 17, 2021, 12:27:22.0 PM:ERROR:ledger_impl.cc(579)] Wallet is null
  1. Confirmed I could tip and auto-contribute works too.
example example example example
Screen Shot 2021-08-17 at 12 17 47 PM Screen Shot 2021-08-17 at 12 21 00 PM Screen Shot 2021-08-17 at 12 27 36 PM Screen Shot 2021-08-17 at 12 43 59 PM

Verification passed on

Brave 1.29.60 Chromium: 92.0.4515.131 (Official Build) beta (64-bit)
Revision 6b8d6c56ce21e38a72f7c4becb5abc1fa5134f29-refs/branch-heads/4515@{#1933}
OS Ubuntu 18.04 LTS

Verified test plan from brave/brave-core#9504

Scenario: App loses access to system password store

image
image
image

Scenario: Rewards wallet data is corrupted

image


Verification PASSED on Win 10 x64 using the following build:

Brave | 1.29.75 Chromium: 93.0.4577.58 (Official Build) (64-bit)
-- | --
Revision | c4410ece044414ea42fa4ba328d08195e818a99c-refs/branch-heads/4577@{#1076}
OS | Windows 10 OS Version 2009 (Build 19042.1165)
  • Started Brave on staging and enabled rewards via the rewards panel
  • once rewards was enabled, claimed the grant for 30 BAT
  • closed brave and corrupted brave.rewards.wallets.brave via the Preference file

Once the above was completed, launched Brave and ensured that the following appeared within the logs/terminal:

[94300:64832:0830/134227.983:INFO:state.cc(387)] Base64 decoding failed for wallets.brave
[94300:64832:0830/134227.983:INFO:ledger_impl.cc(579)] Wallet is null

Ensured that both tipping & AC is working after receiving the Wallet is null error in the terminal/reward logs:

Tipping Example Tipping Tipping Example
image image
AC Example AC Example
image image

Ensured that ad notifications was also working as per the following:

[95960:102320:0830/140319.500:INFO:notification_helper_win.cc(200)] Notifications are enabled
[95960:102320:0830/140319.502:VERBOSE1:eligible_ad_notifications.cc(87)] Get eligible ads for parent-child segments:
[95960:102320:0830/140319.502:VERBOSE1:eligible_ad_notifications.cc(89)]   sports-cycling
[95960:102320:0830/140319.502:VERBOSE1:eligible_ad_notifications.cc(89)]   technology & computing-programming
[95960:102320:0830/140319.502:VERBOSE1:eligible_ad_notifications.cc(89)]   travel-travel
[95960:102320:0830/140319.503:VERBOSE1:eligible_ad_notifications.cc(100)] No eligible ads for parent-child segments
[95960:102320:0830/140319.503:VERBOSE1:eligible_ad_notifications.cc(122)] Get eligible ads for parent segments:
[95960:102320:0830/140319.503:VERBOSE1:eligible_ad_notifications.cc(124)]   sports
[95960:102320:0830/140319.503:VERBOSE1:eligible_ad_notifications.cc(124)]   technology & computing
[95960:102320:0830/140319.503:VERBOSE1:eligible_ad_notifications.cc(124)]   travel
[95960:102320:0830/140319.504:VERBOSE1:eligible_ad_notifications.cc(135)] No eligible ads for parent segments
[95960:102320:0830/140319.504:VERBOSE1:eligible_ad_notifications.cc(147)] Get eligble ads for untargeted segment
[95960:102320:0830/140319.505:VERBOSE2:ad_priority.h(32)] 18 ads with a priority of 1 in bucket 1
[95960:102320:0830/140319.505:VERBOSE1:ad_notification_serving.cc(126)] Found 18 eligible ads
[95960:102320:0830/140319.506:VERBOSE1:ad_notification_serving.cc(222)] Serving ad notification:
  uuid: dd4b70e8-ba76-489e-8ac0-4cbcd3660ca1
  creativeInstanceId: faec6d5b-9e30-45a5-8420-3435abe1695a
  creativeSetId: 45ea5952-37ca-41f0-88c2-4b546b0e6f4b
  campaignId: 1c1ac828-db13-49bb-9128-090132044b7f
  advertiserId: ecbcc833-8b1d-4867-98f7-ad2341396ce8
  segment: untargeted
  title: Ad6
  body: Ad6
  targetUrl: https://brave.com
ads Example ads Example
ads1 ads2

@GeetaSarvadnya
Copy link

@zenparsing Is there any way to test this issue on Windows as the keychain concept is not there in windows.

@zenparsing
Copy link

@GeetaSarvadnya I haven't been able to discover how to manipulate user-based encryption keys on Windows, perhaps @emerick might know? Otherwise, it should be sufficient to follow the "Rewards wallet data is corrupted" scenario, since that will test the same code paths.

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Aug 24, 2021

@zenparsing To test the "Rewards wallet data is corrupted" scenario, I would need corrupted brave.rewards.wallets.brave preferences profile as per my understanding. Please let me know if you have specific steps to test this scenario.

@zenparsing zenparsing removed the OS/Android Fixes related to Android browser functionality label Aug 25, 2021
@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Aug 30, 2021

@zenparsing @emerick If there are no specific steps to verify the issue on Windows, probably we can add macOS and Linux label only.

cc: @LaurenWags @kjozwiak

@kjozwiak
Copy link
Member

So Win does have a built-in password store in C:\Windows\System32\Config\ via the SAM file but encrypted using the system file. You'll need to either download a tool that decrypts the SAM file or boot into a Linux distro in the same machine and extract the hashes from SAM. I'm not sure if it's even possible to edit/replace the SAM file on the system. Assuming editing/replacing the SAM file is going to case a lot of issues. QA will only check the corrupted preference file case. CCing @zenparsing

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

Successfully merging a pull request may close this issue.

7 participants