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

Stronger checks for invalid sync codes #19200

Closed
fmarier opened this issue Nov 3, 2021 · 7 comments · Fixed by brave/brave-core#10960
Closed

Stronger checks for invalid sync codes #19200

fmarier opened this issue Nov 3, 2021 · 7 comments · Fixed by brave/brave-core#10960
Assignees

Comments

@fmarier
Copy link
Member

fmarier commented Nov 3, 2021

Here are a few small improvements that came up during a review of the Sync code:

None of these were found to be exploitable, but we'd like this code to be extra-paranoid about key-generation errors.

@fmarier fmarier added security feature/sync OS/Android Fixes related to Android browser functionality OS/Desktop labels Nov 3, 2021
@rebron rebron added the priority/P2 A bad problem. We might uplift this to the next planned release. label Nov 9, 2021
@rebron
Copy link
Collaborator

rebron commented Nov 9, 2021

cc: @jsecretan

@AlexeyBarabash
Copy link
Contributor

AlexeyBarabash commented Nov 9, 2021

Not sure about

We should enforce a minimum size (32?) here: https://github.com/brave/brave-core/blob/d1c22d333d92f8fd76a6251e8c3c2fdfbdbae63c/components/brave_sync/crypto/crypto.cc#L22

maybe it will require server change
it is already 32

@fmarier
Copy link
Member Author

fmarier commented Nov 9, 2021

That's right, it already defaults to 32. The change I'm asking for is just to enforce that in the function.

It could be as adding:

CHECK(size >= 32);

at the beginning of that function to crash the browser should this function ever get misused in the future.

@AlexeyBarabash
Copy link
Contributor

@fmarier thanks
I am working on draft PR, how about brave/brave-core@3dc3784#diff-2ef0c64527a52e5910aa131da0822a63d0ab0bae6250bb768bf672d11b9b9287R22 ?

When having CHECK - this makes impossible to make the unit test , because we don't have EXPECT_CRASH option.

@AlexeyBarabash
Copy link
Contributor

PR is opened for review

@stephendonner
Copy link

stephendonner commented Nov 15, 2021

Verified PASSED using

Brave 1.32.103 Chromium: 96.0.4664.45 (Official Build) (x86_64)
Revision 76e4c1bb2ab4671b8beba3444e61c0f17584b2fc-refs/branch-heads/4664@{#947}
OS macOS Version 11.6.1 (Build 20G224)

Steps:

  1. new profile
  2. launched Brave
  3. loaded brave://settings/braveSync/setup and chose Start a new Sync Chain
  4. copied code words into clipboard
  5. shut down Brave
  6. loaded brave://settings/braveSync/setup and confirmed my device was still on the Sync chain
  7. clicked on Leave Sync Chain
  8. clicked OK
  9. confirmed my device left the Sync chain
  10. loaded brave://settings/braveSync/setup and clicked on I have a Sync code
  11. entered the code words from step 4 and clicked Confirm
  12. confirmed via brave://settings/braveSync/setup my device is now again on the original chain created in step 3
example example example example example example example
Screen Shot 2021-11-15 at 9 46 29 AM Screen Shot 2021-11-15 at 9 48 28 AM Screen Shot 2021-11-15 at 9 49 16 AM Screen Shot 2021-11-15 at 9 49 27 AM Screen Shot 2021-11-15 at 9 49 35 AM Screen Shot 2021-11-15 at 9 50 15 AM Screen Shot 2021-11-15 at 9 50 45 AM

Verification PASSED on Win 11 x64 using the following build:

Brave | 1.32.103 Chromium: 96.0.4664.45 (Official Build) (64-bit)
-- | --
Revision | 76e4c1bb2ab4671b8beba3444e61c0f17584b2fc-refs/branch-heads/4664@{#947}
OS | Windows 11 Version 21H2 (Build 22000.318)
  • ensured that you can create a new sync chain via brave://settings/braveSync without any issues
  • ensured that you can add other devices into the same sync chain via QR Code & code words without any issues
  • ensured that the browser stays connected to the sync chain via brave://settings/braveSync after several browser restarts
  • ensured that that leaving the sync chain works without any issues and that the second device is still connected
  • ensured that re-joining the sync chain works without any issues
Initial Sync Chain 2nd Device on Sync Chain Leaving Sync Chain 2nd Device after Leaving Re-joining Re-joined Sync Chain 2nd Device after re-joining
image image image image image image image

@stephendonner stephendonner added QA Pass-macOS and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Nov 15, 2021
@kjozwiak
Copy link
Member

kjozwiak commented Nov 15, 2021

Verification PASSED on Samsun S10+ running Android 11 using the following build:

Brave | 1.32.103 Chromium: 96.0.4664.45
  • ensured that you can create a new sync chain via Settings -> Sync
  • ensured that you can add other devices into the same sync chain via both QR Code & code word
  • ensured that the browser stays connected to the sync chain after several browser restarts
  • ensured that that leaving the sync chain works without any issues and that the second device is still connected
  • ensured that re-joining the sync chain several times works without any issues
Initial Sync Chain 2nd Device on Sync Chain Leaving Sync Chain 2nd Device after Leaving Re-joining Re-joined Sync Chain 2nd Device after re-joining
Screenshot_20211115-155916_Brave Screenshot_20211115-155940_Brave Screenshot_20211115-160123_Brave image Screenshot_20211115-160333_Brave Screenshot_20211115-160554_Brave image

Verification passed on Brave v1.32.106 on Samsung Galaxy Tab S5e (Android 10.0)

Verified test plan from brave/brave-core#10960

  • ensured that you can create a new sync chain via Settings -> Sync
  • ensured that you can add other devices into the same sync chain via both QR Code & code word
  • ensured that the browser stays connected to the sync chain after several browser restarts
  • ensured that that leaving the sync chain works without any issues and that the second device is still connected
  • ensured that re-joining the sync chain several times works without any issues
Screenshot_20211116-185359_Brave Screenshot_20211116-185623_Brave Screenshot_20211116-185628_Brave Screenshot_20211116-191451_Brave Screenshot_20211116-191513_Brave Screenshot_20211116-191633_Brave Screenshot_20211116-191642_Brave

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

Successfully merging a pull request may close this issue.

6 participants