Skip to content

Commit 93e4b2e

Browse files
daniellockyerdaniellockyer
committed
🔒 Fixed remote command injection when using sendmail email transport
refs GHSA-wfrj-qqc2-83cm refs GHSA-48ww-j4fc-435p - a vulnerability in `nodemailer` means that the `sendmail` transport is vulnerable to command injection for flags passed to the `sendmail` binary - updating to the latest version of Nodemailer required creating `@tryghost/nodemailer`, which is a wrapper around Nodemailer and several plugins that used to be in the core - this commit switches to using that package, and fixes up some small code + test changes
1 parent 61058fb commit 93e4b2e

File tree

6 files changed

+709
-206
lines changed

6 files changed

+709
-206
lines changed

‎core/server/services/mail/GhostMailer.js‎

Lines changed: 29 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,18 @@ function createMailError({message, err, ignoreDefaultMessage} = {message: ''}) {
6767

6868
module.exports = class GhostMailer {
6969
constructor() {
70-
const nodemailer = require('nodemailer');
71-
const transport = config.get('mail') && config.get('mail').transport || 'direct';
70+
const nodemailer = require('@tryghost/nodemailer');
71+
72+
let transport = config.get('mail') && config.get('mail').transport || 'direct';
73+
transport = transport.toLowerCase();
74+
7275
// nodemailer mutates the options passed to createTransport
7376
const options = config.get('mail') && _.clone(config.get('mail').options) || {};
7477

7578
this.state = {
7679
usingDirect: transport === 'direct'
7780
};
78-
this.transport = nodemailer.createTransport(transport, options);
81+
this.transport = nodemailer(transport, options);
7982
}
8083

8184
/**
@@ -102,52 +105,42 @@ module.exports = class GhostMailer {
102105

103106
const response = await this.sendMail(messageToSend);
104107

105-
if (this.transport.transportType === 'DIRECT') {
108+
if (this.state.usingDirect) {
106109
return this.handleDirectTransportResponse(response);
107110
}
108111

109112
return response;
110113
}
111114

112-
sendMail(message) {
113-
return new Promise((resolve, reject) => {
114-
this.transport.sendMail(message, (err, response) => {
115-
if (err) {
116-
reject(createMailError({
117-
message: i18n.t('errors.mail.reason', {reason: err.message || err}),
118-
err
119-
}));
120-
}
121-
resolve(response);
115+
async sendMail(message) {
116+
try {
117+
const response = await this.transport.sendMail(message);
118+
return response;
119+
} catch (err) {
120+
throw createMailError({
121+
message: i18n.t('errors.mail.reason', {reason: err.message || err}),
122+
err
122123
});
123-
});
124+
}
124125
}
125126

126127
handleDirectTransportResponse(response) {
127-
return new Promise((resolve, reject) => {
128-
response.statusHandler.once('failed', function (data) {
129-
if (data.error && data.error.code === 'ENOTFOUND') {
130-
reject(createMailError({
131-
message: i18n.t('errors.mail.noMailServerAtAddress.error', {domain: data.domain})
132-
}));
133-
}
134-
135-
reject(createMailError());
136-
});
137-
138-
response.statusHandler.once('requeue', function (data) {
139-
if (data.error && data.error.message) {
140-
reject(createMailError({
141-
message: i18n.t('errors.mail.reason', {reason: data.error.message})
142-
}));
143-
}
128+
if (!response) {
129+
return i18n.t('notices.mail.messageSent');
130+
}
144131

145-
reject(createMailError());
132+
if (response.pending.length > 0) {
133+
throw createMailError({
134+
message: i18n.t('errors.mail.reason', {reason: 'Email has been temporarily rejected'})
146135
});
136+
}
147137

148-
response.statusHandler.once('sent', function () {
149-
resolve(i18n.t('notices.mail.messageSent'));
138+
if (response.errors.length > 0) {
139+
throw createMailError({
140+
message: i18n.t('errors.mail.reason', {reason: response.errors[0].message})
150141
});
151-
});
142+
}
143+
144+
return i18n.t('notices.mail.messageSent');
152145
}
153146
};

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@
7979
"@tryghost/members-importer": "0.3.2",
8080
"@tryghost/members-ssr": "1.0.12",
8181
"@tryghost/mw-session-from-token": "0.1.22",
82+
"@tryghost/nodemailer": "0.3.1",
8283
"@tryghost/package-json": "1.0.2",
8384
"@tryghost/promise": "0.1.9",
8485
"@tryghost/request": "0.1.5",
@@ -148,7 +149,6 @@
148149
"mysql": "2.18.1",
149150
"nconf": "0.11.3",
150151
"node-jose": "2.0.0",
151-
"nodemailer": "0.7.1",
152152
"oembed-parser": "1.4.8",
153153
"passport": "0.4.1",
154154
"passport-google-oauth": "2.0.0",

‎renovate.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
"intl-messageformat",
1212
"moment",
1313
"moment-timezone",
14-
"nodemailer",
1514
"simple-dom"
1615
],
1716
"ignorePaths": ["test"],

‎test/unit/services/mail/GhostMailer.test.js‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ describe('Mail: Ghostmailer', function () {
6161
mailer = new mail.GhostMailer();
6262

6363
mailer.should.have.property('transport');
64-
mailer.transport.transportType.should.eql('SMTP');
64+
mailer.transport.transporter.name.should.eql('SMTP');
6565
mailer.transport.sendMail.should.be.a.Function();
6666
});
6767

@@ -71,18 +71,18 @@ describe('Mail: Ghostmailer', function () {
7171
mailer = new mail.GhostMailer();
7272

7373
mailer.should.have.property('transport');
74-
mailer.transport.transportType.should.eql('DIRECT');
74+
mailer.transport.transporter.name.should.eql('SMTP (direct)');
7575
});
7676

7777
it('sends valid message successfully ', function (done) {
7878
configUtils.set({mail: {transport: 'stub'}});
7979

8080
mailer = new mail.GhostMailer();
8181

82-
mailer.transport.transportType.should.eql('STUB');
82+
mailer.transport.transporter.name.should.eql('Stub');
8383

8484
mailer.send(mailDataNoServer).then(function (response) {
85-
should.exist(response.message);
85+
should.exist(response.response);
8686
should.exist(response.envelope);
8787
response.envelope.to.should.containEql('[email protected]');
8888

@@ -95,7 +95,7 @@ describe('Mail: Ghostmailer', function () {
9595

9696
mailer = new mail.GhostMailer();
9797

98-
mailer.transport.transportType.should.eql('STUB');
98+
mailer.transport.transporter.name.should.eql('Stub');
9999

100100
mailer.send(mailDataNoServer).then(function () {
101101
done(new Error('Stub did not error'));
@@ -125,7 +125,7 @@ describe('Mail: Ghostmailer', function () {
125125
});
126126

127127
it('return correct failure message for domain doesn\'t exist', function (done) {
128-
mailer.transport.transportType.should.eql('DIRECT');
128+
mailer.transport.transporter.name.should.eql('SMTP (direct)');
129129

130130
mailer.send(mailDataNoDomain).then(function () {
131131
done(new Error('Error message not shown.'));
@@ -136,7 +136,7 @@ describe('Mail: Ghostmailer', function () {
136136
});
137137

138138
it('return correct failure message for no mail server at this address', function (done) {
139-
mailer.transport.transportType.should.eql('DIRECT');
139+
mailer.transport.transporter.name.should.eql('SMTP (direct)');
140140

141141
mailer.send(mailDataNoServer).then(function () {
142142
done(new Error('Error message not shown.'));
@@ -147,7 +147,7 @@ describe('Mail: Ghostmailer', function () {
147147
});
148148

149149
it('return correct failure message for incomplete data', function (done) {
150-
mailer.transport.transportType.should.eql('DIRECT');
150+
mailer.transport.transporter.name.should.eql('SMTP (direct)');
151151

152152
mailer.send(mailDataIncomplete).then(function () {
153153
done(new Error('Error message not shown.'));
@@ -169,7 +169,7 @@ describe('Mail: Ghostmailer', function () {
169169
mailer = new mail.GhostMailer();
170170

171171
sandbox.stub(mailer, 'sendMail').resolves();
172-
mailer.transport.transportType = 'NOT DIRECT';
172+
mailer.transport.transporter.name = 'NOT DIRECT';
173173

174174
await mailer.send({
175175
to: '[email protected]',
@@ -184,7 +184,7 @@ describe('Mail: Ghostmailer', function () {
184184
beforeEach(async function () {
185185
mailer = new mail.GhostMailer();
186186
sandbox.stub(mailer, 'sendMail').resolves();
187-
mailer.transport.transportType = 'NOT DIRECT';
187+
mailer.transport.transporter.name = 'NOT DIRECT';
188188
sandbox.stub(settingsCache, 'get').returns('Test');
189189
});
190190

@@ -251,7 +251,7 @@ describe('Mail: Ghostmailer', function () {
251251
mailer = new mail.GhostMailer();
252252

253253
sandbox.stub(mailer, 'sendMail').resolves();
254-
mailer.transport.transportType = 'NOT DIRECT';
254+
mailer.transport.transporter.name = 'NOT DIRECT';
255255

256256
await mailer.send({
257257
to: '[email protected]',
@@ -270,7 +270,7 @@ describe('Mail: Ghostmailer', function () {
270270
mailer = new mail.GhostMailer();
271271

272272
sandbox.stub(mailer, 'sendMail').resolves();
273-
mailer.transport.transportType = 'NOT DIRECT';
273+
mailer.transport.transporter.name = 'NOT DIRECT';
274274

275275
await mailer.send({
276276
to: '[email protected]',
@@ -298,7 +298,7 @@ describe('Mail: Ghostmailer', function () {
298298
mailer = new mail.GhostMailer();
299299

300300
sandbox.stub(mailer, 'sendMail').resolves();
301-
mailer.transport.transportType = 'NOT DIRECT';
301+
mailer.transport.transporter.name = 'NOT DIRECT';
302302

303303
await mailer.send({
304304
to: '[email protected]',
@@ -326,7 +326,7 @@ describe('Mail: Ghostmailer', function () {
326326
mailer = new mail.GhostMailer();
327327

328328
sandbox.stub(mailer, 'sendMail').resolves();
329-
mailer.transport.transportType = 'NOT DIRECT';
329+
mailer.transport.transporter.name = 'NOT DIRECT';
330330

331331
await mailer.send({
332332
to: '[email protected]',

‎test/unit/services/mail/utils.test.js‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,19 @@
11
const should = require('should');
22
const sinon = require('sinon');
33
const mail = require('../../../../core/server/services/mail');
4+
const configUtils = require('../../../utils/configUtils');
45

56
describe('Mail: Utils', function () {
67
const scope = {ghostMailer: null};
78

89
beforeEach(function () {
10+
configUtils.set({mail: {transport: 'stub'}});
911
scope.ghostMailer = new mail.GhostMailer();
10-
11-
sinon.stub(scope.ghostMailer.transport, 'sendMail').callsFake(function (message, sendMailDone) {
12-
sendMailDone(null, {
13-
statusHandler: {
14-
once: function (eventName, eventDone) {
15-
if (eventName === 'sent') {
16-
eventDone();
17-
}
18-
}
19-
}
20-
});
21-
});
2212
});
2313

2414
afterEach(function () {
2515
sinon.restore();
16+
configUtils.restore();
2617
});
2718

2819
it('generate welcome', function (done) {

0 commit comments

Comments
 (0)