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

Remove local file permission #511

Merged
merged 1 commit into from
Aug 7, 2020
Merged

Remove local file permission #511

merged 1 commit into from
Aug 7, 2020

Conversation

Sneezry
Copy link
Member

@Sneezry Sneezry commented Jul 19, 2020

We added local file permission (file://*/*) for #171, and merged the change with #173

This permission is required for some customer use cases (see also #400). From this answer on StackOverflow, the local file permission must be approved by users manually.

However, recently we got an issue to report a possible security issue of local file permission. Because Tabs permission could be granted by default:

You can use most chrome.tabs methods and events without declaring any permissions in the extension's manifest file.

https://developer.chrome.com/extensions/tabs

Extensions with local file permission are able to traverse local files by openning local file urls (for example file:///C:/) in new tabs, and read contents with active tab.

This is a serious security issue, and we need to discard file://*/* permission ASAP no matter it is disabled by default or not.

For importing data with local image files, we could add a new file input controller to make users be able to upload selected file to the extension as what a regular website does.

@Sneezry Sneezry requested a review from mymindstorm July 19, 2020 18:05
@mymindstorm
Copy link
Member

mymindstorm commented Jul 20, 2020

I don't see this as a major security issue with Authenticator, we are using the file://*/* permission as intended. If this is an issue, we should file a bug with Chromium.

Extensions with local file permission are able to traverse local files by openning local file urls (for example file:///C:/) in new tabs, and read contents with active tab.

This is a serious security issue, and we need to discard file:/// permission ASAP no matter it is disabled by default or not.

We obviously don't do this, and it would be hard to do file traversal due to activeTab.

  • We can only inject a script directly after user interaction with the extension in Chrome (I.e. one click = one attempt at a file). Docs
  • We can't list directory contents, so we would have to know the exact path of the file we would want to read.
  • We can't extract data using a content script from files in an unknown or binary format like password databases as they are instantly downloaded and no page is loaded.
  • This would not work in firefox as you cannot open a file path from an extension
  • The user still has to enable "Allow access to file URLs" manually. That permission is not granted by default

@mymindstorm
Copy link
Member

You are right, I forgot to take the user's perspective into consideration. A file picker would be better.

@Sneezry
Copy link
Member Author

Sneezry commented Aug 5, 2020

Yeah, a file picker makes more sense. And because Google official authenticator mobile app supports to export data with QR code in recent version, this feature could be our high priority work item.

@mymindstorm
Copy link
Member

Screenshotting the code is blocked. Opening QR code on phone => taking a picture using webcam => importing codes is a bit of a convoluted process, we could also add an import via webcam page.

@Sneezry
Copy link
Member Author

Sneezry commented Aug 7, 2020

Merge this PR because we are able to import image files with #521 and local file permission is no longer needed.

@Sneezry Sneezry merged commit 9d1999d into dev Aug 7, 2020
@Sneezry Sneezry deleted the local-file-permission branch August 7, 2020 07:04
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.

2 participants