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

SAVE_INIT_DATA should pass seed when recovering from sync words #221

Closed
darkdh opened this issue Oct 10, 2018 · 6 comments
Closed

SAVE_INIT_DATA should pass seed when recovering from sync words #221

darkdh opened this issue Oct 10, 2018 · 6 comments
Assignees

Comments

@darkdh
Copy link
Member

@darkdh darkdh commented Oct 10, 2018

  1. Create a new sync device:
    webview->{GET_INIT_DATA}->deviceA->{GOT_INIT_DATA: {seed: null}}->webview->{SAVE_INIT_DATA: {seed: seed_ABCD}}

  2. Add a new sync device:

  3. deviceA generate sync words from seed_ABCD

  4. Paste sync words into device B for recovery
    webview->{GET_INIT_DATA}->deviceB->{GOT_INIT_DATA: {seed: seed_ABCD}}->webview->{SAVE_INIT_DATA: {seed: null}}

  5. webview should NOT give us null seed

@darkdh darkdh self-assigned this Oct 10, 2018
darkdh added a commit that referenced this issue Oct 10, 2018
fix #221
@diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Oct 10, 2018

the seed doesn't actually get sent to the server, btw (otherwise the server would be able to decrypt records). it is only sent between the sync client and the sync webview process.

@darkdh
Copy link
Member Author

@darkdh darkdh commented Oct 10, 2018

gotcha, I updated the issue description

@diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Oct 10, 2018

@darkdh the reason seed is empty in the 2nd case is because Device B already has the seed (it is sent to the sync webview in GOT_INIT_DATA: {seed: seed_ABCD}). According to the specification in client/constants/messages.js, browser must save values in persistent storage if non-empty. We were assuming that Device B saved the seed already, so there is no need to save it again.

does this cause problems?

@diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Oct 10, 2018

the impact of https://github.com/brave/sync/pull/222/files#diff-6ce8437e6c27185736d22013ce441e15R71 is that clients like browser-laptop would end up saving the seed twice

@darkdh
Copy link
Member Author

@darkdh darkdh commented Oct 10, 2018

yes, we have problem saving those value in brave-core after dynamic loading sync extension
brave/brave-core@0571802
we can't get seed from words from sync library because we still waiting for extension loaded and initialized. And once it is loaded and initialized we receive GET_INIT_DATA, we don't have way to get seed from words because the call to extension is async so we have no way to reply correct seed even we have sync words.
And we have to do workaround like this
brave/brave-core@6c83036
but this doesn't use js library to get seed.

So the solution from brave-core for this would be caching sync words and calling to our extension handler of sending GOT_INIT_DATA and do passphrase.toBytes32 in it.
That is why we rely on SAVE_INIT_DATA.

cc @AlexeyBarabash

@diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Oct 10, 2018

ok, i'm not 100% sure but i think that wouldn't cause a problem for any of the existing implementations

darkdh added a commit that referenced this issue Oct 10, 2018
fix #221
darkdh added a commit that referenced this issue Oct 10, 2018
fix #221
@darkdh darkdh closed this in #222 Oct 11, 2018
darkdh added a commit that referenced this issue Nov 20, 2018
fix #221
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.