From 64ed246d03a732da18dc94744c921212eaf49ab8 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Tue, 2 Jun 2020 14:30:10 +0100 Subject: [PATCH] Merge pull request from GHSA-4m2q-w26j-h268 no issue - added an `externalRequest` lib - uses same underlying `got` module as our `request` lib - uses `got`'s `beforeRequest` and `beforeRedirect` hooks to perform it's own dns resolution for each url that's encountered and aborts with an error if it resolves to a private IP address block - includes a bypass for Ghost's configured url so that requests to it's own hostname+port are not blocked - updated v2 and canary oembed controllers to use the `externalRequest` lib --- core/server/api/canary/oembed.js | 25 +- core/server/api/v2/oembed.js | 17 +- core/server/lib/request-external.js | 60 ++++ test/api-acceptance/admin/oembed_spec.js | 184 +++++++++++++ test/unit/lib/external-request_spec.js | 332 +++++++++++++++++++++++ 5 files changed, 590 insertions(+), 28 deletions(-) create mode 100644 core/server/lib/request-external.js create mode 100644 test/unit/lib/external-request_spec.js diff --git a/core/server/api/canary/oembed.js b/core/server/api/canary/oembed.js index 9ee2365818..d32f11736f 100644 --- a/core/server/api/canary/oembed.js +++ b/core/server/api/canary/oembed.js @@ -2,7 +2,7 @@ const {i18n} = require('../../lib/common'); const errors = require('@tryghost/errors'); const {extract, hasProvider} = require('oembed-parser'); const Promise = require('bluebird'); -const request = require('../../lib/request'); +const externalRequest = require('../../lib/request-external'); const cheerio = require('cheerio'); const _ = require('lodash'); @@ -22,11 +22,7 @@ async function fetchBookmarkData(url, html) { try { if (!html) { - const response = await request(url, { - headers: { - 'user-agent': 'Ghost(https://github.com/TryGhost/Ghost)' - } - }); + const response = await externalRequest(url); html = response.body; } scraperResponse = await metascraper({html, url}); @@ -97,7 +93,7 @@ function isIpOrLocalhost(url) { const IPV6_REGEX = /:/; // fqdns will not have colons const HTTP_REGEX = /^https?:/i; - let {protocol, hostname} = new URL(url); + const {protocol, hostname} = new URL(url); if (!HTTP_REGEX.test(protocol) || hostname === 'localhost' || IPV4_REGEX.test(hostname) || IPV6_REGEX.test(hostname)) { return true; @@ -125,13 +121,10 @@ function fetchOembedData(_url) { // url not in oembed list so fetch it in case it's a redirect or has a // element - return request(url, { + return externalRequest(url, { method: 'GET', timeout: 2 * 1000, - followRedirect: true, - headers: { - 'user-agent': 'Ghost(https://github.com/TryGhost/Ghost)' - } + followRedirect: true }).then((response) => { // url changed after fetch, see if we were redirected to a known oembed if (response.url !== url) { @@ -156,13 +149,11 @@ function fetchOembedData(_url) { } // fetch oembed response from embedded rel="alternate" url - return request(oembedUrl, { + return externalRequest(oembedUrl, { method: 'GET', json: true, timeout: 2 * 1000, - headers: { - 'user-agent': 'Ghost(https://github.com/TryGhost/Ghost)' - } + followRedirect: true }).then((response) => { // validate the fetched json against the oembed spec to avoid // leaking non-oembed responses @@ -234,7 +225,7 @@ module.exports = { return unknownProvider(url); } return response; - }).catch(() => { + }).catch((err) => { return unknownProvider(url); }); } diff --git a/core/server/api/v2/oembed.js b/core/server/api/v2/oembed.js index 2f8054ad69..6ff8928411 100644 --- a/core/server/api/v2/oembed.js +++ b/core/server/api/v2/oembed.js @@ -2,7 +2,7 @@ const {i18n} = require('../../lib/common'); const errors = require('@tryghost/errors'); const {extract, hasProvider} = require('oembed-parser'); const Promise = require('bluebird'); -const request = require('../../lib/request'); +const externalRequest = require('../../lib/request-external'); const cheerio = require('cheerio'); const _ = require('lodash'); @@ -51,7 +51,7 @@ function isIpOrLocalhost(url) { const IPV6_REGEX = /:/; // fqdns will not have colons const HTTP_REGEX = /^https?:/i; - let {protocol, hostname} = new URL(url); + const {protocol, hostname} = new URL(url); if (!HTTP_REGEX.test(protocol) || hostname === 'localhost' || IPV4_REGEX.test(hostname) || IPV6_REGEX.test(hostname)) { return true; @@ -79,13 +79,10 @@ function fetchOembedData(_url) { // url not in oembed list so fetch it in case it's a redirect or has a // element - return request(url, { + return externalRequest(url, { method: 'GET', timeout: 2 * 1000, - followRedirect: true, - headers: { - 'user-agent': 'Ghost(https://github.com/TryGhost/Ghost)' - } + followRedirect: true }).then((response) => { // url changed after fetch, see if we were redirected to a known oembed if (response.url !== url) { @@ -110,13 +107,11 @@ function fetchOembedData(_url) { } // fetch oembed response from embedded rel="alternate" url - return request(oembedUrl, { + return externalRequest(oembedUrl, { method: 'GET', json: true, timeout: 2 * 1000, - headers: { - 'user-agent': 'Ghost(https://github.com/TryGhost/Ghost)' - } + followRedirect: true }).then((response) => { // validate the fetched json against the oembed spec to avoid // leaking non-oembed responses diff --git a/core/server/lib/request-external.js b/core/server/lib/request-external.js new file mode 100644 index 0000000000..40221fb677 --- /dev/null +++ b/core/server/lib/request-external.js @@ -0,0 +1,60 @@ +const got = require('got'); +const dnsPromises = require('dns').promises; +const errors = require('@tryghost/errors'); +const ghostVersion = require('./ghost-version'); +const config = require('../../shared/config'); +const validator = require('../data/validation').validator; + +function isPrivateIp(addr) { + return /^(::f{4}:)?10\.([0-9]{1,3})\.([0-9]{1,3})\.([0-9]{1,3})$/i.test(addr) || + /^(::f{4}:)?192\.168\.([0-9]{1,3})\.([0-9]{1,3})$/i.test(addr) || + /^(::f{4}:)?172\.(1[6-9]|2\d|30|31)\.([0-9]{1,3})\.([0-9]{1,3})$/i.test(addr) || + /^(::f{4}:)?127\.([0-9]{1,3})\.([0-9]{1,3})\.([0-9]{1,3})$/i.test(addr) || + /^(::f{4}:)?169\.254\.([0-9]{1,3})\.([0-9]{1,3})$/i.test(addr) || + /^f[cd][0-9a-f]{2}:/i.test(addr) || + /^fe80:/i.test(addr) || + /^::1$/.test(addr) || + /^::$/.test(addr); +} + +async function errorIfHostnameResolvesToPrivateIp(options) { + // allow requests through to local Ghost instance + const siteUrl = new URL(config.get('url')); + const requestUrl = new URL(options.href); + if (requestUrl.host === siteUrl.host) { + return Promise.resolve(); + } + + const result = await dnsPromises.lookup(options.hostname); + + if (isPrivateIp(result.address)) { + return Promise.reject(new errors.InternalServerError({ + message: 'URL resolves to a non-permitted private IP block', + code: 'URL_PRIVATE_INVALID', + context: options.href + })); + } +} + +// same as our normal request lib but if any request in a redirect chain resolves +// to a private IP address it will be blocked before the request is made. +const externalRequest = got.extend({ + headers: { + 'user-agent': 'Ghost/' + ghostVersion.original + ' (https://github.com/TryGhost/Ghost)' + }, + hooks: { + init: [(options) => { + if (!options.hostname || !validator.isURL(options.hostname)) { + throw new errors.InternalServerError({ + message: 'URL empty or invalid.', + code: 'URL_MISSING_INVALID', + context: options.href + }); + } + }], + beforeRequest: [errorIfHostnameResolvesToPrivateIp], + beforeRedirect: [errorIfHostnameResolvesToPrivateIp] + } +}); + +module.exports = externalRequest; diff --git a/test/api-acceptance/admin/oembed_spec.js b/test/api-acceptance/admin/oembed_spec.js index 4eedd91160..2201bcdc1c 100644 --- a/test/api-acceptance/admin/oembed_spec.js +++ b/test/api-acceptance/admin/oembed_spec.js @@ -1,10 +1,14 @@ const nock = require('nock'); +const sinon = require('sinon'); const should = require('should'); const supertest = require('supertest'); const testUtils = require('../../utils/index'); const config = require('../../../core/shared/config/index'); const localUtils = require('./utils'); +// for sinon stubs +const dnsPromises = require('dns').promises; + const ghost = testUtils.startGhost; describe('Oembed API', function () { @@ -22,6 +26,10 @@ describe('Oembed API', function () { }); }); + afterEach(function () { + sinon.restore(); + }); + it('can fetch an embed', function (done) { let requestMock = nock('https://www.youtube.com') .get('/oembed') @@ -59,6 +67,39 @@ describe('Oembed API', function () { }); describe('with unknown provider', function () { + it('fetches url and follows redirects', function (done) { + const redirectMock = nock('http://test.com/') + .get('/') + .reply(302, undefined, {Location: 'http://oembed.test.com'}); + + const pageMock = nock('http://oembed.test.com') + .get('/') + .reply(200, ''); + + const oembedMock = nock('http://oembed.test.com') + .get('/my-embed') + .reply(200, { + version: '1.0', + type: 'link' + }); + + const url = encodeURIComponent('http://test.com/'); + request.get(localUtils.API.getApiQuery(`oembed/?url=${url}`)) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + redirectMock.isDone().should.be.true(); + pageMock.isDone().should.be.true(); + oembedMock.isDone().should.be.true(); + done(); + }); + }); + it('fetches url and follows ', function (done) { const pageMock = nock('http://test.com') .get('/') @@ -87,6 +128,39 @@ describe('Oembed API', function () { }); }); + it('follows redirects when fetching ', function (done) { + const pageMock = nock('http://test.com') + .get('/') + .reply(200, ''); + + const alternateRedirectMock = nock('http://test.com') + .get('/oembed') + .reply(301, undefined, {Location: 'http://test.com/oembed-final'}); + + const alternateMock = nock('http://test.com') + .get('/oembed-final') + .reply(200, { + version: '1.0', + type: 'link' + }); + + const url = encodeURIComponent('http://test.com'); + request.get(localUtils.API.getApiQuery(`oembed/?url=${url}`)) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + pageMock.isDone().should.be.true(); + alternateRedirectMock.isDone().should.be.true(); + alternateMock.isDone().should.be.true(); + done(); + }); + }); + it('rejects invalid oembed responses', function (done) { const pageMock = nock('http://test.com') .get('/') @@ -330,5 +404,115 @@ describe('Oembed API', function () { done(); }); }); + + it('skips fetching url that resolves to private IP', function (done) { + sinon.stub(dnsPromises, 'lookup').callsFake(function (hostname) { + if (hostname === 'page.com') { + return Promise.resolve({address: '192.168.0.1'}); + } + return dnsPromises.lookup.wrappedMethod.apply(this, arguments); + }); + + const pageMock = nock('http://page.com') + .get('/') + .reply(200, ''); + + const oembedMock = nock('http://oembed.com') + .get('/oembed') + .reply(200, { + version: '1.0', + type: 'link' + }); + + const url = encodeURIComponent('http://page.com'); + request.get(localUtils.API.getApiQuery(`oembed/?url=${url}`)) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(422) + .end(function (err, res) { + if (err) { + return done(err); + } + pageMock.isDone().should.be.false(); + oembedMock.isDone().should.be.false(); + done(); + }); + }); + + it('aborts fetching if a redirect resolves to private IP', function (done) { + sinon.stub(dnsPromises, 'lookup').callsFake(function (hostname) { + if (hostname === 'page.com') { + return Promise.resolve({address: '192.168.0.1'}); + } + return dnsPromises.lookup.wrappedMethod.apply(this, arguments); + }); + + const redirectMock = nock('http://redirect.com') + .get('/') + .reply(301, undefined, {Location: 'http://page.com'}); + + const pageMock = nock('http://page.com') + .get('/') + .reply(200, ''); + + const oembedMock = nock('http://oembed.com') + .get('/oembed') + .reply(200, { + version: '1.0', + type: 'link' + }); + + const url = encodeURIComponent('http://redirect.com'); + request.get(localUtils.API.getApiQuery(`oembed/?url=${url}`)) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(422) + .end(function (err, res) { + if (err) { + return done(err); + } + redirectMock.isDone().should.be.true(); + pageMock.isDone().should.be.false(); + oembedMock.isDone().should.be.false(); + done(); + }); + }); + + it('skips fetching if it resolves to a private IP', function (done) { + sinon.stub(dnsPromises, 'lookup').callsFake(function (hostname) { + if (hostname === 'oembed.com') { + return Promise.resolve({address: '192.168.0.1'}); + } + return dnsPromises.lookup.wrappedMethod.apply(this, arguments); + }); + + const pageMock = nock('http://page.com') + .get('/') + .reply(200, ''); + + const oembedMock = nock('http://oembed.com') + .get('/oembed') + .reply(200, { + version: '1.0', + type: 'link' + }); + + const url = encodeURIComponent('http://page.com'); + request.get(localUtils.API.getApiQuery(`oembed/?url=${url}`)) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(422) + .end(function (err, res) { + if (err) { + return done(err); + } + pageMock.isDone().should.be.true(); + oembedMock.isDone().should.be.false(); + done(); + }); + }); }); }); diff --git a/test/unit/lib/external-request_spec.js b/test/unit/lib/external-request_spec.js new file mode 100644 index 0000000000..bf05ef3d79 --- /dev/null +++ b/test/unit/lib/external-request_spec.js @@ -0,0 +1,332 @@ +const sinon = require('sinon'); +const should = require('should'); +const rewire = require('rewire'); +const nock = require('nock'); +const externalRequest = rewire('../../../core/server/lib/request-external'); +const configUtils = require('../../utils/configUtils'); + +// for sinon stubs +const dnsPromises = require('dns').promises; + +describe('External Request', function () { + describe('with private ip', function () { + beforeEach(function () { + sinon.stub(dnsPromises, 'lookup').callsFake(function () { + return Promise.resolve({address: '192.168.0.1'}); + }); + }); + + afterEach(function () { + configUtils.restore(); + sinon.restore(); + nock.cleanAll(); + }); + + it('allows configured hostname', function () { + configUtils.set('url', 'http://example.com'); + + const url = 'http://example.com/endpoint/'; + const expectedResponse = { + body: 'Response body', + url: 'http://example.com/endpoint/', + statusCode: 200 + }; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://example.com') + .get('/endpoint/') + .reply(200, 'Response body'); + + return externalRequest(url, options).then(function (res) { + requestMock.isDone().should.be.true(); + should.exist(res); + should.exist(res.body); + res.body.should.be.equal(expectedResponse.body); + should.exist(res.url); + res.statusCode.should.be.equal(expectedResponse.statusCode); + should.exist(res.statusCode); + res.url.should.be.equal(expectedResponse.url); + }); + }); + + it('allows configured hostname+port', function () { + configUtils.set('url', 'http://example.com:2368'); + + const url = 'http://example.com:2368/endpoint/'; + const expectedResponse = { + body: 'Response body', + url: 'http://example.com:2368/endpoint/', + statusCode: 200 + }; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://example.com:2368') + .get('/endpoint/') + .reply(200, 'Response body'); + + return externalRequest(url, options).then(function (res) { + requestMock.isDone().should.be.true(); + should.exist(res); + should.exist(res.body); + res.body.should.be.equal(expectedResponse.body); + should.exist(res.url); + res.statusCode.should.be.equal(expectedResponse.statusCode); + should.exist(res.statusCode); + res.url.should.be.equal(expectedResponse.url); + }); + }); + + it('blocks configured hostname with incorrect port', function () { + configUtils.set('url', 'http://example.com'); + + const url = 'http://example.com:1234/endpoint/'; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + return externalRequest(url, options).then(() => { + throw new Error('Request should have rejected with non-permitted IP message'); + }, (err) => { + should.exist(err); + err.message.should.be.equal('URL resolves to a non-permitted private IP block'); + }); + }); + + it('blocks configured hostname+port with incorrect port', function () { + configUtils.set('url', 'http://example.com:2368'); + + const url = 'http://example.com:1234/endpoint/'; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + return externalRequest(url, options).then(() => { + throw new Error('Request should have rejected with non-permitted IP message'); + }, (err) => { + should.exist(err); + err.message.should.be.equal('URL resolves to a non-permitted private IP block'); + }); + }); + + it('blocks on request', function () { + const url = 'http://some-website.com/'; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://some-website.com') + .get('/files/') + .reply(200, 'Response'); + + return externalRequest(url, options).then(function () { + throw new Error('Request should have rejected with non-permitted IP message'); + }, (err) => { + should.exist(err); + err.message.should.be.equal('URL resolves to a non-permitted private IP block'); + requestMock.isDone().should.be.false(); + }); + }); + + it('blocks on redirect', function () { + configUtils.set('url', 'http://some-website.com'); + + const url = 'http://some-website.com/endpoint/'; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://some-website.com') + .get('/endpoint/') + .reply(301, 'Oops, got redirected', + { + location: 'http://someredirectedurl.com/files/' + }); + + const secondRequestMock = nock('http://someredirectedurl.com') + .get('/files/') + .reply(200, 'Redirected response'); + + return externalRequest(url, options).then(function () { + throw new Error('Request should have rejected with non-permitted IP message'); + }, (err) => { + should.exist(err); + err.message.should.be.equal('URL resolves to a non-permitted private IP block'); + requestMock.isDone().should.be.true(); + secondRequestMock.isDone().should.be.false(); + }); + }); + }); + + describe('general behaviour', function () { + beforeEach(function () { + sinon.stub(dnsPromises, 'lookup').callsFake(function (host) { + return Promise.resolve({address: '123.123.123.123'}); + }); + }); + + afterEach(function () { + configUtils.restore(); + sinon.restore(); + nock.cleanAll(); + }); + + it('[success] should return response for http request', function () { + const url = 'http://some-website.com/endpoint/'; + const expectedResponse = { + body: 'Response body', + url: 'http://some-website.com/endpoint/', + statusCode: 200 + }; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://some-website.com') + .get('/endpoint/') + .reply(200, 'Response body'); + + return externalRequest(url, options).then(function (res) { + requestMock.isDone().should.be.true(); + should.exist(res); + should.exist(res.body); + res.body.should.be.equal(expectedResponse.body); + should.exist(res.url); + res.statusCode.should.be.equal(expectedResponse.statusCode); + should.exist(res.statusCode); + res.url.should.be.equal(expectedResponse.url); + }); + }); + + it('[success] can handle redirect', function () { + const url = 'http://some-website.com/endpoint/'; + const expectedResponse = { + body: 'Redirected response', + url: 'http://someredirectedurl.com/files/', + statusCode: 200 + }; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://some-website.com') + .get('/endpoint/') + .reply(301, 'Oops, got redirected', + { + location: 'http://someredirectedurl.com/files/' + }); + + const secondRequestMock = nock('http://someredirectedurl.com') + .get('/files/') + .reply(200, 'Redirected response'); + + return externalRequest(url, options).then(function (res) { + requestMock.isDone().should.be.true(); + secondRequestMock.isDone().should.be.true(); + should.exist(res); + should.exist(res.body); + res.body.should.be.equal(expectedResponse.body); + should.exist(res.url); + res.statusCode.should.be.equal(expectedResponse.statusCode); + should.exist(res.statusCode); + res.url.should.be.equal(expectedResponse.url); + }); + }); + + it('[failure] can handle invalid url', function () { + const url = 'test'; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + return externalRequest(url, options).then(() => { + throw new Error('Request should have rejected with invalid url message'); + }, (err) => { + should.exist(err); + err.message.should.be.equal('URL empty or invalid.'); + }); + }); + + it('[failure] can handle empty url', function () { + const url = ''; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + return externalRequest(url, options).then(() => { + throw new Error('Request should have rejected with invalid url message'); + }, (err) => { + should.exist(err); + err.message.should.be.equal('URL empty or invalid.'); + }); + }); + + it('[failure] can handle an error with statuscode not 200', function () { + const url = 'http://nofilehere.com/files/test.txt'; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://nofilehere.com') + .get('/files/test.txt') + .reply(404); + + return externalRequest(url, options).then(() => { + throw new Error('Request should have errored'); + }, (err) => { + requestMock.isDone().should.be.true(); + should.exist(err); + err.statusMessage.should.be.equal('Not Found'); + }); + }); + + it('[failure] returns error if request errors', function () { + const url = 'http://nofilehere.com/files/test.txt'; + const options = { + headers: { + 'User-Agent': 'Mozilla/5.0' + } + }; + + const requestMock = nock('http://nofilehere.com') + .get('/files/test.txt') + .times(3) // 1 original request + 2 default retries + .reply(500, {message: 'something awful happened', code: 'AWFUL_ERROR'}); + + return externalRequest(url, options).then(() => { + throw new Error('Request should have errored with an awful error'); + }, (err) => { + requestMock.isDone().should.be.true(); + should.exist(err); + err.statusMessage.should.be.equal('Internal Server Error'); + err.body.should.match(/something awful happened/); + err.body.should.match(/AWFUL_ERROR/); + }); + }); + }); +});