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

updates related to #1509 #5330

Draft
wants to merge 39 commits into
base: master
Choose a base branch
from
Draft

Conversation

rrrooommmaaa
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa commented Jul 26, 2023

Trying to fix #5373 (to be merged after PR #5372)


Tests (delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@rrrooommmaaa
Copy link
Contributor Author

@tomholub

These issues are encountered:

  1. broadcast messages sent with chrome.runtime.sendMessage don't reach content script's handler
  2. in Firefox, broadcast messages sent with chrome.runtime.sendMessage from the content script don't reach any frames (only background)

@tomholub
Copy link
Collaborator

deleted my comment. what if we used the insecure messaging everywhere, does it easily reach where we need it?

then we encrypt & sign, decrypt & verify.

@rrrooommmaaa
Copy link
Contributor Author

Somewhat easier, I think.
You said to take the symmetric key for encryption from the local storage.
There might be problems addressing local storage from the content script, not sure.
We have this comment:

if (Env.isContentScript()) {
// extension storage can be disallowed in rare cases for content scripts throwing 'Error: Access to extension API denied.'
// go through bg script to avoid such errors

Also, how should we keep session passphrases? Keeping them in the content script would mean that opening several gmail tabs would require entering passphrases into each of them independently, right?

@tomholub
Copy link
Collaborator

tomholub commented Jul 26, 2023

Also, how should we keep session passphrases? Keeping them in the content script would mean that opening several gmail tabs would require entering passphrases into each of them independently, right?

I'm ok with that limitation. We could then reintroduce a background worker later, if it turns out a problem.

// extension storage can be disallowed in rare cases for content scripts throwing 'Error: Access to extension API denied.'
// go through bg script to avoid such errors

Could have been fixed on recent browser versions? That's probably from 6 years ago. We could just try it and see what happens. Alternatively, we could try window.localStorage instead, in case it can be adequately accessed from everywhere. Worst case, each content script would make a worker, and access the extension storage through its worker.

@rrrooommmaaa
Copy link
Contributor Author

then we encrypt & sign, decrypt & verify.

You were previously talking about using a symmetric key. Can you elaborate on sign/verify part?

@tomholub
Copy link
Collaborator

I don't have anything that specific in mind, but surely window.crypto has some way to encrypt things in a manner that can be verified to be authentic when decrypting.

Roughly we could send something like {destination: [list of receiving frames], content: encryptedData}

Where the encryptedData also contains the destination. That way, receiving frames don't have to decrypt it to decide if they should drop the message or not, while at the same time they can still verify that the message and destination is authentic and matching once they do decrypt it.

Then the content would just be an encrypted/serialised json of whatever today is in these messages.

Just a rough idea.

@rrrooommmaaa
Copy link
Contributor Author

some API is not available for the content script. e.g. chrome.tabs, so some features aren't possible without the background page, e.g. to open a settings page from this notification
image
What should we do about it?

@tomholub
Copy link
Collaborator

tomholub commented Jul 30, 2023

some API is not available for the content script. e.g. chrome.tabs, so some features aren't possible without the background page, e.g. to open a settings page from this notification
image
What should we do about it?

We could just make it a regular a href target=_blank ? I forgot why we do this through background page.

@rrrooommmaaa
Copy link
Contributor Author

I forgot why we do this through background page.

To disallow opening settings tab multiple times?

@tomholub
Copy link
Collaborator

I forgot why we do this through background page.

To disallow opening settings tab multiple times?

Sounds not so important, ok to not worry about that

@rrrooommmaaa
Copy link
Contributor Author

We also use chrome.windows.create to create Oauth popup.
What should we do? Embed it in a frame?

@rrrooommmaaa
Copy link
Contributor Author

this mainly involves reconnectAcctAuthPopup process initiated by the content script

@tomholub
Copy link
Collaborator

We also use chrome.windows.create to create Oauth popup.
What should we do? Embed it in a frame?

this mainly involves reconnectAcctAuthPopup process initiated by the content script

This will not be easy, imo it warrants a separate PR. So I'd leave that as is for now, from background script.

@rrrooommmaaa
Copy link
Contributor Author

There is a problem using fetch() from the content script in Firefox: The page’s settings blocked the loading of a resource at inline (“script-src”) Any ideas of how to fix this?
Should I make fetch() calls from the background page in this PR? For Firefox only or both?
Switching to direct fetch() would simplify upload/download progress implementation, but will break Firefox.
In Chrome, fetch is allowed in the content script though with some peculiarities: after logging in to a mock google account, the browser submits google's access token to some other endpoints, e.g. ekm's /keys/private, meaning:
the mock endpoint expects an idtoken like Authorization: Bearer fakeheader... and passing this in headers of the fetch request is overridden by the browser, if it has already remembered google's accessToken. The mock endpoint thus receives Authorization: Bearer mock-access-token-... instead. Changing the url of mock ekm to https://ekm.localhost doesn't help.
In production there isn't such a problem as domains don't look similar there.
I wonder how we can wire mock endpoints and oauth in a better way. Or would it be acceptable to update the mock endpoint to accept idtoken or accesstoken?
@tomholub @sosnovsky

@sosnovsky
Copy link
Collaborator

Maybe then it'll be better to implement migration to native fetch in another PR?
So in this PR you'll implement fix for original issue #5329 and then we can find solution for this fetch migration (or just leave it for later).

It's possible to allow inline scripts using nonce or hashes - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#unsafe_inline_script, but not sure if it's possible in our case.

@rrrooommmaaa rrrooommmaaa changed the title use chrome.runtime broadcast sends with custom generated tabId replaced ajax calls with fetch() Aug 16, 2023
@rrrooommmaaa rrrooommmaaa changed the title replaced ajax calls with fetch() updates related to #1509 Aug 20, 2023
const encryptedMsg = await SymmetricMessageEncryption.encrypt(validMsg);
BrowserMsg.sendToChildren(encryptedMsg);
if (postToSelf) {
window.postMessage(encryptedMsg, '*');

Check warning

Code scanning / CodeQL

Cross-window communication with unrestricted target origin Medium

Sensitive data
is sent to another window without origin restriction.
@rrrooommmaaa rrrooommmaaa mentioned this pull request Aug 22, 2023
5 tasks
# Conflicts:
#	extension/js/background_page/bg-handlers.ts
#	extension/js/common/api/account-servers/external-service.ts
#	extension/js/common/api/authentication/google/google-oauth.ts
#	extension/js/common/api/email-provider/gmail/gmail.ts
#	extension/js/common/api/email-provider/gmail/google.ts
#	extension/js/common/api/shared/api.ts
#	extension/js/common/browser/browser-msg.ts
#	test/source/mock/attester/attester-endpoints.ts
#	test/source/mock/fes/customer-url-fes-endpoints.ts
#	test/source/mock/keys-openpgp-org/keys-openpgp-org-endpoints.ts
#	test/source/mock/lib/api.ts
# Conflicts:
#	extension/js/common/api/account-servers/external-service.ts
#	extension/js/common/browser/browser-msg.ts
# Conflicts:
#	extension/js/common/api/authentication/google/google-oauth.ts
#	extension/js/common/browser/browser-msg.ts
#	extension/js/common/platform/store/encryption-key-store.ts
#	extension/js/common/symmetric-message-encryption.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canceling passphrase dialog sometimes leads to glitches
3 participants