476825 - TwitterFox extension evaluates JSON data in an unsafe way

Status: RESOLVED FIXED

(addons.mozilla.org :: Security, defect)

Description

[Wladimir Palant]

Looking at the method onLoad() in the file components/nsTwitterNotifier.js, I see the following handling for JSON data received from twitter.com:

    case 200:
    case 304:
    default:
      var resp = null;
      if (!req.responseText.match(/^\s*$/)) {
        req.responseText = req.responseText.replace(/Couldn\'t find Status with ID=\d+,/, '');
        try {
          var resp = eval('(' + req.responseText + ')');
        }
        catch (e) {
          this.reportError("An error occurred while requesting " + req.__url);
          this.log("Response text: " + e.message);
          this.handleError(req, "Can't parse JSON. Twitter server responded with an error.");
        }
      }

At least all connections use HTTPS. Still, a malicious response from twitter.com (e.g. if the server is hacked) can take over user's browser. Similar JSON handling can also be found in the method rateLimitExceeded(). For
reference: https://developer.mozilla.org/en/JSON#Using_JSON

Comment 1

[Rey Bango]

Developer has been emailed

Comment 2

[Kazuho Okui]

Thanks, I'll taking care of it and release fixed version ASAP.

Comment 3

[Kazuho Okui]

Is there any safe way to decode JSON in Firefox 2.x? Or should I drop to support Fx2?

Comment 4

[Rey Bango]

Thanks for the quick response Kazuho. This may help:

https://developer.mozilla.org/En/Components.utils.evalInSandbox

Comment 5

[Justin Scott [:fligtar]]

You can also use the JSON parser script from http://json.org/js.html

Comment 6

[Kazuho Okui]

I just want to make sure that this is correct to fix the issue.

I changed the code from:

    var resp = eval('(' + req.responseText + ')');

to:

    var s = Components.utils.Sandbox("https://twitter.com/");
    var resp = Components.utils.evalInSandbox(req.responseText, s);

Comment 7

[Wladimir Palant]

This approach is a possibility but it won't prevent code execution, only move it to the sandbox. As a result, you will have to take quite some precautions when working with the resp variable, as outlined in the evalInSandbox documentation. I think using the code from JSON.jsm (correct link is http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/loader/JSON.jsm, I changed it in the documentation as well) would be a better solution.

Comment 8

[Kazuho Okui]

Thanks, I updated my extension, waiting reviewer's approval.

https://addons.mozilla.org/en-US/firefox/addons/versions/5081#version-1.7.5

Source codef changes is here:

http://www.naan.net/trac/changeset/1679/branches/TwitterFox/1.7.3/components/nsTwitterNotifier.js

Comment 9

[Wladimir Palant]

Thanks, these changes look good. There is another eval() call there but from what I can tell it is only used for data exchanged between different extension components.

Comment 10

[Daniel Veditz [:dveditz]]

This appears fixed

Comment 11

[Wladimir Palant]

I think that this should be public by now.