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

Merged
merged 1 commit into from Jul 14, 2014

Conversation

Projects
None yet
4 participants
@patrickkettner
Member

patrickkettner commented Jul 8, 2014

background

tl:dr - someone created a alphanum only swf converter, which means that they can in theory use it as a callback at a JSONP endpoint, and as a result, send data across domains.

Prepending callbacks with an empty inline comment breaks the flash parser, and prevents the issue. This is how google, facebook, github, et al are handeling it.

CVE-2014-4671A

@hueniverse hueniverse added the security label Jul 14, 2014

@hueniverse hueniverse added this to the 6.1.0 milestone Jul 14, 2014

@hueniverse hueniverse self-assigned this Jul 14, 2014

hueniverse added a commit that referenced this pull request Jul 14, 2014

Merge pull request #1766 from patrickkettner/rosetta-flash
prepend jsonp callbacks with a comment to prevent the rosetta-flash vulnerability

@hueniverse hueniverse merged commit d47f57a into hapijs:master Jul 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@patrickkettner patrickkettner deleted the patrickkettner:rosetta-flash branch Jul 14, 2014

hueniverse added a commit that referenced this pull request Jul 14, 2014

@@ -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);

This comment has been minimized.

@mathiasbynens

mathiasbynens Jul 21, 2014

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).

@mathiasbynens

mathiasbynens Jul 21, 2014

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).

This comment has been minimized.

@nlf

nlf Jul 21, 2014

Member

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.

@nlf

nlf Jul 21, 2014

Member

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.

This comment has been minimized.

@mathiasbynens

mathiasbynens Jul 21, 2014

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

@mathiasbynens

mathiasbynens Jul 21, 2014

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

This comment has been minimized.

@nlf

nlf Jul 21, 2014

Member

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.

@nlf

nlf Jul 21, 2014

Member

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.

This comment has been minimized.

@patrickkettner

patrickkettner Jul 21, 2014

Member

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

@patrickkettner

patrickkettner Jul 21, 2014

Member

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

This comment has been minimized.

@@ -53,7 +53,7 @@ internals.Payload.prototype.size = function () {
internals.Payload.prototype.jsonp = function (variable) {
this._sizeOffset += variable.length + 3;

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment