From a7d1e682e6d6a11c53733cd31ca3b24d5a86d1ba Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Tue, 2 Apr 2019 17:58:47 +0200 Subject: [PATCH 1/7] Extract logic to get the socket name to a function --- src/main-process/atom-application.js | 64 +++++++++++++++------------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index 9a241c32b..f7eff78eb 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -34,6 +34,40 @@ const getDefaultPath = () => { } } +// Returns a unique id for each Atom instance. This id allows many Atom windows +// that share the same configuration to reuse the same main process. +const getAtomInstanceId = (atomVersion) => { + const {username} = os.userInfo() + + // Lowercasing the ATOM_HOME to make sure that we don't get multiple sockets + // on case-insensitive filesystems due to arbitrary case differences in paths. + const atomHomeUnique = path.resolve(process.env.ATOM_HOME).toLowerCase() + const hash = crypto + .createHash('sha1') + .update(atomVersion) + .update(process.arch) + .update(username || '') + .update(atomHomeUnique) + + return hash.digest('base64') +} + +const getSocketName = (atomVersion) => { + // We only keep the first 12 characters of the hash as not to have excessively long + // socket file. Note that macOS/BSD limit the length of socket file paths (see #15081). + // The replace calls convert the digest into "URL and Filename Safe" encoding (see RFC 4648). + const atomInstanceId = getAtomInstanceId(atomVersion) + .substring(0, 12) + .replace(/\+/g, '-') + .replace(/\//g, '_') + + if (process.platform === 'win32') { + return `\\\\.\\pipe\\atom-${atomInstanceId}-sock` + } else { + return path.join(os.tmpdir(), `atom-${atomInstanceId}.sock`) + } +} + // The application's singleton class. // // It's the entry point into the Atom application and maintains the global state @@ -44,35 +78,7 @@ class AtomApplication extends EventEmitter { // Public: The entry point into the Atom application. static open (options) { if (!options.socketPath) { - const {username} = os.userInfo() - - // Lowercasing the ATOM_HOME to make sure that we don't get multiple sockets - // on case-insensitive filesystems due to arbitrary case differences in paths. - const atomHomeUnique = path.resolve(process.env.ATOM_HOME).toLowerCase() - const hash = crypto - .createHash('sha1') - .update(options.version) - .update('|') - .update(process.arch) - .update('|') - .update(username || '') - .update('|') - .update(atomHomeUnique) - - // We only keep the first 12 characters of the hash as not to have excessively long - // socket file. Note that macOS/BSD limit the length of socket file paths (see #15081). - // The replace calls convert the digest into "URL and Filename Safe" encoding (see RFC 4648). - const atomInstanceDigest = hash - .digest('base64') - .substring(0, 12) - .replace(/\+/g, '-') - .replace(/\//g, '_') - - if (process.platform === 'win32') { - options.socketPath = `\\\\.\\pipe\\atom-${atomInstanceDigest}-sock` - } else { - options.socketPath = path.join(os.tmpdir(), `atom-${atomInstanceDigest}.sock`) - } + options.socketPath = getSocketName(options.version) } // FIXME: Sometimes when socketPath doesn't exist, net.connect would strangely From 5e124f437e8a4b79f22ce4df3c093872af3b6d70 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Tue, 2 Apr 2019 21:06:29 +0200 Subject: [PATCH 2/7] Implement server-side authentication on pipes This is done by using a secret as the name of the pipe --- src/main-process/atom-application.js | 84 +++++++++++++++++----------- 1 file changed, 52 insertions(+), 32 deletions(-) diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index f7eff78eb..ecdd5dd9c 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -34,40 +34,45 @@ const getDefaultPath = () => { } } -// Returns a unique id for each Atom instance. This id allows many Atom windows -// that share the same configuration to reuse the same main process. -const getAtomInstanceId = (atomVersion) => { +const getSocketSecretPath = (atomVersion) => { const {username} = os.userInfo() + const atomHome = path.resolve(process.env.ATOM_HOME) - // Lowercasing the ATOM_HOME to make sure that we don't get multiple sockets - // on case-insensitive filesystems due to arbitrary case differences in paths. - const atomHomeUnique = path.resolve(process.env.ATOM_HOME).toLowerCase() - const hash = crypto - .createHash('sha1') - .update(atomVersion) - .update(process.arch) - .update(username || '') - .update(atomHomeUnique) - - return hash.digest('base64') + return path.join(atomHome, `.atom-socket-secret-${username}-${atomVersion}`) } -const getSocketName = (atomVersion) => { - // We only keep the first 12 characters of the hash as not to have excessively long - // socket file. Note that macOS/BSD limit the length of socket file paths (see #15081). - // The replace calls convert the digest into "URL and Filename Safe" encoding (see RFC 4648). - const atomInstanceId = getAtomInstanceId(atomVersion) - .substring(0, 12) - .replace(/\+/g, '-') - .replace(/\//g, '_') +const getSocketPath = (socketSecret) => { + if (!socketSecret) { + return null + } + + const socketName = socketSecret.substr(0, 12) if (process.platform === 'win32') { - return `\\\\.\\pipe\\atom-${atomInstanceId}-sock` + return `\\\\.\\pipe\\atom-${socketName}-sock` } else { - return path.join(os.tmpdir(), `atom-${atomInstanceId}.sock`) + return path.join(os.tmpdir(), `atom-${socketName}.sock`) } } +const getExistingSocketSecret = (atomVersion) => { + const socketSecretPath = getSocketSecretPath(atomVersion) + + if (!fs.existsSync(socketSecretPath)) { + return null + } + + return fs.readFileSync(socketSecretPath, 'utf8') +} + +const createSocketSecret = (atomVersion) => { + const socketSecret = crypto.randomBytes(32).toString('hex') + + fs.writeFileSync(getSocketSecretPath(atomVersion), socketSecret, 'utf8') + + return socketSecret +} + // The application's singleton class. // // It's the entry point into the Atom application and maintains the global state @@ -77,21 +82,19 @@ module.exports = class AtomApplication extends EventEmitter { // Public: The entry point into the Atom application. static open (options) { - if (!options.socketPath) { - options.socketPath = getSocketName(options.version) - } + const socketPath = options.socketPath || getSocketPath(getExistingSocketSecret(options.version)) // FIXME: Sometimes when socketPath doesn't exist, net.connect would strangely // take a few seconds to trigger 'error' event, it could be a bug of node // or electron, before it's fixed we check the existence of socketPath to // speedup startup. - if ((process.platform !== 'win32' && !fs.existsSync(options.socketPath)) || - options.test || options.benchmark || options.benchmarkTest) { + if ((process.platform !== 'win32' && !fs.existsSync(socketPath)) || + options.test || options.benchmark || options.benchmarkTest || !socketPath) { new AtomApplication(options).initialize(options) return } - const client = net.connect({path: options.socketPath}, () => { + const client = net.connect({path: socketPath}, () => { client.write(JSON.stringify(options), () => { client.end() app.quit() @@ -116,11 +119,13 @@ class AtomApplication extends EventEmitter { this.version = options.version this.devMode = options.devMode this.safeMode = options.safeMode - this.socketPath = options.socketPath this.logFile = options.logFile this.userDataDir = options.userDataDir this._killProcess = options.killProcess || process.kill.bind(process) - if (options.test || options.benchmark || options.benchmarkTest) this.socketPath = null + + if (!options.test && !options.benchmark && !options.benchmarkTest) { + this.socketPath = getSocketPath(createSocketSecret(this.version)) + } this.waitSessionsByWindow = new Map() this.windowStack = new WindowStack() @@ -379,6 +384,20 @@ class AtomApplication extends EventEmitter { } } + deleteSocketSecretFile () { + const socketSecretPath = getSocketSecretPath(this.version) + + if (fs.existsSync(socketSecretPath)) { + try { + fs.unlinkSync(socketSecretPath) + } catch (error) { + // Ignore ENOENT errors in case the file was deleted between the exists + // check and the call to unlink sync. + if (error.code !== 'ENOENT') throw error + } + } + } + // Registers basic application commands, non-idempotent. handleEvents () { const getLoadSettings = () => { @@ -486,6 +505,7 @@ class AtomApplication extends EventEmitter { this.disposable.add(ipcHelpers.on(app, 'will-quit', () => { this.killAllProcesses() this.deleteSocketFile() + this.deleteSocketSecretFile() })) this.disposable.add(ipcHelpers.on(app, 'open-file', (event, pathToOpen) => { From d40ca4b675567f52fe008989ee204058b6223884 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Thu, 4 Apr 2019 11:09:05 +0200 Subject: [PATCH 3/7] Implement authentication and encryption of the client --- src/main-process/atom-application.js | 52 ++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index ecdd5dd9c..b0ada5a53 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -46,7 +46,12 @@ const getSocketPath = (socketSecret) => { return null } - const socketName = socketSecret.substr(0, 12) + // Hash the secret to create the socket name to not expose it. + const socketName = crypto + .createHmac('sha256', socketSecret) + .update('socketName') + .digest('hex') + .substr(0, 12) if (process.platform === 'win32') { return `\\\\.\\pipe\\atom-${socketName}-sock` @@ -66,13 +71,44 @@ const getExistingSocketSecret = (atomVersion) => { } const createSocketSecret = (atomVersion) => { - const socketSecret = crypto.randomBytes(32).toString('hex') + const socketSecret = crypto.randomBytes(16).toString('hex') fs.writeFileSync(getSocketSecretPath(atomVersion), socketSecret, 'utf8') return socketSecret } +const encryptOptions = (options, secret) => { + const message = JSON.stringify(options) + + const initVector = crypto.randomBytes(16).toString('hex') + + const cipher = crypto.createCipheriv('aes-256-gcm', secret, initVector) + + let content = cipher.update(message, 'utf8', 'hex') + content += cipher.final('hex') + + const authTag = cipher.getAuthTag().toString('hex') + + return JSON.stringify({ + authTag, + content, + initVector + }) +} + +const decryptOptions = (optionsMessage, secret) => { + const {authTag, content, initVector} = JSON.parse(optionsMessage) + + const decipher = crypto.createDecipheriv('aes-256-gcm', secret, initVector) + decipher.setAuthTag(Buffer.from(authTag, 'hex')) + + let message = decipher.update(content, 'hex', 'utf8') + message += decipher.final('utf8') + + return JSON.parse(message) +} + // The application's singleton class. // // It's the entry point into the Atom application and maintains the global state @@ -82,7 +118,8 @@ module.exports = class AtomApplication extends EventEmitter { // Public: The entry point into the Atom application. static open (options) { - const socketPath = options.socketPath || getSocketPath(getExistingSocketSecret(options.version)) + const socketSecret = getExistingSocketSecret(options.version) + const socketPath = options.socketPath || getSocketPath(socketSecret) // FIXME: Sometimes when socketPath doesn't exist, net.connect would strangely // take a few seconds to trigger 'error' event, it could be a bug of node @@ -95,7 +132,7 @@ class AtomApplication extends EventEmitter { } const client = net.connect({path: socketPath}, () => { - client.write(JSON.stringify(options), () => { + client.write(encryptOptions(options, socketSecret), () => { client.end() app.quit() }) @@ -124,7 +161,8 @@ class AtomApplication extends EventEmitter { this._killProcess = options.killProcess || process.kill.bind(process) if (!options.test && !options.benchmark && !options.benchmarkTest) { - this.socketPath = getSocketPath(createSocketSecret(this.version)) + this.socketSecret = createSocketSecret(this.version) + this.socketPath = getSocketPath(this.socketSecret) } this.waitSessionsByWindow = new Map() @@ -362,7 +400,9 @@ class AtomApplication extends EventEmitter { const server = net.createServer(connection => { let data = '' connection.on('data', chunk => { data += chunk }) - connection.on('end', () => this.openWithOptions(JSON.parse(data))) + connection.on('end', () => { + this.openWithOptions(decryptOptions(data, this.socketSecret)) + }) }) server.listen(this.socketPath) From bca056d956a48e87c251d37fa4882b6950b76043 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Thu, 4 Apr 2019 12:25:01 +0200 Subject: [PATCH 4/7] Remove the handling logic for the --socket-path arg --- src/main-process/atom-application.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index b0ada5a53..0417d9185 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -119,14 +119,16 @@ class AtomApplication extends EventEmitter { // Public: The entry point into the Atom application. static open (options) { const socketSecret = getExistingSocketSecret(options.version) - const socketPath = options.socketPath || getSocketPath(socketSecret) + const socketPath = getSocketPath(socketSecret) // FIXME: Sometimes when socketPath doesn't exist, net.connect would strangely // take a few seconds to trigger 'error' event, it could be a bug of node // or electron, before it's fixed we check the existence of socketPath to // speedup startup. - if ((process.platform !== 'win32' && !fs.existsSync(socketPath)) || - options.test || options.benchmark || options.benchmarkTest || !socketPath) { + if ( + !socketPath || options.test || options.benchmark || options.benchmarkTest || + (process.platform !== 'win32' && !fs.existsSync(socketPath)) + ) { new AtomApplication(options).initialize(options) return } @@ -425,6 +427,10 @@ class AtomApplication extends EventEmitter { } deleteSocketSecretFile () { + if (!this.socketSecret) { + return + } + const socketSecretPath = getSocketSecretPath(this.version) if (fs.existsSync(socketSecretPath)) { From 31d7dd6bb9c8a555796547dd45cfb06fc8869d50 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Thu, 4 Apr 2019 12:27:33 +0200 Subject: [PATCH 5/7] Add explicit handle for errors when decrypting --- src/main-process/atom-application.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index 0417d9185..dc5671ddc 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -73,7 +73,7 @@ const getExistingSocketSecret = (atomVersion) => { const createSocketSecret = (atomVersion) => { const socketSecret = crypto.randomBytes(16).toString('hex') - fs.writeFileSync(getSocketSecretPath(atomVersion), socketSecret, 'utf8') + fs.writeFileSync(getSocketSecretPath(atomVersion), socketSecret, {encoding: 'utf8', mode: 0o600}) return socketSecret } @@ -81,7 +81,7 @@ const createSocketSecret = (atomVersion) => { const encryptOptions = (options, secret) => { const message = JSON.stringify(options) - const initVector = crypto.randomBytes(16).toString('hex') + const initVector = crypto.randomBytes(16) const cipher = crypto.createCipheriv('aes-256-gcm', secret, initVector) @@ -93,14 +93,14 @@ const encryptOptions = (options, secret) => { return JSON.stringify({ authTag, content, - initVector + initVector: initVector.toString('hex') }) } const decryptOptions = (optionsMessage, secret) => { const {authTag, content, initVector} = JSON.parse(optionsMessage) - const decipher = crypto.createDecipheriv('aes-256-gcm', secret, initVector) + const decipher = crypto.createDecipheriv('aes-256-gcm', secret, Buffer.from(initVector, 'hex')) decipher.setAuthTag(Buffer.from(authTag, 'hex')) let message = decipher.update(content, 'hex', 'utf8') @@ -403,7 +403,13 @@ class AtomApplication extends EventEmitter { let data = '' connection.on('data', chunk => { data += chunk }) connection.on('end', () => { - this.openWithOptions(decryptOptions(data, this.socketSecret)) + try { + const options = decryptOptions(data, this.socketSecret) + this.openWithOptions(options) + } catch (e) { + // Error while parsing/decrypting the options passed by the client. + // We cannot trust the client, aborting. + } }) }) From df54e900d5ef25c7dd52b07d6141c91069ae5208 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Fri, 5 Apr 2019 00:01:31 +0200 Subject: [PATCH 6/7] Add test to ensure that the piping logic works as expected --- spec/main-process/atom-application.test.js | 37 ++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index 7381a21bc..e984abc3f 100644 --- a/spec/main-process/atom-application.test.js +++ b/spec/main-process/atom-application.test.js @@ -939,6 +939,43 @@ describe('AtomApplication', function () { }) } + it('reuses the main process between invocations', async () => { + const tempDirPath1 = makeTempDir() + const tempDirPath2 = makeTempDir() + + const options = { + pathsToOpen: [tempDirPath1] + } + + // Open the main application + const originalApplication = buildAtomApplication(options) + await originalApplication.initialize(options) + + // Wait until the first window gets opened + await conditionPromise( + () => originalApplication.getAllWindows().length === 1 + ) + + // Open another instance of the application on a different path. + AtomApplication.open({ + resourcePath: ATOM_RESOURCE_PATH, + atomHomeDirPath: process.env.ATOM_HOME, + pathsToOpen: [tempDirPath2] + }) + + await conditionPromise( + () => originalApplication.getAllWindows().length === 2 + ) + + // Check that the original application now has two opened windows. + assert.deepEqual( + originalApplication.getAllWindows().map( + window => window.loadSettings.initialPaths + ), + [[tempDirPath2], [tempDirPath1]] + ) + }) + function buildAtomApplication (params = {}) { const atomApplication = new AtomApplication( Object.assign( From 7ad8976a0033cfd3bed97a7c754b08e4f7b1c983 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Fri, 5 Apr 2019 15:03:21 +0200 Subject: [PATCH 7/7] Do not check for order of windows in test The order is not deterministic on Windows, since it depends on focus state --- spec/main-process/atom-application.test.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index e984abc3f..7180f2b2f 100644 --- a/spec/main-process/atom-application.test.js +++ b/spec/main-process/atom-application.test.js @@ -967,12 +967,18 @@ describe('AtomApplication', function () { () => originalApplication.getAllWindows().length === 2 ) - // Check that the original application now has two opened windows. - assert.deepEqual( - originalApplication.getAllWindows().map( - window => window.loadSettings.initialPaths + // Check that the original application now has the two opened windows. + assert.notEqual( + originalApplication.getAllWindows().find( + window => window.loadSettings.initialPaths[0] === tempDirPath1 ), - [[tempDirPath2], [tempDirPath1]] + undefined + ) + assert.notEqual( + originalApplication.getAllWindows().find( + window => window.loadSettings.initialPaths[0] === tempDirPath2 + ), + undefined ) })