brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.8k stars 2.32k forks source link

Windows should not install VPN services until VPN is purchased/enabled #33726

Closed bsclifton closed 9 months ago

bsclifton commented 1 year ago

Background

Starting with product version 1.59.117 on Windows, WireGuard is used as the default for Brave VPN.

With product version 1.57.47, Brave will install a service Brave Vpn Wireguard Service if a user has admin privileges. This service is marked as Manual start and is not started. The binary is also installed on disk in the directory the the browser binaries are installed.

This change was introduced here: https://github.com/brave/brave-core/pull/18565

The pull request links to the devops issue where we compile the binary and also to the privacy/security review where this was vetted. Originally, this work was all behind a feature flag exposed via brave://flags.

There is also a Brave Vpn Service that is installed (also set to Manual start, not started) which has been there for a longer time. This service was added here: https://github.com/brave/brave-core/pull/15915

That change went live with Brave product version 1.50.114 on Windows. This service was added to provide an OS level way to stop leaking of DNS due to a Windows feature called Smart Multi-Homed Name Resolution and is only used when a customer has purchased VPN and the VPN is connected. More information about Smart Multi-Homed Name Resolution and why this service was created can be found here: https://github.com/brave/brave-browser/issues/25489

Here is a picture from services.msc courtesy of ghacks.net image

These services will only be used when the person buys Brave VPN (via account.brave.com) and engages with the UI in the product.

Description

On Windows only, there are two VPN related services (Brave Vpn Service and Brave Vpn Wireguard Service) registered with Windows when Brave is freshly installed. They can be viewed in services.msc. They are both set to Manual start and are not used until a person 1) uses Brave and 2) purchases Brave VPN and then 3) connects to Brave VPN.

At that point, a config (with the VPN details) is written to disk and the service is started.

These services are installed at install time - since the installer is already doing a UAC prompt (admin escalation). The ideal situation would be to move these services to be installed when VPN is first USED (post purchase) and not at install time.

What does the fix look like

As we solve this issue, here's what we plan to do

sirmrgentleman commented 1 year ago

Coming from #33592 which was closed as a duplicate of this. I also had a similarly named task appear in Task Manager as a startup application, enabled by default. I have since disabled it due to not needing it. The full filepath is C:\Program Files\BraveSoftware\Brave-Browser\Application\118.1.59.117\BraveVpnWireguardService which differs from the service listed in services.msc(Brave Vpn Wireguard service) which has a path of C:\Program Files\BraveSoftware\Brave-Browser\Application\118.1.59.117\BraveVpnWireguardService\brave_vpn_wireguard_service.exe image

joejoejo commented 1 year ago

do you guys know when the beta will get this fix so my computer does not have the brave vpn anymore

joshwenke commented 1 year ago

It is terribly disappointing that a security-branded browser is doing something blatantly insecure. At least you are rolling back the change... but how did this get approved in the first place? I sing Brave's praises to family and friends, please don't give me a reason to change that.

Marko-98 commented 1 year ago

Now I wonder if they do that on Android. Because inside the Brave Browser, there is an entry in the browser menu Brave VPN, to which when touched, part is open where it asks for subscription.

bsclifton commented 1 year ago

@Marko-98 no, there is no service on Android.

@joshwenke I can understand the concerns by folks about bloat (ex: services or files put in place which will never be used) - but I'm a bit puzzled at how adding a service equates to Brave being insecure? We wrote the code ourselves and applied our same review process to it. The services are off by default... but even if turned on, there's no action taken by the service unless you 1) have purchased VPN and 2) are connecting. Our security team has worked to review the code thoroughly to make sure having these doesn't increase the attack surface for Brave.

Code is included in Brave for features even if people don't use them. For example, if you choose not to use vertical tabs, the code for vertical tabs is still there.

We're actively making changes so that no services will be registered / no dependencies downloaded until you purchase Brave VPN and turn it on.

Thanks for your patience. When I have more information (link to pull request, link to code branch) I'll share here

Dskobra commented 1 year ago

Code is included in Brave for features even if people don't use them. For example, if you choose not to use vertical tabs, the code for vertical tabs is still there.

You're joking right? Vertical tabs doesn't install a windows service without telling people... For a privacy based browser you guys certainly have a blatant disregard for people's privacy and consent. How many more fiasco's will you guys continue to have before you knock it off or people completely lose trust? Don't think some of us haven't forgotten about the whole referral code issue a couple years ago. Personally my patience is growing thin.

We're actively making changes so that no services will be registered / no dependencies downloaded until you purchase Brave VPN and turn it on.

This should have been done at the start. There's zero excuses.

Thanks for your patience. When I have more information (link to pull request, link to code branch) I'll share here

I'm expecting you guys to do better and put clear policies in place to avoid these types of things. Privacy and consent need to be at the front and center of Brave itself. Controversial features like this should always have an opt-in or at the very least a way to disable/uninstall it from the start.

bsclifton commented 1 year ago

OK folks - have an update. I'm doing some experimenting and we should be able to have a nice and clean solution soon.

We'll have to iron out some details about the experience (talking with the team on that), but you can see my prototype branch here: https://github.com/brave/brave-core/compare/bsc-brave-vpn-client-component-updater

I'll continue to work through this and will share updates as they happen.

@Dskobra fair point on setting a boundary on installing a service. We have a privacy policy here: https://brave.com/privacy/browser/

I reviewed and we do have VPN captured in the privacy policy - but these recent changes on Windows (installation of service) are not covered there. I've already pinged the appropriate folks to get that updated. And we can revisit once the planned changes are completed and released.

We have already made changes to processes within the company to help prevent future issues from happening. This is a learning experience and I'm trying to be as transparent as possible. Thanks

Dskobra commented 1 year ago

@Dskobra fair point on setting a boundary on installing a service. We have a privacy policy here: https://brave.com/privacy/browser/

I reviewed and we do have VPN captured in the privacy policy - but these recent changes on Windows (installation of service) are not covered there. I've already pinged the appropriate folks to get that updated. And we can revisit once the planned changes are completed and released.

We have already made changes to processes within the company to help prevent future issues from happening. This is a learning experience and I'm trying to be as transparent as possible. Thanks

I appreciate you working towards a solution and trying to be as transparent as possible. However, this situation was definitely not transparent. Personally I was more unnerved to find a system tray icon and auto start entry for something I never used. I don't know if this was rushed or what happened, but I think a way to disable the feature and remove it should have been included when being pushed to the user.

Anyway hopefully this is a lesson learned and I don't see any future surprises.

bsdinis1 commented 1 year ago

having a browser that auto-updates itself in the background, and in that process it installs an application as a Windows service that autoruns at startup... yea... I don't see any problem at all

bsdinis1 commented 1 year ago

and it even re-enabled the Services on its own... congratulations

bsclifton commented 1 year ago

@bsdinis1 I deleted your first comment above because it's not adding anything and was abusive. Let's keep things constructive and respectful here please.

Your second comment above captured the problem. Services were re-registered after Brave updated.

Until we solve this issue (which we are actively working on), the services will fix themselves on upgrade if you installed as Administrator. That is due to how Omaha and the mini-installer work. All of the logic for the mini-installer will run on each update.

We're working through a few different proposals for how to solve this entire issue. As shared, there have been some prototypes happening. Will update once we have more to share.

ghost commented 1 year ago

Like I said in my other comment, in the 'old issue' that was closed as being a Duplicated of this, If any of you don't want to see 'VPN installed', in any new update, which is obvious it is going to do that, then uninstall Brave and Install it WITHOUT admin rights.

There is NO reason why 99% of people have to install a browser, unless you share your computer with someone, and you want all users to have the most up-to-date version and don't re-use disk space, then Per-User installation should be what users do.

The problem is doing a Per-User install is not obvious, but it is easy. All you need to do is to say no when it asks for admin rights, then you will get asked if you want to install it without them.

So first, since you already installed Brave with admin rights is to Uninstall Brave and wait, go to Task Scheduler or taskschd.msc and make sure the two tasks are gone and the updater is uninstalled in program files (x86), if not, it will create a mess because you will have two different updaters if you rush and install Brave without admin rights.

Then download the installer from https://github.com/brave/brave-browser/releases/tag/v1.60.104 or whatever version you previously have, search for BraveBrowserStandaloneSetup.exe or BraveBrowserStandaloneSilentSetup.exe.

Silent will install automatically without admin rights, so it might be better, although you won't get any confirmation or anything or nothing when it is finished installing, you might see Brave gets added to desktop and taskbar but that's it. normal Standalone version asks for admin rights, so just say no to get the "do you want to still install without admin rights?"

You can also use Winget, which uses the silent installer.

winget install -e --id Brave.Brave in Terminal that will install Stable version. (you can get the others here

benefits of user Per-User install? no services installed, including updater.

The updater will run as a startup process which can be easily disabled if you desire. Task Scheduler tasks are still added, which you can also disable if you desire for the people who 'don't want automatic updates'.

It is simple as that, if people want to avoid services to be installed, then don't give admin rights. if you installed Brave with admin rights, you already allowed Brave to install VPN.

Brave will fix this someday eventually, but people can help but stop complaining when they can easily 'fix it' by doing what they had to do from the beginning, Per-User installs, because I am sure you are not sharing your device with other users, that have their own Windows accounts, so giving admin rights to a browser is just nonsense even if it is the Default behavior.

bridiver commented 1 year ago

There is NO reason why 99% of people have to install a browser, unless you share your computer with someone, and you want all users to have the most up-to-date version and don't re-use disk space, then Per-User installation should be what users do.

@Emi-HoloGhostRevisionist88 this is actually not necessarily true if you use vpn and want the best privacy protection on Windows. System-wide protection against the Windows multi-homed DNS leak https://support.brave.com/hc/en-us/articles/11973307463181-What-is-the-Brave-VPN-and-the-Windows-Smart-Multi-Homed-Name-Resolution-Feature- require admin to temporarily set firewall rules that prevent DNS resolution on the non-vpn interface (the source of the dns leak).

Without admin, only Brave itself is protected by using DoH instead of the system DNS resolver. Brave browser does not run with elevated priviledges, but a small helper service that is only enabled with Brave VPN does run with the necessary priviledges and it automatically enables the firewall rules whenever the VPN is running. It runs as a background service because the vpn itself can continue to run even if Brave is closed and we want to make sure the firewall rules are always removed when the vpn is not connected.

However, this is only a problem if the dns server for the local interface is on the same network. Many routers proxy dns requests and set themselves as the DNS server in DHCP.

bridiver commented 1 year ago

Personally I was more unnerved to find a system tray icon and auto start entry for something I never used. I don't know if this was rushed or what happened, but I think a way to disable the feature and remove it should have been included when being pushed to the user.

@Dskobra this is definitely a bug and should not have happened. Something that is actually running by default is definitely in a completely different category from registering services that are not running by default. We will address both issues, but the tray icon running without enabling the vpn is not intended behavior afaik and if it was, that was a judgment error that will be corrected.

bridiver commented 1 year ago

We're actively making changes so that no services will be registered / no dependencies downloaded until you purchase Brave VPN and turn it on.

This should have been done at the start. There's zero excuses.

@Dskobra Brave is run by people and people make mistakes and sometimes they only register as mistakes in hindsight. If people feel like Brave is trying to do something sneaky then they should just use a different browser. If I thought Brave was trying to do something sneaky, I would find a different job.

I see some people calling this bloatware, but I have a hard time seeing how a very good privacy feature that is integrated into a privacy focused browser qualifies as bloatware. Should the services have been installed by default even if they are not enabled by deafult? No. Should the tray icon be enabled by default? Definitely not (this is a bug). Should we have shipped the actual binaries separately from Brave? Personally I don't see a problem with shipping code that only runs when enabled. As @bsclifton said, we ship all kinds of code that not all users enable.

Maybe some explanation would help. The primary reason for registering the services by default in the "manual" state (and by extension shipping the binaries for those services) is that some functions (like the fix for the windows mutlti-homed DNS leak mentioned in a post above) require admin to run and the browser does not run with admin privileges. The updater can run with admin privileges because that is necessary to update a system install of Brave so we used the updater to register the services, but did not set them to start automatically. If you are an admin user, you could say this is not necessary because we can prompt for admin when needed and you can accept or deny that. However, if you are a regular user with a system install of Brave you have no way to allow this and you would not get the benefits of things like system-wide protection from the windows multi-homed DNS leak. Was this the right trade-off? In retrospect no because we don't want to upset our users or make them think we're doing anything sneaky. We hear that you feel this was not the right decision and we're going to change it. I can't guarantee that we're never going to make a decision you don't agree with again (or make a mistake), but I can tell you that we will never intentionally do that or try to be sneaky about it, at least not as long as I'm working here because I would quit if we started doing that.

ghost commented 1 year ago

@bridiver Well, my post was aimed at people who 'don't want to use VPN' and don't want to get it reinstalled on every update until this is fixed, some people will delete the service through terminal which is the worst possible workaround, so if they are going to keep doing that, and follow bad practices, I think it is better if they just switch to Per-user installation and forget about this.

The whole admin vs non-admin rights is a big issue in software in Windows, like Windows giving users full admin rights to anyone was always problematic, this is why malware and virus get installed, so while it is necessary for some stuff, it should also be planned better. So installing per-user creates less security issues, but you are right, sometimes is a most to have a service.

But talking about Brave VPN, for example, since I never got VPN installed, I never noticed VPN installing like this, so I never reported and raised the concern about it, I wonder why other Nightly users who install with admin rights didn't report it.... but speaking about me, I have reported 3000 million other issues since I am Brave user, so I would have reported this as well.

Now since I never got installed VPN, this raise the question, what about per-user installed Brave users? people who used Winget or the silentinstaller? if per-user Brave users want to use VPN, do they have to install Brave with admin rights to get full protection? or are you going to implement a way so users can get the same best privacy protection on Windows or it is not possible? like a little button people can push to install the VPN as service or something with information about it and benefits?

The good thing is most people already just give admin rights when installing Chromium browsers already, so this situation will not apply to most people, but I think it has to be done in a way benefits all brave users, like, even if I didn't install Brave with admin rights, I should be able to get VPN installed as a service to make it work better, but also, even if I installed with admin rights, I should be able to run the VPN only while the browser is running, not just running 24/7 if people desire.

The option of 'system-wide all the time' is nice, but some people don't want or need VPN to run as service 24/7.

But anyway, this 'switch to per-user installation' guide for anyone who cares, was for people who aren't going to get VPN ever anyway, you know, if it is not free, they will not pay for it therefore it is useless for them, like you say, some people will wrongly call 'bloat' to anything, and sometimes people gotta do something about it if they can If people aren't patient until your fix, then, like in this case, they can use Brave without admin rights or just switch browsers or whatever they want to do, but this is about if people don't want to bother with automatic updates, with VPN, and all, services are going to always be an obstacle that can be avoided if they desire.

bridiver commented 1 year ago

Well, my post was aimed at people who 'don't want to use VPN'

That's fair and is generally good advice in most situations, I just wanted to point out that we're using admin specifically to increase user privacy in this case.

bridiver commented 1 year ago

and it even re-enabled the Services on its own... congratulations

The current code will re-register the services on update (we are changing this), but they are registered as manual startup and do not run unless the vpn is enabled.

This is not a straightforward problem as I described above. In particular the vpn helper service provides very important privacy protections to work around a bug in Windows (which affects all VPNs, not just Brave) that would otherwise leak your DNS requests. Those protections require admin privilege to enable firewall rules to block dns requests on the local interface and therefore cannot be enabled by Brave directly since it does not run with admin privilege. Because Brave is a privacy focused browser, protecting people who do use Brave VPN from this windows bug is a critical issue. I think we have found a way to continue to protect vpn users without registering the services using the installer/updater (which does run with admin), but it's not a trivial change are we are not going to leave vpn users unprotected while we work on it.

If you don't like this and don't want it to be reinstalled on updates (pending the fix) then you can follow @Emi-HoloGhostRevisionist88's suggestion and re-install the browser without admin. If you deny the UAC prompt, the browser will install without admin privileges and will be unable to install the services that require admin. No other functionality will be affected and it will be installed for your user account only instead of a system-wide install. When uninstalling Brave, make sure to leave "Also delete your browsing data?" unchecked or you will lose all your preferences, history, bookmarks, etc... I understand that it's not obvious that denying the UAC prompt will result in a user install and we will look at making it more explicit by asking if you want user or system before requesting the UAC prompt https://github.com/brave/brave-browser/issues/33968

LilRedDog commented 1 year ago

I'm just here because I was noticing a lot of behind my back internet traffic and when I looked into the metered service I found over 1.5 GB of traffic in the past 30 days with three different executables all named wireguardservice.exe

That is a lot of traffic for something in the background when Brave, all by itself, has only used 17.4 GB in the past 30 days.

bridiver commented 1 year ago

I'm just here because I was noticing a lot of behind my back internet traffic and when I looked into the metered service I found over 1.5 GB of traffic in the past 30 days with three different executables all named wireguardservice.exe

That is a lot of traffic for something in the background when Brave, all by itself, has only used 17.4 GB in the past 30 days.

Brave does not contain an executable called wireguardservice.exe and none of the wireguard code or the brave vpn services in general run unless you have enabled vpn. Perhaps you have something else using wireguard? What is the full path of the exe?

Brave itself and all of the code it includes is open source so it's literally impossible for us to hide anything. @bsclifton even provided links to the relevant code.

LilRedDog commented 1 year ago

Well, crap! I thought I had figured out what was causing all the background activity.

Thanks!

I do have a VPN that has wireguard as one of its protocols and, most of the time, I let it pick whatever it wants.

Sorry about that :(

DuckDuckGo sent me here as a hit.

ghost commented 1 year ago

Well, my post was aimed at people who 'don't want to use VPN'

That's fair and is generally good advice in most situations, I just wanted to point out that we're using admin specifically to increase user privacy in this case.

Oh I know, I just wanted to make it clear, I really don't have an issue with VPN I have used per-user installation for years and that's why In ever noticed VPN installing like that, so I never reported it, so if people want to avoid things with updates, then this is the only real workarounds as you explained above.

But what about my questions, will Brave allow per-user installations to get the VPN installed as service to be run with full protections and all that? and will admin users get not to run VPN 24/7 and while the browser is closed? In other words, will Brave have more control or will it stay like: Per-user = less secure, admin = services and more secure forever? I understand the limitations of being a browser and executing an external exe like that, so you are the only one to ask that here.

codetheron commented 1 year ago

If you don't like this and don't want it to be reinstalled on updates (pending the fix) then you can follow @Emi-HoloGhostRevisionist88's suggestion and re-install the browser without admin.

Ok, this is an workaround but I would like to install executable files in "Program Files". I don't like applications that are installed in one-click in some AppData folder. I know that this is convenient, works on managed computers (without admin privileges) and so on. Still, it's a pity that Microsoft hasn't (to my knowledge) designed a default location for such a executables.

So, please fix this situation and be more aware in future as it's very reputation damaging for 'privacy focused browser'.

ghost commented 1 year ago

@Un1Happy I already gave the workaround, use it or leave it, you can also remove the service easily. Development takes time. Are you this angry only because Brave installs 3 services for being art of the Omaha update? what's the difference? in fact, the only difference is VPN can easily be disabled and not ran, but the Brave update like Edge or Chrome, is running all the time. Why? again, because you gave admin rights to a browser, if you didn't, you wouldn't have this issues.

@codetheron why should Microsoft dictate where to install software? Program Files was always a bad idea, but it has a purpose, and per-user installations have another purpose, a well designed program installs per-user installations in %localappdata%\programs and then in the end it doesn't matter because what matters are shorcuts and same with start menu, all users or per user. You are not gaining anything by having all your programs in program files.

It's like people who complain about Registry, when Registry is okay, only because some developers do a terrible job, doesn't mean the things in place are bad.

For example powertoys run will only use indexed locations for finding programs, so it doesn't matter the folder as long as it is indexed. So putting portable programs in Downloads can make things easier, but it doesn't stop you from adding environmental variables or indexing or shortcuts in start menu/programs or whatever.

And I am a Nightly user, so I will blame Nightly users who install with admin rights because they could have reported the issue and they didn't. Which was leaked to Stable builds, like many other issues, like how Shortcuts weren't working for mac users unless they had a US layout keyboard, or how Dark mode broke in Linux for like a month. You can blame Brave team all you want, but there is a reason why it is open source and has a github and Nightly builds, so whatever developers forget or don't see, people can see it and report it if necessary.

This is still not a privacy issue, so I don't get why people start bringing that up, unless you can prove otherwise. Services has nothing to do with privacy, having updater installing 3 services vs none, doesn't change anything. You can call it an 'annoyance' for someone who doesn't want and is not interested in VPN, but that is still NOT a privacy issue, and the only ones who try to make this problem bigger are the same ones who still today mention how 3 years ago, Brave was injecting referral codes in every single URL typed by users and how Brave is not privacy browser for having automatic updates. Unless you can prove VPN is affecting privacy, then, people should drop the excuse to be angry at Brave team for VPN installing. Again, why didn't Nightly users reported it? I didn't because I didn't see it, what's the excuse of other people who saw VPN getting installed and said nothing? Brave team needs people testing and people reporting as well, they don't have eyes everywhere, why aren't you running Nightly and making sure this doesn't happen? well, that's because 'crap happens', so while we can blame everyone, even Nightly users, the truth is stuff will always happen, no matter what. But the more testers the better, but I know sometimes Nightly users don't report bugs and problems as they should, and they just want to use the 'latest' features.

bridiver commented 1 year ago

This is a github repo issue and it exists to discuss changes to the code, it is not a forum to express your personal opinions about Brave. If you want to have a discussion about how you feel about Brave or VPN there are plenty of other places you can do it, but this is not it. Any future comments that do not directly relate to this change will be deleted.

This issue was opened to change the thing that many people commenting on this issue are complaining about. It's not clear to me what purpose that could possibly serve other than distracting the very people who are trying to change it.

bridiver commented 1 year ago

But what about my questions, will Brave allow per-user installations to get the VPN installed as service to be run with full protections and all that? and will admin users get not to run VPN 24/7 and while the browser is closed?

@Emi-HoloGhostRevisionist88 Although it's not obvious, Brave already allows a per-user install. If you uninstall Brave (uncheck Also delete my browsing data) and then reinstall and deny the UAC prompt, it will do a user install and no services will be registered. The services that do get registered for admin installs never run unless you enable the VPN. The only thing that might possibly run is the vpn tray icon that was reported by @Dskobra, but we have had trouble replicating that. If it does run, hiding it actually creates a registry entry that prevents it from running again.

I have already created an issue to make it more obvious that there is a way to do a per-user install https://github.com/brave/brave-browser/issues/33968

ghost commented 1 year ago

@bridiver I have been the one telling people since VPN issue happened to install per-user install, because that's why I never noticed VPN.

That's not my question.

Let me re-make my question.

Brave install two VPN services

  1. Brave VPN Wireguard Service which is in charge of the VPN, it is the one that 'can survive restarts' that mean, if I want to use VPN 24/7, I have to use that service and make sure it starts on Windows start.
  2. Brave VPN service which deals with the DNS leak with Smart Multi-Homed Name Resolution.

Per-User don't install those services because there was no admin rights on installation. So VPN runs differently, it can't run while Brave is closed, it can't survive restarts and you don't get the Windows DNS leak fix, (correct me if I am wrong), VPN runs only while Brave is running.

Is Brave going to provide a way for per-user installations to get VPN services to be installed as services = have a button or toggle or something, where people are asked for admin rights to install the services? or do people have to reinstall Brave with admin rights to Brave to get these benefits?

Dskobra commented 1 year ago

@Dskobra, but we have had trouble replicating that. If it does run, hiding it actually creates a registry entry that prevents it from running again.

@bridiver It's possible something I did might have triggered it. One of the first things I did when i saw the service is went through the brave://flags to see if there was some hidden option to remove the service. I ended up turning off "Enable experimental Brave VPN" and "Enable DoH for Brave VPN" then back on after seeing no change. Shortly after a restart I noticed the tray icon which I then disabled the autostart entry in Windows startup settings. brave brave 2

bridiver commented 1 year ago

Is Brave going to provide a way for per-user installations to get VPN services to be installed as services = have a button or toggle or something, where people are asked for admin rights to install the services? or do people have to reinstall Brave with admin rights to Brave to get these benefits?

We don't currently have any plans to enable this for user installs. Our focus right now is finding a way to register the services only when vpn is enabled, but retain the admin rights required for some operations. While it would technically be possible to prompt for admin on a user install, there's also a concern that while some people view the prompt as a feature because they want to provide explicit permission, other people will view it as confusing or annoying. Also the vast majority of user installs likely do not have admin rights in the first place. I wouldn't say it's off the table and feel free to open up a new issue for it, but our primary focus right now is this issue.

Dskobra commented 1 year ago

@bridiver I cant seem to reproduce the tray icon now. Swear there was a black icon that showed up that said brave vpn. Oh well it doesn't show anymore so guess that weird issue is resolved. O.o

codetheron commented 1 year ago

While it would technically be possible to prompt for admin on a user install, there's also a concern that while some people view the prompt as a feature because they want to provide explicit permission, other people will view it as confusing or annoying.

Definitely - if installer asks me for admin rights (UAC) - as for me it does require it. A lot of other software have option in installer to choose per system or per user install, thus...

I have already created an issue to make it more obvious that there is a way to do a per-user install https://github.com/brave/brave-browser/issues/33968

.., this is great idea :)

Back on the topic. I have this installation of Brave for about 9 months, after that only auto-updated it. It's installed in Program Files on Windows 10 Pro x64, current x64 version of Brave - 1.59.124. I also have ProtonVPN (not in autostart) - using it very rarely but still. An in this config I have:

Good luck with fixing it @bridiver πŸ‘

levicki commented 1 year ago

@bsclifton

These services will only be used when the person buys Brave VPN (via account.brave.com) and engages with the UI in the product.

Even if we ignore for the moment that the VPN was added to upsell the subscription and not just to provide additional privacy, these particular services should have never been installed without the users' informed consent during setup.

Privacy without the consent is just an illusion of privacy, because it is based on trust ("trust us to do no wrong") instead on transparency ("here's what we are doing, why we are doing it, and how we are doing it").

Our security team has worked to review the code thoroughly to make sure having these doesn't increase the attack surface for Brave.

This isn't about increasing the attack surface of your browser because your browser is not running all the time — this is about increasing an attack surface of the operating system by adding services running under NT AUTHORITY\SYSTEM account that malware can potentially utilize for escalation of privileges if there are exploitable bugs in the service code.

Code is included in Brave for features even if people don't use them. For example, if you choose not to use vertical tabs, the code for vertical tabs is still there.

This is really a disingenuous argument and you should be ashamed for making it.

You are comparing code in the browser which for most users is running without Administrator privileges with the code in a Windows service which is running under NT AUTHORITY\SYSTEM account which has SE_TCB_NAME privilege allowing it to act as a part of the operating system itself (i.e. has more rights than the Administrator account).

@bridiver

With that out of the way, I would like to propose a significant change:

If you are going to offer WireGuard VPN subscription, then at least do so by using official applications for the respective platforms, instead of rolling your own support for it into the browser. That would be beneficial for several reasons:

  1. Users will be able to opt-out of installing it
  2. Users will trust official WireGuard implementation more than yours
  3. If users already have official WireGuard app installed, you just have to offer new connection profile

Current implementation totally disregards the possibility that users might already have a VPN and doesn't play nice with official solution.

In the meantime, for the users who would like to remove the services, here's the workaround using admin command prompt:

sc delete BraveVpnService
sc delete BraveVpnWireguardService

This will remove the services immediately if they are not started, otherwise they will disappear after a reboot.

bridiver commented 1 year ago

Code is included in Brave for features even if people don't use them. For example, if you choose not to use vertical tabs, the code for vertical tabs is still there.

This is really a disingenuous argument and you should be ashamed for making it.

It is not disingenuous in the slightest bit and I am not even remotely ashamed to be making it. We consider the VPN to be a feature of Brave. Period. You make not like this, but it is our decision to make. There are also important reasons for this that I explain below. I acknowledge that it's not ideal that the services are registered (but do not run) by default and I explained the reasons for it. We are looking into alternatives that will still allow us to provide the important privacy protections for people using the VPN (like blocking the windows DNS leak) without registering them by default using the installer/updater. Windows is the only platform where we do this because it's not an issue on other operating systems.

You are comparing code in the browser which for most users is running without Administrator privileges with the code in a Windows service which is running under NT AUTHORITY\SYSTEM account which has SE_TCB_NAME privilege allowing it to act as a part of the operating system itself (i.e. has more rights than the Administrator account).

The updater also runs with admin and so does the elevation service that is also part of Chrome and other chromium based browsers. No one ever gave individual explicit permissions for these services to be installed either and they actually run by default. Those have been there for a very long time and no one has complained about them as far as I know. I already explained that this is not a trivial problem to solve, particularly for users who have a system install, but are not admin users themselves.

As I said above, further comments that do not directly relate to this issue will be deleted as this is not the appropriate forum to discuss this in. I'm going to leave this one only because below it does propose some changes that are actually relevant here, but pease do not push this again. You are free to comment on solutions to this issue, but the github issue is the place to discuss code changes, not your opinions on decisions or statements made by Brave.

People seem to frequently comment on this with only their particular situation in mind and appear to give little to no thought to the idea that we have a wide range of users with different levels of technical expertise and there are always tradeoffs to make. If we made Brave just for you, we could do things exactly the way you think they should be done and the choices would be simple and straightforward, but we are trying to make Brave for a lot of different types of people and that means we have to make tradeoffs. Every new prompt we show will scare/annoy some users off and they will stop using Brave. Trying to make this decision sound obviously wrong fails to consider that we are trying to balance a lot of different concerns here. If you don't trust that we are trying to do the right thing and find the right balance, why are you using Brave in the first place?

With that out of the way, I would like to propose a significant change:

  • Everyone running latest Linux already has WireGuard VPN support in the kernel.
  • There's an offical WireGuard VPN app for Windows.
  • There is an official Wireguard VPN app on iOS and Android.
  • Some people like me even have WireGuard VPN support in their router / firewall.

If you are going to offer WireGuard VPN subscription, then at least do so by using official applications for the respective platforms, instead of rolling your own support for it into the browser. That would be beneficial for several reasons:

  1. Users will be able to opt-out of installing it
  2. Users will trust official WireGuard implementation more than yours
  3. If users already have official WireGuard app installed, you just have to offer new connection profile

We already include the official wireguard implementation in the form of the wireguard dll and that is what one of the two services manages (when it does run), but the critical issue here is that we add an extra layer of privacy on top of wireguard because you don't login to Guardian directly. The authentication works by logging into Brave to get an authentication token. That authentication token is then used to login to Guardian. So Brave knows who you are because we need that information for authentication and billing, but we don't know anything about the VPN because that is managed by Guardian. Guardian in turn does not know who you are because you logged in using a generic authentication token that was issued by Brave. The browser is a critical part of this because it retrieves and manages the authentication tokens issued through https://account.brave.com/.

Current implementation totally disregards the possibility that users might already have a VPN and doesn't play nice with official solution.

This is intentional because as I explained above, we are adding an extra layer of privacy here between the user and the VPN provider. While you may prefer to just install/use the wireguard app or wireguard built into your router, this is not a user friendly experience for most people and the extra layer of privacy would make it even more complicated. You are certainly welcome to get a subscription directly from Guardian or some other VPN provider and use your existing implementations so we are not disregarding anything.

In the meantime, for the users who would like to remove the services, here's the workaround using admin command prompt:

sc delete BraveVpnService sc delete BraveVpnWireguardService This will remove the services immediately if they are not started, otherwise they will disappear after a reboot.

This is actually not a great solution because until we change this code, the updater will reinstall them if they are missing. As suggested above by @Emi-HoloGhostRevisionist88 the best solution if you want to make sure Brave cannot install admin services (now or in the future) is to switch to a user install. Alternatively, I think (I haven't verified this) that if you switch the service from "manual" to "disabled" then the installer/updater will not change that because I think it only checks to see if the service exists or not. This is also what people normally do to prevent windows built-in services from running because those are impossible to uninstall.

bridiver commented 1 year ago

@levicki I'm going to address one point from your post

apps like Brave should not circumvent the existing operating system VPN infrastructure by bundling custom VPN services but should instead hook into the OS and allow proper management using OS or 3rd party VPN client implementors' policies.

We are not circumventing anything. The VPN is a regular VPN connection in the OS just like any other VPN would be (although at some point you will have to re-authenticate through Brave to update your anonymous credentials when they expire). What we are doing is improving privacy by decoupling the authentication to the VPN itself from any user identifiable information. We are absolutely not rolling our own VPN tunnel, we are building on top of it to enhance user privacy.

This issue is for changing the services to not register by default. If there is some other specific aspect of Brave vpn that you would like to see changed, feel free to open an issue for that.

levicki commented 1 year ago

This issue is for changing the services to not register by default. If there is some other specific aspect of Brave vpn that you would like to see changed, feel free to open an issue for that.

The only thing I'd like to see in addition to the services not being installed by default is if there was a group policy to control their use under Windows.

Also, more transparency in the future before adding new services wouldn't hurt.

Marko-98 commented 1 year ago

It is not disingenuous in the slightest bit and I am not even remotely ashamed to be making it. We consider the VPN to be a feature of Brave. Period. You make not like this, but it is our decision to make.

@bridiver Honestly, I have nothing against you adding feature to the browser you think are useful. But it would really be nice if we are asked about features upfront. You can either offer users before installation which features user wants to use and install it accordingly. Or, you can install everything and on first browser launch ask user if he wants to use VPN, for example, or not. If users chooses to not use the product, browser should automatically remove the VPN components in the background.

I actually made a feature request here (#33733) which would basically allow us to completely disable features we don't need or want to use. It doesn't make sense to me that some features can be completely disabled and hidden from the UI, but others can be disabled while leaving its traces in the browser itself. There are also things I can completely disable and remove from the UI on desktop version, but can't do the same on Android. That inconsistency is killing me.

bridiver commented 1 year ago

@bridiver Honestly, I have nothing against you adding feature to the browser you think are useful. But it would really be nice if we are asked about features upfront. You can either offer users before installation which features user wants to use and install it accordingly. Or, you can install everything and on first browser launch ask user if he wants to use VPN, for example, or not. If users chooses to not use the product, browser should automatically remove the VPN components in the background.

Please see my previous post. This is wildly impractical and would result in users quitting Brave because they are annoyed about prompts and/or don't understand what we're the features are. It's also not practical from a development standpoint to maintain something like this.

I actually made a feature request here (#33733) which would basically allow us to completely disable features we don't need or want to use. It doesn't make sense to me that some features can be completely disabled and hidden from the UI, but others can be disabled while leaving its traces in the browser itself. There are also things I can completely disable and remove from the UI on desktop version, but can't do the same on Android. That inconsistency is killing me.

This is also impractical. There is a cost associated with maintaining flags inside the code, both an ongoing development cost of maintaining the flags and also an ongoing time cost to run tests that check that all aspects of the feature are properly disabled along with the flag. We use flags primarily when we are testing features and generally remove them we have rolled them out to all users. We do not have the budget to maintain an infinitely customizable browser.

bridiver commented 1 year ago

The only thing I'd like to see in addition to the services not being installed by default is if there was a group policy to control their use under Windows.

I think (not 100% sure) the vpn can already be disabled by group policy, but I'm not certain if that currently has any effect on whether the services are installed or not. If the VPN is disabled by policy, they will never run.

Also, more transparency in the future before adding new services wouldn't hurt.

I'm not sure what you mean here. If you're talking about some kind of prompt, that topic has already been covered in previous posts. If you mean before we write the code, there is always a github issue and code review/PR for any feature that goes into Brave.

I've already given plenty of warning so do not expect any further posts that are not strictly about this particular change to be answered and they will most likely get deleted.

ghost commented 1 year ago

Is Brave going to provide a way for per-user installations to get VPN services to be installed as services = have a button or toggle or something, where people are asked for admin rights to install the services? or do people have to reinstall Brave with admin rights to Brave to get these benefits?

We don't currently have any plans to enable this for user installs. Our focus right now is finding a way to register the services only when vpn is enabled, but retain the admin rights required for some operations. While it would technically be possible to prompt for admin on a user install, there's also a concern that while some people view the prompt as a feature because they want to provide explicit permission, other people will view it as confusing or annoying. Also the vast majority of user installs likely do not have admin rights in the first place. I wouldn't say it's off the table and feel free to open up a new issue for it, but our primary focus right now is this issue.

okay! that's all I wanted to know. This issue is called "Windows should not install VPN services until VPN is purchased/enabled" that means, you are going to fix it for Brave with admin rights, but per-user installs won't get it installed any service even if they enable the VPN.

Since you Brave Team say Brave Vpn Service is for the DNS leak, you are still leaving potential people unprotected only because they installed per-user install? If users aren't running the full version of VPN = DNS can leak = not really secure/private solution.

So I don't see how it is not relevant for this discussion. Only because per-user installs don't have VPN or Updater services installed, doesn't mean, they should run an insecure VPN solution that can leak DNS because Microsoft features, so yes, these services should be also installed for per-user when they enable VPN. That's the proper way of doing things, people can't just change from admin to non-admin, and then the other way back, only because a feature is more secure, it should be secure 100%

Of course it can be done in a new issue, but you already say 'it is off the table' already so what's the point?, my opinion is simple, Brave should offer the same protection to users regardless of how they got Brave installed, through Win Store, Web installer, Github, Winget. And you are willing to 'off the table' that already because users could be annoyed.

If users are going to use VPN, you can inform why admin rights VPN is necessary, and if users want it, push the button to install VPN as service. You know "Brave is not running with admin rights, this means you are not fully protected against DNS leaks and you can't use Brave after a restart, if you want to have these benefits and be fully protected, please push this button to install VPN as service"

I never noticed VPN installing services, because I am per-user installation, that means my VPN experience is not good for not giving Brave admin rights. I could upgrade to normal admin rights version, but I won't, because Brave should give users the same level of protection.

This is why I say, Brave should do the right thing, like Brave should not install VPN services until VPN is purchased/enabled for ANY installation of Brave.

You have disabled CNAME uncloaking in a lot of situations to avoid DNS leakage, so it is weird you made a full services to take care of it because you know it can happen in Windows, and you still shrugs and say... "well, not many people use per-user installations, they will be okay with their not full protection, not running 24/7 VPN, if they want to fix it, they can install brave with admin rights".

Of course Brave will do whatever they want, but yes, it is still relevant to this discussion, not in the way people see it "oh Brave installed services for VPN, how terrible" but "Brave will not fix VPN for some users who DONT get the services installed because they think it doesn't matter and it is off the table"

I also had to know your stance on what I asked, I will not make an issue so it will be ignored, I didn't even know I had to have services installed for VPN to fully work. And know I know the answer, well, what can I say? that looks even worst than what people are complaining about here.

VPN should be the priority regardless of admin rights or not, there shouldn't be any DNS leaks, no privacy or security reasons, and that's it. If the services in question provide that, then, all users who use VPN should or not get them if wanted or not, without changing installation.

Now I got the answer, I can fully leave the discussion, since I am 'off the table' unless I go and uninstall and reinstall full admin rights, then this issue will apply to me (in theory).

bridiver commented 1 year ago

@Emi-HoloGhostRevisionist88 did I mistype? I meant to say it isn't off the table, but is separate from this issue. With a user install you are still protected within Brave because it uses DNS over HTTPS by default.

bsclifton commented 1 year ago

Thanks for the good input and discussion @Emi-HoloGhostRevisionist88 πŸ˜„ I like the idea of offering a way to escalate a user install to admin explicitly to install services like this. If you can create an issue, we can prioritize that work πŸ˜„πŸ‘

Not a lot of progress made over the weekend - but I am working on a solution for this issue right now. Starting with code to remove the services and removing the tray icon. Then, we can add the services after someone makes the purchases.

@Marko-98 thanks for creating https://github.com/brave/brave-browser/issues/33733 πŸ˜„ This is something that you should be able to do now with group policy. If you check here under Brave Specific Policy Settings: https://support.brave.com/hc/en-us/articles/360039248271-Group-Policy

You can use regedit.exe and make sure there is key at HKEY_LOCAL_MACHINE\SOFTWARE\Policies\BraveSoftware\Brave . Then create these subkeys (DWORD) with the follow values (all or choose the ones you want disabled): TorDisabled = 1 IPFSEnabled = 0 BraveRewardsDisabled = 1 BraveWalletDisabled = 1 BraveVPNDisabled = 1

Those features should be completely hidden from the UI and will be non-functional. Disabling VPN via group policy was recently added with https://github.com/brave/brave-browser/issues/29397 and that should also prevent the services from being installed. However, I need to verify it doesn't install the services AND I need to work with support to update the page to include it! Will follow up on that later tonight

@Marko-98 I'm not sure if that's closes out the root concern you had in https://github.com/brave/brave-browser/issues/33733 - but please let me know

levicki commented 1 year ago

If you're talking about some kind of prompt, that topic has already been covered in previous posts.

Why can't other software developers do the same, sensible, thing which Apple already does on each update that brings new features?

It is really stupidly simple.

Upon the first launch after an update which introduces new features, those features are presented in a form similar to a setup wizard with number of pages equal to number of new features which is usually titled something like "See what's new in X" (which can even be dismissed without looking if you're so inclined).

For every new feature they show you a brief description — not what it does, but how can it be useful for you and they offer you to enable it right away or do it later in the Settings.

I can't imagine any good excuse why you wouldn't want to do that too — it is an approach that takes minimal development effort (you develop wizard once then control when and what it shows with some JSON or XML config deployed together with an update), it has great discoverabilty while at the same time allows users to make an informed choice without feeling that anything is being forced on them. It also shows you are being transparent about what you do and that you actually respect user choice.

Sadly, the only way some companies like Microsoft, Google, and now it seems Brave know how to make a new feature discoverable is to shove it in without users' consent and make it enabled by default.

Marko-98 commented 1 year ago

This is also impractical. There is a cost associated with maintaining flags inside the code, both an ongoing development cost of maintaining the flags and also an ongoing time cost to run tests that check that all aspects of the feature are properly disabled along with the flag. We use flags primarily when we are testing features and generally remove them we have rolled them out to all users. We do not have the budget to maintain an infinitely customizable browser.

@bridiver I never asked for flags, just the few options in the settings, I really don't think that would be massive financial drain.

I'm not sure if that's closes out the root concern you had in https://github.com/brave/brave-browser/issues/33733 - but please let me know

@bsclifton THANK YOU! That was exactly what I was looking for, I just wish there would be an option for this in typical browser settings (maybe under new category Brave features? πŸ˜€), so the changes can be made on other platforms like Android as well.

I do hope you'll eventually add more settings like the ability to hide the Brave Shields context menu entry (without having to start it with --disable-brave-extension and disabling Brave Search bar in private tab. I understand it's not priority for you currently, but maybe in a future versions...? πŸ™‚

bridiver commented 1 year ago

This is also impractical. There is a cost associated with maintaining flags inside the code, both an ongoing development cost of maintaining the flags and also an ongoing time cost to run tests that check that all aspects of the feature are properly disabled along with the flag. We use flags primarily when we are testing features and generally remove them we have rolled them out to all users. We do not have the budget to maintain an infinitely customizable browser.

@bridiver I never asked for flags, just the few options in the settings, I really don't think that would be massive financial drain.

I'm not sure what you're asking for then because we already have prefs for most things, but disabling them is not going to remove the code from Brave. In any case you should open separate issues for any specific requests you have if you haven't already.

bridiver commented 1 year ago

We have done some investigation/prototyping and have a plan to solve this issue without breaking any functionality for Brave VPN users so I'm going to close the comments here as there's really nothing new to add at this point and they are being abused by some people. If you have additional concerns outside of this issue, please feel free to open a new issue for those specific concerns.

bsclifton commented 12 months ago

Hi folks - I wanted to give an update for those subscribed to this issue for updates

I have a work-in-progress code branch here: https://github.com/brave/brave-core/pull/20754

When a person runs the installer for Brave, a UAC prompt will be presented if your user is in the administrative group. When accepted, this is considered a system install and (with the currently available version of Brave on Windows) the VPN services are registered. That same process (registering the services) will run on updates.

Taking advantage of that logic that runs on update, I've written code to remove the services and the tray icon and verified the code works when an update is rolled out. This is the first commit in my branch. I'm working through the scenario now where we would like to add the service registration back in - for paid customers only. We have a hook created for this and I'm working through that code now. There are a lot of small details here but good progress is being made. Once I get something working, the last step would be to revisit the service removal code and make sure the service ONLY stays in place for paid customers (versus removing 100% of the time).

Any solution we have will need to security reviewed BUT we will expedite the fixes when complete.

bsclifton commented 11 months ago

OK folks - I'm happy to share an update

I've got a proof of concept almost finished. This includes removing the services from the machine when folks receive the update and I have tested/verified that logic. Pull request is up at https://github.com/brave/brave-core/pull/20754

I'll continue working on this - the code needs more testing, fixes, and clean up and then I'll open a privacy/security review

Thanks for your patience on this

bsclifton commented 11 months ago

Proof of concept is complete enough to demonstrate the proposal https://github.com/brave/brave-core/pull/20754

I've opened the review internally for privacy/security/others to review. This link won't be visible to non-employees https://github.com/brave/reviews/issues/1447

I'll share updates as the review takes place - most of the comments should be public on https://github.com/brave/brave-core/pull/20754 as they happen. If things look good w/ that review, there are some technical tasks up next (get rid of layer violations in the code, reduce the patches, etc) and then the code will be formally reviewed for acceptance.

I've re-opened conversation here so that folks can comment/ask questions/etc. Feel free to comment on the pull request too. Let's please keep it respectful πŸ˜„ Thanks in advance

bsclifton commented 11 months ago

Code is getting a lot closer to done - special thanks to @simonhong for helping me with the clean up! πŸ˜„

Privacy and security review is complete. Will work with peers to get the technical review completed. Adding unit tests now and will update this issue with a test plan soon.

bsclifton commented 11 months ago

Still working through review feedback here! Thanks for your patience everybody. I have done a lot of testing at this point and it's working great. Just need to address the comments for code cleanup