Usage of `eval()` in third-party library ajv-4.1.1.js

REOPENED
Assigned to

Status

()

enhancement
P3
normal
REOPENED
2 months ago
16 days ago

People

(Reporter: jallmann, Assigned: jallmann)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 obsolete attachment)

All eval()-like functions (eval(), new Function(), setTimeout("")) are being removed from code running with system privileges, see Bug 1473549. An assertion is active to enforce this.

There are three occurences of eval() in ajv-4.1.1.js that require the file to be whitelisted for this assertion. In order to clear ajv-4.1.1.js from the whitelist, eval() needs to be removed or refactored.

https://searchfox.org/mozilla-central/source/testing/modules/ajv-4.1.1.js#109

https://searchfox.org/mozilla-central/source/testing/modules/ajv-4.1.1.js#121

https://searchfox.org/mozilla-central/source/testing/modules/ajv-4.1.1.js#604

A simple try run with the file removed from the whitelist to see which tests fail.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fde0ea4d02b46d70861af3d75930692b9539e85e

Allow eval() by flipping pref in all affected test files.

Assignee: nobody → jallmann
Keywords: checkin-needed

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f2475241df7
Remove ajv-4.1.1.js from eval()-whitelist. r=ckerschb

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

This change is really worrying. In order to allow those tests to use a helper which uses eval, we blanket enable all eval usage in system principal documents when they're running. Which means that if real, unexpected system principal eval usage happens during those tests, they will continue to work as expected, but the same code will fail in the real world.

I've run into that sort of thing actually happening enough times that this makes me really uncomfortable.

Flags: needinfo?(jallmann)

That's a valid point. Our reasoning here was to prefer allowing eval() in certain tests over keeping the helper file whitelisted for the eval()-assertion, in an attempt to make the whitelist entirely obsolete as soon as possible. Ideally, of course, these tests would be refactored to avoid using eval at all. Maybe this could be planned in a follow-up bug.
I guess in the end it's about deciding how to except remaining uses of eval()-like functions, especially in test code, from the assertion. I'm not sure which of the two options, whitelisting certain files or disabling the assertion in specific tests, is generally preferable.

Flags: needinfo?(jallmann) → needinfo?(ckerschb)

I agree that limiting the exceptions to the tests that use eval in test-only code makes more sense than enabling them in production. I think the exceptions that we add for those tests should be as limited as possible, though. Adding just this script to the whitelist seems like the obvious choice (and maybe adding a helper function for that somewhere...)

Ha, interestingly I just asked Gijs the same question in a different bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1550465#c3

As of now I am leaning towards keeping the whitelist which seems safer, because we can whitelist individual files where eval is allowed in comparision to whitelist an entire tests which might lead to accidentally greenlighting eval() is system land which should actually be blocked.

Flags: needinfo?(ckerschb)

Hey Aryx, given the latest discussions within this bug. Would it be hard to backout that changeset again for us? If not, could you please do that for us and re-open the bug?

If it's too hard or doesn't work easily, we could write another patch to revert those changes.

Flags: needinfo?(aryx.bugmail)
Status: RESOLVED → REOPENED
Flags: needinfo?(aryx.bugmail)
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #10)

https://hg.mozilla.org/integration/autoland/rev/0daa6c2569f4d924c80b3f70a555cc6d3e699bac

Thanks!

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jallmann, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(jallmann)

The r+-patch for this bug has become obsolete since ajv-4.1.1.js is likely staying on a permanent whitelist for the eval()-assertion.
Is there a way I can make that clear, to avoid the impression that there is a patch here, ready and waiting to land?

Flags: needinfo?(jallmann) → needinfo?(ckerschb)

(In reply to Jonas Allmann [:jallmann] from comment #13)

The r+-patch for this bug has become obsolete since ajv-4.1.1.js is likely staying on a permanent whitelist for the eval()-assertion.
Is there a way I can make that clear, to avoid the impression that there is a patch here, ready and waiting to land?

I think you can 'abandon' the revision within phabricator so the bot does not think there is an r+ patch to be checked in.

Flags: needinfo?(ckerschb)
Attachment #9066017 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.