Potential DOM Based Cross Site Scripting in gmail.js

[gursev]

The gmail.js library dynamically creates functions out of response data and executes them, leading to potential DOM based XSS issues.

Instance 1:

  api.tools.parse_response = function(response) {
      var parsedResponse = [],
          data, dataLength, endIndex, realData;

      try {

        // gmail post response structure
        // )}]'\n<datalength><rawData>\n<dataLength><rawData>...

        // prepare response, remove eval protectors
        response = response.replace(/\n/g, ' ');
        response = response.substring(response.indexOf("'") + 1, response.length);

        while(response.replace(/\s/g, '').length > 1) {

          // how long is the data to get
          dataLength = response.substring(0, response.indexOf('[')).replace(/\s/g, '');
          if (!dataLength) {dataLength = response.length;}

          endIndex = (parseInt(dataLength, 10) - 2) + response.indexOf('[');
          data = response.substring(response.indexOf('['), endIndex);

          var get_data = new Function('"use strict"; return ' + data); // DOM XSS when data is 'alert(1);'

Instance 2:

  api.helper.get.visible_emails_post = function(get_data) {
    var emails = [];

    var get_data = get_data.substring(get_data.indexOf('['), get_data.length);
        get_data = '"use strict"; return ' + get_data;
        get_data = new Function(get_data); // DOM XSS when data is 'alert(1);'

Instance 3:

  api.helper.get.email_data_post = function(get_data) {
    var get_data = get_data.substring(get_data.indexOf('['), get_data.length);
        get_data = '"use strict"; return ' + get_data;
        get_data = new Function(get_data); // DOM XSS when data is 'alert(1);'

@KartikTalwar

[gursev]

@KartikTalwar Can you please review this and provide your feedback? Also, if possible, can you please add security label to this bug?

[gravis]

Any chance to see this addressed?

[josteink]

It's been suggested to me that we could use JSON.parse instead of creating a new Function-object, and in general that seems reasonable.

Based on the extremely limited testing I've done with api.tools.parse_response, this works for me.

To facilitate more thorough testing, I can create a feature-branch where all use of new Function() is replaced by JSON.parse. Interested parties can then test this code and report back if that works for them.

If so, we can merge this back to master. Sounds like a plan?

[josteink]

And consider it done!

I've created some basic unit-tests for data-parsing on all functions where new Function() is used to process incoming data. On the current master-branch, all tests pass.

I've also created a new feature-branch called jsonparse where all instances of new Function() is replaced with JSON.parse(). On this branch too, all tests currently pass.

So I'm decently confident that this will be a non-breaking change, but I would appreciate it if you guys would be willing test it first, and report back if you have any issues or not before we merge to master.

Ladies and gentlemen, the fate of this issue now lies in your hands. Make the most of it :)

[josteink]

Fixed in a83436f.

Closing.