Closed krumelmonster closed 1 month ago
Hi @krumelmonster
Thank you for reporting this vulnerability. Since this issue is the same one as the one reported on HackerOne, we would rather track this vulnerability there as it is the official responsible disclosure channel. This issue is already being handled internally by security and Electron developers.
How can users track the progress of this issue? Why not keep this issue here open until a fix is released in Rocket.Chat stable version?
@foss-
As per the responsible disclosure policy and vulnerability management process, security issues must be reported via HackerOne and tracked internally there. Once a fix is created, it will show in the releases and sometimes a CVE will be assigned to the vulnerability.
It is done this way so we prevent malicious attackers from having access to vulnerabilities long before a fix is released. In this case, since this one is being considered as a low risk / low severity vulnerability due to the attack complexity, there's not much of a problem in having the issue here. However, if it was a critical vulnerability - e.g. RCE, NoSQL injection, etc. - we would have to delete the issue and still keep tracking it via internal channels until a fix was officially released.
As mentioned in the hackerone ticket, I will not further discuss this issue privately. After two years of explaining all the ins and outs of this issue and answering all your questions, including providing suggestions on how to address it, I believe I have given sufficient information. Only yesterday did you move the severity from High to Low, despite Rocket.Chat's stance on this issue in 2022 being:
As a public disclaimer we have detected a process flaw where we were not aware of some researchers questions in our hackerone program, we detected it and changed the process so we can avoid this error again. Regarding the vulnerability, our developers are aware and they're working to deliver the patch we will release new information regarding this vulnerability as soon as the vulnerability has been fixed.
https://github.com/RocketChat/Rocket.Chat/issues/27175
Given this history, it is disheartening that an issue compromising TLS integrity is deemed low-severity. I believe that a vulnerability so likely to render TLS ineffective is inherently low-complexity yet high-risk.
Since you have chosen not to engage in a public dialogue, I will conclude my involvement and retract the offer of working to submit a pull request. It is my opinion that a product marketed with "complete privacy and security" should not leave an Improper TLS Certificate Validation issue unaddressed for over two years. This undermines the product’s security claims and its suitability for use.
We understand that you may be disappointed with the way the matter has been handled. Rocket.Chat is currently restructuring its internal application security program so that vulnerabilities can be better prioritized and addressed more quickly than before.
As mentioned above, the issue was categorized as a low severity vulnerability due to the complexity of the attack scenario. From your explanation of the problem - both on HackerOne and here - we understand that the attack would only be possible if the attacker finds a way to change DNS to point the target domain to the device they control, and also if the user clicks on "Yes" when prompted to accept the certificate. That is definitely not low-complexity.
Notwithstanding, this issue is being handled internally by security and development professionals. The idea is having a dialogue that wouldn't select "Yes" by default and that would be more verbose to the end user. In the future, we may have a more robust way of managing certificates in the Electron app.
I explicitly stated in the hackerone ticket that
There are various ways of relaying the connections to the middleperson but most importantly would this be done on a DNS or an IP level.
This means for example: ISPs, anyone conntected to the same network, anyone aware of your wifi credentials, hosting providers, VPN-providers, CDNs, anyone in control of malware connected to network involved in the communication between client and server and many more can present the user with an unlimited number of dialogs such as this, completely compromising the users Rocket!Chat account including all data and privilege of that account if the user is tricked into clicking the "Yes"-Button, Spacebar or Enter:
Changing the DNS is just one of many ways to exploit this vulnerability. It constitutes an Improper TLS Certificate Validation issue, where invalid certificates are presented to the user as acceptable. Consequently, trust between the client and server cannot be assured, and all user sessions must be considered compromised.
I have made very detailed suggestions on how to address the issue in the hackerone ticket.
I understand, but none of those examples are low-complexity. They all assume that the attacker has privileged access to the local network, or to the ISPs, or to the CDNs, or to the VPN providers, and so forth. I'm not saying it is impossible, I'm just saying it is complex.
Regardless of the severity, we deeply appreciate the report and we are taking a look.
What you describe is not the Complexity, it is the Attack Vector and the Attack Vector of this vulnerability is not Local Network or Adjacent Network but rather Network. These are the CVSS definitions:
A vulnerability exploitable with Network access means the vulnerable component is bound to the network stack and the attacker's path is through OSI layer 3 (the network layer). Such a vulnerability is often termed 'remotely exploitable' and can be thought of as an attack being exploitable one or more network hops away (e.g. across layer 3 boundaries from routers).
A vulnerability exploitable with Adjacent Network access means the vulnerable component is bound to the network stack, however the attack is limited to the same shared physical (e.g. Bluetooth, IEEE 802.11), or logical (e.g. local IP subnet) network, and cannot be performed across an OSI layer 3 boundary (e.g. a router).
A vulnerability exploitable with Local access means that the vulnerable component is not bound to the network stack, and the attacker's path is via read/write/execute capabilities. In some cases, the attacker may be logged in locally in order to exploit the vulnerability, or may rely on User Interaction to execute a malicious file.
Such MITM vulnerabilities are generally regarded AV:N (as illustrated in the cvss examples section) as they neither require network access to be local (as would be with AV:A) nor do they require shell access or similar to either trusted party (as would be with AV:L). I am referring to the examples and definitions provided in the CVSS documentation as that is the scoring system hackerone uses https://www.first.org/cvss/
High Complexity (AC:H) is also not given as AC:H is defined as (emphasis theirs)
The successful attack depends on the evasion or circumvention of security-enhancing techniques in place that would otherwise hinder the attack. These include: Evasion of exploit mitigation techniques. The attacker must have additional methods available to bypass security measures in place. For example, circumvention of address space randomization (ASLR) or data execution prevention (DEP) must be performed for the attack to be successful. Obtaining target-specific secrets. The attacker must gather some target-specific secret before the attack can be successful. A secret is any piece of information that cannot be obtained through any amount of reconnaissance. To obtain the secret the attacker must perform additional attacks or break otherwise secure measures (e.g. knowledge of a secret key may be needed to break a crypto channel). This operation must be performed for each attacked target.
Further applying the definitions of CVSS including the Requirement of user interaction (UI:R) yields a score of 8.8 when assuming Low Complexity (AC:L) and a score of 7.5 when (in my opinion wrongfully) assuming High Complexity (AC:H).
You have however manually changed the severity from 8.3 (High) to Low (below 3.9).
Thank you for working on this! On my test system, "Yes" is still the default selected option. Only difference is I'm seeing is the new warning message which is an improvement.
I don't believe this fully resolves the issue but as the hackerone is still open, I understand this patch is just the first step.
Possibly related https://github.com/electron/electron/issues/18258 "dialog.showMessageBox doesn't respect defaultId option on Windows" and https://github.com/electron/electron/issues/22600 . It does default to "No" now on my linux.
Search before asking
Operating System
Operating System Version
Windows 10, Arch Linux and others
It happens on the web browser too?
No, it just happens on the Desktop app
Rocket.Chat Desktop App Version
4.4.0, 3.7.5 and others
Rocket.Chat Server Version
any
Describe the bug
This vulnerability has been reported confidentially 2022-05-30 and the responsible disclosure timeline is elapsed.
Summary
Do you trust certificate "[...]"?
, allowing attackers to change the question by means of social engineering, further tricking the user to choose "Yes". (e.g.Do you trust certificate from "Microsoft" and want to keep your installation of "Windows"?
, see screenshot in comment below)Screenshots
For comparison: How Browsers handle invalid certificates when opening a site (note that in the given situation, where a secure site is already present, browsers would not even display a dialog and silently block the attack)
Firefox: ![image](https://github.com/RocketChat/Rocket.Chat.Electron/assets/13542760/313ee191-eca8-4d77-92dc-edf907910d77) Chrome: ![image](https://github.com/RocketChat/Rocket.Chat.Electron/assets/13542760/4e0b54e3-e7fd-47ec-b718-cefee4edd56c)Impact
If a user can be tricked intro pressing the Enter key, the Spacebar key or the Yes button, an attacker could gain full control over their account.
How to Reproduce
openssl req -new -x509 -days 365 -nodes -out yourboss.pem -keyout yourboss.key -subj '/CN=your boss'
Describe your Expected behavior
When solving this issue, please consider whether it is neccessary to allow insecure TLS at all, and if so, how much work you want to put into allowing this in a secure way. I see three options:
Forbid invalid TLS-Certificates altogether, the dialog could say: "Cannot connect to Server: TLS error: ." -- no options just ok. This is how many applications handle such situations and this change would be a very easy and complete fix of this vulnerability. I believe it should still be possible to set up self-signed certificates and the such by working with the systems certificate store.
Redesign the certificate error dialog in more a secure way, minimal requirements for this are:
Implementing solution 2 will certainly be a lot of work and only be effective if all above requirements are met.
A compromise: Option one but allow skilled users/administrators to install custom trusted certificate files either via the gui or by placing them in Rocket!Chats config directory.
For a very quick mitigation: Forbid invalid TLS-Certificates altogether (who needs this anyway?) but with an opt-in to invalid certificates from the Settings dialog, make it very clear in the dialog that enabling this makes Rocket.Chat insecure
Anything else
https://github.com/RocketChat/Rocket.Chat/issues/27175 1586249
Are you willing to submit a code contribution?