Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

prepend jsonp callbacks with a comment to prevent the rosetta-flash vulnerability #1766

Merged
merged 1 commit into from Jul 14, 2014
Merged

prepend jsonp callbacks with a comment to prevent the rosetta-flash vulnerability #1766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/response/payload.js
View file
Open in desktop
Expand Up @@ -53,7 +53,7 @@ internals.Payload.prototype.size = function () {
internals.Payload.prototype.jsonp = function (variable) {

this._sizeOffset += variable.length + 3;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ 7

this._prefix = variable + '(';
this._prefix = '/**/' + variable + '(';
this._data = Buffer.isBuffer(this._data) ? this._data : this._data.replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029');
this._suffix = ');';
};
6 changes: 3 additions & 3 deletions test/response.js
View file
Open in desktop
Expand Up @@ -1011,7 +1011,7 @@ describe('Response', function () {

server.inject('/?callback=me', function (res) {

expect(res.payload).to.equal('me({"some":"value"});');
expect(res.payload).to.equal('/**/me({"some":"value"});');
expect(res.headers['content-length']).to.equal(21);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON-P responses should also get an X-Content-Type-Options: nosniff header, just in case. This would have prevented the Rosetta Flash vulnerability too, in modern browsers, and it might prevent similar attacks that bypass the /**/-based protection in the future.

See https://gist.github.com/mathiasbynens/5547352 for an example in PHP (LOLWAT, I know).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That header can be added by setting

{
  security: {
    noSniff: true
  }
}

in your server options.

Or with several other security related headers by just setting { security: true }.

Just to remind people that this functionality does exist.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would make sense to enable this header by default for JSON-P responses?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a while I think @hueniverse was debating enabling all of the security headers by default. I'm not sure how he feels about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll submit another PR and we can find out :]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR

done();
});
Expand Down Expand Up @@ -1060,7 +1060,7 @@ describe('Response', function () {
Zlib.unzip(new Buffer(res.payload, 'binary'), function (err, result) {

expect(err).to.not.exist;
expect(result.toString()).to.equal('docall({"first":"1","last":"2"});');
expect(result.toString()).to.equal('/**/docall({"first":"1","last":"2"});');
done();
});
});
Expand All @@ -1078,7 +1078,7 @@ describe('Response', function () {

server.inject('/?callback=me', function (res) {

expect(res.payload).to.equal('me(value);');
expect(res.payload).to.equal('/**/me(value);');
expect(res.headers['content-length']).to.equal(10);
done();
});
Expand Down