672199 - Module exports must be frozen

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Module exports must be frozen to avoid attacks were malicious code takes over by overriding exported properties!

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/214

Pointer to Github pull-request

Comment 2

[Brian Warner [:warner :bwarner]]

Comment on attachment 546578 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/214

Looks ok to me: I like the idea of freezing these exports.

Do we need to worry about other callers (i.e. user code) using api-utils/keyboard/observer and being affected by the API change this introduces? I'd guess no, but wanted to be sure.

We should probably mention the freeze() in the how-to-write-a-module docs. Also it might be nice to have a quick test (just confirm that something like require("foo").newprop=123 throws an exception).

r+ with doc and test.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> Do we need to worry about other callers (i.e. user code) using
> api-utils/keyboard/observer and being affected by the API change this
> introduces? I'd guess no, but wanted to be sure.

That's low level API so our users are aware that they may change at any point. If you refer to our own modules, no other module depends on those and all tests pass.

> We should probably mention the freeze() in the how-to-write-a-module docs.

CC-ing Will in order to get his review on docs side.

> Also it might be nice to have a quick test (just confirm that something like > require("foo").newprop=123

Yeap, good point I'll update pull requests by adding such tests!

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

In fact, I think it's better to tackle documentation improvement, under a separate, Bug 674191

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It looks like this patch does not works on nightly, for some reason freezing plain-text-console throws:

TypeError: can't change object's extensibility

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Given the fact that we run into Bug 674195 with this change we can't land this yet, also I'd suggest landing just an API changes to the low level modules that we know are going to break with freeze. Later once platform bugs are fixed we can can go ahead with a freeze.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 546578 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/214

I have updated patch with a workaround for Bug 674195. Warner can you please take another look at it ?

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Please note that I created separate bug for suggested doc changes, as they don't necessary need to block each other

Comment 9

[Brian Warner [:warner :bwarner]]

hm, another workaround might be to append "Object.freeze(exports)" to each module's source string just before we evalInSandbox it, to have them freeze themselves. Modules might be able to dodge that (maybe redefining 'Object' or 'exports' to cause the freeze() to do something different), but I think that's ok: what we really care about is protecting modules from outsiders, even if they didn't think to do the freeze() themselves.

I'd like to see the docs land quickly: I'm worried about users doing something weird (which violates the freeze and throws an exception), and then not being able to find out why their exports are frozen when they didn't do an explicit freeze() (and when none of the .js modules do it either).

I'm completely cool with landing just the low-level API changes now, and landing the freeze() later.

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 546578 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/214

Ah, I see I should have read the new patch before suugesting something you already implemented :). Looks good: this patch adds a freeze() by evaluating Object.freeze() inside the sandbox, and adds disabled tests for the problems-with-proxies in bug 672199. So it does more than just the low-level API changes, it actually goes ahead and freeze everything, but I'm cool with that. r+

I'm still hoping the docs get updated before freeze, so that rare percentage of developers who try to modify the exports later will know why it's failing (especially if it fails silently). The case I'm most worried about is folks who are keeping mutable state on their exports object (maybe because they don't want to write a getter method?), and aren't using strict: their mutations will be silently ignored, which would be pretty confusing.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> I'm still hoping the docs get updated before freeze, so that rare percentage of > developers who try to modify the exports later will know why it's failing
> (especially if it fails silently).

I have r+ doc changes from Will so I expect him to land it today!

Landed: https://github.com/mozilla/addon-sdk/commit/67b2d9baf6850d00de2b57d9fd87467ca02146a9