From a141580f25e23d1986e8a0ab9e955439169dfa41 Mon Sep 17 00:00:00 2001 From: Mikael Brevik Date: Thu, 11 Mar 2021 20:19:36 +0100 Subject: [PATCH] patch: fixes security issue with non-escaping inputs [GHSL-2020-373] --- lib/utils.js | 8 ++-- test/notify-send.js | 20 +++++++-- test/terminal-notifier.js | 94 +++++++++++++++++++-------------------- 3 files changed, 67 insertions(+), 55 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index 2bfacf3..38e529e 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -298,9 +298,9 @@ module.exports.constructArgumentList = function (options, extra) { var keepNewlines = !!extra.keepNewlines; var wrapper = extra.wrapper === undefined ? '"' : extra.wrapper; - var escapeFn = function (arg) { + var escapeFn = function escapeFn(arg) { if (isArray(arg)) { - return removeNewLines(arg.join(',')); + return removeNewLines(arg.map(escapeFn).join(',')); } if (!noEscape) { @@ -313,9 +313,7 @@ module.exports.constructArgumentList = function (options, extra) { }; initial.forEach(function (val) { - if (typeof val === 'string') { - args.push(escapeFn(val)); - } + args.push(escapeFn(val)); }); for (var key in options) { if ( diff --git a/test/notify-send.js b/test/notify-send.js index 894e5df..5e767b9 100644 --- a/test/notify-send.js +++ b/test/notify-send.js @@ -70,13 +70,27 @@ describe('notify-send', function () { notifier.notify({ message: 'some\n "me\'ss`age`"' }); }); - it('should only include strings as arguments', function (done) { - var expected = ['"HACKED"', '--expire-time', '"10000"']; + it('should escape array items as normal items', function (done) { + var expected = [ + '"Hacked"', + '"\\`touch HACKED\\`"', + '--app-name', + '"foo\\`touch exploit\\`"', + '--category', + '"foo\\`touch exploit\\`"', + '--expire-time', + '"10000"' + ]; expectArgsListToBe(expected, done); var notifier = new Notify({ suppressOsdCheck: true }); var options = JSON.parse( - '{"title":"HACKED", "message":["`touch HACKED`"]}' + `{ + "title": "Hacked", + "message":["\`touch HACKED\`"], + "app-name": ["foo\`touch exploit\`"], + "category": ["foo\`touch exploit\`"] + }` ); notifier.notify(options); }); diff --git a/test/terminal-notifier.js b/test/terminal-notifier.js index bb5cecb..912d372 100644 --- a/test/terminal-notifier.js +++ b/test/terminal-notifier.js @@ -11,38 +11,38 @@ var originalUtils = utils.fileCommandJson; var originalMacVersion = utils.isMountainLion; var originalType = os.type; -describe('Mac fallback', function() { +describe('Mac fallback', function () { var original = utils.isMountainLion; var originalMac = utils.isMac; - afterEach(function() { + afterEach(function () { utils.isMountainLion = original; utils.isMac = originalMac; }); - it('should default to Growl notification if older Mac OSX than 10.8', function(done) { - utils.isMountainLion = function() { + it('should default to Growl notification if older Mac OSX than 10.8', function (done) { + utils.isMountainLion = function () { return false; }; - utils.isMac = function() { + utils.isMac = function () { return true; }; var n = new NotificationCenter({ withFallback: true }); - n.notify({ message: 'Hello World' }, function(_, response) { + n.notify({ message: 'Hello World' }, function (_, response) { expect(this).toBeInstanceOf(Growl); done(); }); }); - it('should not fallback to Growl notification if withFallback is false', function(done) { - utils.isMountainLion = function() { + it('should not fallback to Growl notification if withFallback is false', function (done) { + utils.isMountainLion = function () { return false; }; - utils.isMac = function() { + utils.isMac = function () { return true; }; var n = new NotificationCenter(); - n.notify({ message: 'Hello World' }, function(err, response) { + n.notify({ message: 'Hello World' }, function (err, response) { expect(err).toBeTruthy(); expect(this).not.toBeInstanceOf(Growl); done(); @@ -50,65 +50,65 @@ describe('Mac fallback', function() { }); }); -describe('terminal-notifier', function() { - beforeEach(function() { - os.type = function() { +describe('terminal-notifier', function () { + beforeEach(function () { + os.type = function () { return 'Darwin'; }; - utils.isMountainLion = function() { + utils.isMountainLion = function () { return true; }; }); - beforeEach(function() { + beforeEach(function () { notifier = new NotificationCenter(); }); - afterEach(function() { + afterEach(function () { os.type = originalType; utils.isMountainLion = originalMacVersion; }); // Simulate async operation, move to end of message queue. function asyncify(fn) { - return function() { + return function () { var args = arguments; - setTimeout(function() { + setTimeout(function () { fn.apply(null, args); }, 0); }; } - describe('#notify()', function() { - beforeEach(function() { - utils.fileCommandJson = asyncify(function(n, o, cb) { + describe('#notify()', function () { + beforeEach(function () { + utils.fileCommandJson = asyncify(function (n, o, cb) { cb(null, ''); }); }); - afterEach(function() { + afterEach(function () { utils.fileCommandJson = originalUtils; }); - it('should notify with a message', function(done) { - notifier.notify({ message: 'Hello World' }, function(err, response) { + it('should notify with a message', function (done) { + notifier.notify({ message: 'Hello World' }, function (err, response) { expect(err).toBeNull(); done(); }); }); - it('should be chainable', function(done) { + it('should be chainable', function (done) { notifier .notify({ message: 'First test' }) - .notify({ message: 'Second test' }, function(err, response) { + .notify({ message: 'Second test' }, function (err, response) { expect(err).toBeNull(); done(); }); }); - it('should be able to list all notifications', function(done) { - utils.fileCommandJson = asyncify(function(n, o, cb) { + it('should be able to list all notifications', function (done) { + utils.fileCommandJson = asyncify(function (n, o, cb) { cb( null, fs @@ -117,14 +117,14 @@ describe('terminal-notifier', function() { ); }); - notifier.notify({ list: 'ALL' }, function(_, response) { + notifier.notify({ list: 'ALL' }, function (_, response) { expect(response).toBeTruthy(); done(); }); }); - it('should be able to remove all messages', function(done) { - utils.fileCommandJson = asyncify(function(n, o, cb) { + it('should be able to remove all messages', function (done) { + utils.fileCommandJson = asyncify(function (n, o, cb) { cb( null, fs @@ -133,14 +133,14 @@ describe('terminal-notifier', function() { ); }); - notifier.notify({ remove: 'ALL' }, function(_, response) { + notifier.notify({ remove: 'ALL' }, function (_, response) { expect(response).toBeTruthy(); - utils.fileCommandJson = asyncify(function(n, o, cb) { + utils.fileCommandJson = asyncify(function (n, o, cb) { cb(null, ''); }); - notifier.notify({ list: 'ALL' }, function(_, response) { + notifier.notify({ list: 'ALL' }, function (_, response) { expect(response).toBeFalsy(); done(); }); @@ -148,24 +148,24 @@ describe('terminal-notifier', function() { }); }); - describe('arguments', function() { - beforeEach(function() { + describe('arguments', function () { + beforeEach(function () { this.original = utils.fileCommandJson; }); - afterEach(function() { + afterEach(function () { utils.fileCommandJson = this.original; }); function expectArgsListToBe(expected, done) { - utils.fileCommandJson = asyncify(function(notifier, argsList, callback) { + utils.fileCommandJson = asyncify(function (notifier, argsList, callback) { expect(argsList).toEqual(expected); callback(); done(); }); } - it('should allow for non-sensical arguments (fail gracefully)', function(done) { + it('should allow for non-sensical arguments (fail gracefully)', function (done) { var expected = [ '-title', '"title"', @@ -191,8 +191,8 @@ describe('terminal-notifier', function() { }); }); - it('should validate and transform sound to default sound if Windows sound is selected', function(done) { - utils.fileCommandJson = asyncify(function(notifier, argsList, callback) { + it('should validate and transform sound to default sound if Windows sound is selected', function (done) { + utils.fileCommandJson = asyncify(function (notifier, argsList, callback) { expect(testUtils.getOptionValue(argsList, '-title')).toBe('"Heya"'); expect(testUtils.getOptionValue(argsList, '-sound')).toBe('"Bottle"'); callback(); @@ -206,14 +206,14 @@ describe('terminal-notifier', function() { }); }); - it('should convert list of actions to flat list', function(done) { + it('should convert list of actions to flat list', function (done) { var expected = [ '-title', '"title \\"message\\""', '-message', '"body \\"message\\""', '-actions', - 'foo,bar,baz "foo" bar', + '"foo","bar","baz \\"foo\\" bar"', '-timeout', '"10"', '-json', @@ -232,7 +232,7 @@ describe('terminal-notifier', function() { }); }); - it('should still support wait flag with default timeout', function(done) { + it('should still support wait flag with default timeout', function (done) { var expected = [ '-title', '"Title"', @@ -252,7 +252,7 @@ describe('terminal-notifier', function() { notifier.notify({ title: 'Title', message: 'Message', wait: true }); }); - it('should let timeout set precedence over wait', function(done) { + it('should let timeout set precedence over wait', function (done) { var expected = [ '-title', '"Title"', @@ -277,7 +277,7 @@ describe('terminal-notifier', function() { }); }); - it('should not set a default timeout if explicitly false', function(done) { + it('should not set a default timeout if explicitly false', function (done) { var expected = [ '-title', '"Title"', @@ -299,7 +299,7 @@ describe('terminal-notifier', function() { }); }); - it('should escape all title and message', function(done) { + it('should escape all title and message', function (done) { var expected = [ '-title', '"title \\"message\\""',