Skip to content

Commit

Permalink
Merge pull request from GHSA-3fjj-p79j-c9hh
Browse files Browse the repository at this point in the history 
* fix: content-type spoofing

* refactor: improve performance
  • Loading branch information
@climba03003
climba03003 committed Nov 21, 2022
1 parent 6fc06c1 commit 62dde76
Show file tree
Hide file tree
Showing 4 changed files with 293 additions and 9 deletions.
81 changes: 75 additions & 6 deletions lib/contentTypeParser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

const { AsyncResource } = require('async_hooks')
const lru = require('tiny-lru').lru
// TODO: find more perforamant solution
const { parse: parseContentType } = require('content-type')

const secureJson = require('secure-json-parse')
const {
Expand Down Expand Up @@ -33,7 +35,7 @@ function ContentTypeParser (bodyLimit, onProtoPoisoning, onConstructorPoisoning)
this.customParsers = new Map()
this.customParsers.set('application/json', new Parser(true, false, bodyLimit, this[kDefaultJsonParse]))
this.customParsers.set('text/plain', new Parser(true, false, bodyLimit, defaultPlainTextParser))
this.parserList = ['application/json', 'text/plain']
this.parserList = [new ParserListItem('application/json'), new ParserListItem('text/plain')]
this.parserRegExpList = []
this.cache = lru(100)
}
Expand Down Expand Up @@ -66,7 +68,7 @@ ContentTypeParser.prototype.add = function (contentType, opts, parserFn) {
this.customParsers.set('', parser)
} else {
if (contentTypeIsString) {
this.parserList.unshift(contentType)
this.parserList.unshift(new ParserListItem(contentType))
} else {
this.parserRegExpList.unshift(contentType)
}
Expand Down Expand Up @@ -97,11 +99,20 @@ ContentTypeParser.prototype.getParser = function (contentType) {
const parser = this.cache.get(contentType)
if (parser !== undefined) return parser

const parsed = safeParseContentType(contentType)

// dummyContentType always the same object
// we can use === for the comparsion and return early
if (parsed === dummyContentType) {
return this.customParsers.get('')
}

// eslint-disable-next-line no-var
for (var i = 0; i !== this.parserList.length; ++i) {
const parserName = this.parserList[i]
if (contentType.indexOf(parserName) !== -1) {
const parser = this.customParsers.get(parserName)
const parserListItem = this.parserList[i]
if (compareContentType(parsed, parserListItem)) {
const parser = this.customParsers.get(parserListItem.name)
// we set request content-type in cache to reduce parsing of MIME type
this.cache.set(contentType, parser)
return parser
}
Expand All @@ -110,8 +121,9 @@ ContentTypeParser.prototype.getParser = function (contentType) {
// eslint-disable-next-line no-var
for (var j = 0; j !== this.parserRegExpList.length; ++j) {
const parserRegExp = this.parserRegExpList[j]
if (parserRegExp.test(contentType)) {
if (compareRegExpContentType(contentType, parsed.type, parserRegExp)) {
const parser = this.customParsers.get(parserRegExp.toString())
// we set request content-type in cache to reduce parsing of MIME type
this.cache.set(contentType, parser)
return parser
}
Expand Down Expand Up @@ -346,6 +358,63 @@ function removeAllContentTypeParsers () {
this[kContentTypeParser].removeAll()
}

// dummy here to prevent repeated object creation
const dummyContentType = { type: '', parameters: Object.create(null) }

function safeParseContentType (contentType) {
try {
return parseContentType(contentType)
} catch (err) {
return dummyContentType
}
}

function compareContentType (contentType, parserListItem) {
if (parserListItem.isEssence) {
// we do essence check
return contentType.type.indexOf(parserListItem) !== -1
} else {
// when the content-type includes parameters
// we do a full-text search
// reject essence content-type before checking parameters
if (contentType.type.indexOf(parserListItem.type) === -1) return false
for (const key of parserListItem.parameterKeys) {
// reject when missing parameters
if (!(key in contentType.parameters)) return false
// reject when parameters do not match
if (contentType.parameters[key] !== parserListItem.parameters[key]) return false
}
return true
}
}

function compareRegExpContentType (contentType, essenceMIMEType, regexp) {
if (regexp.source.indexOf(';') === -1) {
// we do essence check
return regexp.test(essenceMIMEType)
} else {
// when the content-type includes parameters
// we do a full-text match
return regexp.test(contentType)
}
}

function ParserListItem (contentType) {
this.name = contentType
// we pre-calculate all the needed information
// before content-type comparsion
const parsed = safeParseContentType(contentType)
this.type = parsed.type
this.parameters = parsed.parameters
this.parameterKeys = Object.keys(parsed.parameters)
this.isEssence = contentType.indexOf(';') === -1
}

// used in ContentTypeParser.remove
ParserListItem.prototype.toString = function () {
return this.name
}

module.exports = ContentTypeParser
module.exports.helpers = {
buildContentTypeParser,
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@
"@fastify/fast-json-stringify-compiler": "^4.1.0",
"abstract-logging": "^2.0.1",
"avvio": "^8.2.0",
"content-type": "^1.0.4",
"find-my-way": "^7.3.0",
"light-my-request": "^5.6.1",
"pino": "^8.5.0",
Expand Down
214 changes: 214 additions & 0 deletions test/content-parser.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,3 +395,217 @@ test('Safeguard against malicious content-type / 3', async t => {

t.same(response.statusCode, 415)
})

test('Safeguard against content-type spoofing - string', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('text/plain', function (request, body, done) {
t.pass('should be called')
done(null, body)
})
fastify.addContentTypeParser('application/json', function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'text/plain; content-type="application/json"'
},
body: ''
})
})

test('Safeguard against content-type spoofing - regexp', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser(/text\/plain/, function (request, body, done) {
t.pass('should be called')
done(null, body)
})
fastify.addContentTypeParser(/application\/json/, function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'text/plain; content-type="application/json"'
},
body: ''
})
})

test('content-type match parameters - string 1', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('text/plain; charset=utf8', function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})
fastify.addContentTypeParser('application/json; charset=utf8', function (request, body, done) {
t.pass('should be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; charset=utf8'
},
body: ''
})
})

test('content-type match parameters - string 2', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('application/json; charset=utf8; foo=bar', function (request, body, done) {
t.pass('should be called')
done(null, body)
})
fastify.addContentTypeParser('text/plain; charset=utf8; foo=bar', function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; foo=bar; charset=utf8'
},
body: ''
})
})

test('content-type match parameters - regexp', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser(/application\/json; charset=utf8/, function (request, body, done) {
t.pass('should be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; charset=utf8'
},
body: ''
})
})

test('content-type fail when parameters not match - string 1', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('application/json; charset=utf8; foo=bar', function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

const response = await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; charset=utf8'
},
body: ''
})

t.same(response.statusCode, 415)
})

test('content-type fail when parameters not match - string 2', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('application/json; charset=utf8; foo=bar', function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

const response = await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; charset=utf8; foo=baz'
},
body: ''
})

t.same(response.statusCode, 415)
})

test('content-type fail when parameters not match - regexp', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser(/application\/json; charset=utf8; foo=bar/, function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

const response = await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; charset=utf8'
},
body: ''
})

t.same(response.statusCode, 415)
})
6 changes: 3 additions & 3 deletions test/custom-parser.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1053,7 +1053,7 @@ test('The charset should not interfere with the content type handling', t => {
url: getUrl(fastify),
body: '{"hello":"world"}',
headers: {
'Content-Type': 'application/json charset=utf-8'
'Content-Type': 'application/json; charset=utf-8'
}
}, (err, response, body) => {
t.error(err)
Expand Down Expand Up @@ -1236,7 +1236,7 @@ test('contentTypeParser should add a custom parser with RegExp value', t => {
url: getUrl(fastify),
body: '{"hello":"world"}',
headers: {
'Content-Type': 'weird-content-type+json'
'Content-Type': 'weird/content-type+json'
}
}, (err, response, body) => {
t.error(err)
Expand Down Expand Up @@ -1266,7 +1266,7 @@ test('contentTypeParser should add multiple custom parsers with RegExp values',
done(null, 'xml')
})

fastify.addContentTypeParser(/.*\+myExtension$/, function (req, payload, done) {
fastify.addContentTypeParser(/.*\+myExtension$/i, function (req, payload, done) {
let data = ''
payload.on('data', chunk => { data += chunk })
payload.on('end', () => {
Expand Down

0 comments on commit 62dde76

Please sign in to comment.