Security risk: RTL can be accessed with empty SSO key when the space runs out

[Kixunil]

Describe the bug

If the partition used for storing the SSO cookie runs out of space, the SSO token is set to empty leading to enabling trivial access by anyone. LN node may still be working if the partition is different (as was in my case), so stealing is possible. Non-private disclosure was selected because this can only happen in edge case scenarios and should be very hard to cause intentionally.

The bug occurs because the ENOSPC error is only generated by the OS when writing to a file, not when it's created. The best practice to avoid these kinds of errors and other catastrophic data corruption bugs is to write a temporary file in the same directory, sync it (fsync system call) and if all that succeeded move it over the old location. Move is atomic, so the data can't get corrupted. In case of RTL, unlinking the cookie file before attempting to do anything else sounds like a good way to prevent reuse of the cookie.

Additionally, I recommend to not read the cookie from the file at all, just write it. Remember and use the copy that is already in memory that was used for writing. This way no weird kind of OS failure can ever cause the cookie to be predictable by the attacker. Writing the code this way would've prevented this attack.

Feel free to take inspiration from a similar code I wrote for btc-rpc-explorer that was written defensively, using both mentioned techniques. Although now I see it doesn't contain explicit fsync call - not a big deal there since it actually doesn't need to be physically stored on disk (Cache in RAM is enough, next boot will invalidate it anyway.) Note that token is always refreshed on start - this is intentional to avoid surprises like this one.

The code mentioned above happens to also have more optimal token length and representation.

To Reproduce
Steps to reproduce the behavior:

1. Install and configure RTL to use SSO, pointing to a different partition than LN node
2. Fill up the prtition
3. Attempt to login - it will fail
4. Attempt to login with empty key - it will succeed

Your environment

• Version of RTL: 0.9.0 - I already checked that the cookie handling code is broken in master
  The rest is irrelevant as the code is OS-independent.

Additional context
The cookie handling is implemented in:

RTL/connect.js

Lines 301 to 333 in f817ae3

	connect.readCookie = (cookieFile) => {
	let exists = fs.existsSync(cookieFile);
	if (exists) {
	try {
	common.cookie = fs.readFileSync(cookieFile, 'utf-8');
	} catch (err) {
	console.error('Something went wrong while reading cookie: \n' + err);
	throw new Error(err);
	}
	} else {
	try {
	var dirname = path.dirname(cookieFile);
	connect.createDirectory(dirname);
	fs.writeFileSync(cookieFile, crypto.randomBytes(64).toString('hex'));
	common.cookie = fs.readFileSync(cookieFile, 'utf-8');
	}
	catch(err) {
	console.error('Something went wrong while reading the cookie: \n' + err);
	throw new Error(err);
	}
	}
	}
	connect.refreshCookie = (cookieFile) => {
	try {
	fs.writeFileSync(cookieFile, crypto.randomBytes(64).toString('hex'));
	common.cookie = fs.readFileSync(cookieFile, 'utf-8');
	}
	catch(err) {
	console.error('Something went wrong while refreshing cookie: \n' + err);
	throw new Error(err);
	}
	}

[ShahanaFarooqui]

@Kixunil, Thanks for raising the issue and bringing it to our attention.

Although, we still want to continue to read the cookie from a file only. Authentication through cookie is only encouraged when the user is redirected from some secure third party application. Initially, it was introduced to enable single sign-on feature on BTCPayServer and changing it will break their logic.

Thus, we decided to fix the bug with PR #646 where we added the logic to:
1: Throw the error in the UI if the user tries to send an empty access key.
2: RTL server will return 406, unauthenticated user error if the cookie is null or empty.

[Kixunil]

sign-on feature on BTCPayServer and changing it will break their logic.

Does it mean BTCPayServer writes the cookie itself instead of just reading it?

[ShahanaFarooqui]

sign-on feature on BTCPayServer and changing it will break their logic.

Does it mean BTCPayServer writes the cookie itself instead of just reading it?

No, BTCPayServer only sets up the location of the cookie file and passes that value as access-key in the url. While writting, reading, refreshing and authentication from that cookie file is RTL server's responsibility.

[Kixunil]

In that case changing the code to only write would not have broken BTCPayServer because I it only reads the cookie file.