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.

Sign up for GitHub

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

Jump to bottom

Avoid the use of mojo_base.mojom.FilePath #41834

Closed
cdesouza-chromium opened this issue Oct 23, 2024 · 4 comments · Fixed by brave/brave-core#26170
Closed

Avoid the use of mojo_base.mojom.FilePath #41834

cdesouza-chromium opened this issue Oct 23, 2024 · 4 comments · Fixed by brave/brave-core#26170
Assignees
@cdesouza-chromium
Labels
dev-concern OS/Desktop QA/In-Progress Indicates that QA is currently in progress for that particular issue QA/Yes release-notes/exclude
Milestone
1.74.x - Beta

Comments

@cdesouza-chromium
Copy link
Contributor

Description

We should remove the use of mojo_base.mojom.FilePath from the codebase, as it allows for relative paths parent paths, which can be a source of vulnerability if the browser process is compromised.
@cdesouza-chromium cdesouza-chromium self-assigned this Oct 23, 2024
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 23, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
4e2a18c 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Additionally, this change removes the setting of executable bit when
extracting the tor binary, as this has been correct upstream since[1].

[1] https://crrev.com/c/3379289

Resolves brave/brave-browser#41834
@cdesouza-chromium cdesouza-chromium mentioned this issue Oct 23, 2024
Merged
24 tasks
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 24, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
277468a 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Additionally, this change removes the setting of executable bit when
extracting the tor binary, as this has been correct upstream since[1].

[1] https://crrev.com/c/3379289

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 24, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
24d2cca 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Additionally, this change removes the setting of executable bit when
extracting the tor binary, as this has been correct upstream since[1].

[1] https://crrev.com/c/3379289

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 24, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
a1c6336 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Additionally, this change removes the setting of executable bit when
extracting the tor binary, as this has been correct upstream since[1].

[1] https://crrev.com/c/3379289

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 24, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
31bf17d 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Additionally, this change removes the setting of executable bit when
extracting the tor binary, as this has been correct upstream since[1].

[1] https://crrev.com/c/3379289

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 24, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
059604b 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 24, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
29b9178 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 24, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
eb24bdc 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 25, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
159127e 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Resolves brave/brave-browser#41834
cdesouza-chromium added a commit to brave/brave-core that referenced this issue Oct 25, 2024
@cdesouza-chromium
[CodeHealth] Use SafeBaseName for TorConfig
50ff319 
The use of `base::FilePath` in `TorConfig` could be a serious source for
a vulnerability if someone got control of the browser process. This
change employs the use of `SafeBaseName`, which only allows for the base
component of a path. With this browser process and the tor launcher do
agree over a certain definition of how the files are discovered.

Resolves brave/brave-browser#41834
@brave-builds brave-builds added this to the 1.73.x - Nightly milestone Oct 25, 2024
@LaurenWags
Copy link
Member

@cdesouza-chromium since this one is QA/Yes it requires a test plan, could you please add one? If this label is incorrect and should be QA/No, you can change it and no test plan is needed.

In the mean time, adding QA/Blocked until this is sorted 👍🏻

cc @rebron @kjozwiak

@cdesouza-chromium
Copy link
Contributor Author

@LaurenWags The test is mainly to open a Private Window with Tor, and check that tor navigation, and features are working in general.

@LaurenWags
Copy link
Member

Thanks @cdesouza-chromium. We do a general regression test of Tor during our manual passes for all 3 desktop OSes, so we can just do a quick check here to make sure nothing obvious is broken 👍🏻

@MadhaviSeelam MadhaviSeelam added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Nov 22, 2024
@MadhaviSeelam
Copy link

MadhaviSeelam commented Nov 22, 2024 •
edited
Loading

Verification In progress using

Brave | 1.74.11 Chromium: 131.0.6778.85 (Official Build) beta (64-bit)
-- | --
Revision | c777fe11bc7afc00156e58af64a6a5bb9559da7d
OS | Windows 11 Version 24H2 (Build 26100.2314)
  1. Installed 1.74.11
  2. launched Brave
  3. clicked hamburger menu >> New private window with Tor
  4. visited https://check.torproject.org/ in a Tor window
  5. confirmed a success message for using a Tor exit node
  6. visited https://check.torproject.org/ in a Tor window and noted down exit node IP address
  7. did a hard refresh (Ctrl+Shift+R/Cmd+Shift+R)
  8. confirmed exit IP changed after page reloaded
  9. visited https://check.torproject.org/ in a Tor window and note down exit node IP address
  10. clicked New Tor connection for this site in app menu
  11. confirmed the exit node IP address changes after page is reloaded
  12. visited following sites in Tor window and confirmed all pages are resolved
  13. visited https://browserleaks.com/geo in a Tor window
  14. confirmed location isn't shown
  15. visited following Torrent viewer sites
  16. confirmed it doesn't load in a Tor window
  17. visited Ubuntu
  18. confirmed able to download a file in a Tor window.
  19. confirmed all Download/Cancel, Download/Retry and Download works in Tor window

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cdesouza-chromium cdesouza-chromium

Labels
dev-concern OS/Desktop QA/In-Progress Indicates that QA is currently in progress for that particular issue QA/Yes release-notes/exclude
Projects
None yet
Milestone
1.74.x - Beta
Development

Successfully merging a pull request may close this issue.

[CodeHealth] Use SafeBaseName for TorConfig brave/brave-core
4 participants
@LaurenWags @cdesouza-chromium @brave-builds @MadhaviSeelam