Closed Bug 1558986 Opened 5 years ago Closed 5 years ago

Stop using eval in calendar

Categories

(Calendar :: General, defect)

Lightning 69
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 69.0

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

References

Details

Attachments

(1 file, 1 obsolete file)

MOZ_ASSERT(false, "do not use eval with system privileges");

xul.dll!nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(nsIPrincipal * subjectPrincipal, JSContext * cx) Line 205 C++
xul.dll!nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext * cx, JS::Handle<JS::Value> aValue) Line 410 C++
xul.dll!EvalKernel(JSContext * cx, JS::Handle<JS::Value> v, EvalType evalType, js::AbstractFramePtr caller, JS::Handle<JSObject > env, unsigned char * pc, JS::MutableHandle<JS::Value> vp) Line 219 C++
xul.dll!js::IndirectEval(JSContext * cx, unsigned int argc, JS::Value * vp) Line 424 C++
xul.dll!CallJSNative(JSContext * cx, bool(
)(JSContext *, unsigned int, JS::Value *) native, const JS::CallArgs & args) Line 448 C++
xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 540 C++
xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args) Line 595 C++

JS stack:
1 <TOP LEVEL> ["resource://gdata-provider/modules/gdataSession.jsm":546:70]
2 <TOP LEVEL> ["chrome://gdata-provider/content/gdata-calendar-creation.js":15:46]
3 loadScript(node = [object XULElement]) ["resource:///modules/Overlays.jsm":463:30]
this = [object Object]
4 load(urls = chrome://gdata-provider/content/gdata-calendar-creation.xul) ["resource:///modules/Overlays.jsm":195:32]
this = [object Object]
5 load(overlayProvider = [object Object], window = [object ChromeWindow]) ["resource:///modules/Overlays.jsm":40:13]

Flags: needinfo?(geoff)

gdataSession.jsm:546, that's that cryptic stuff. ["\x65\x76\x61\x6C"] = eval :-(

Yes it is, but that file is white-listed, so I don't know how there's a problem.

Flags: needinfo?(geoff)

I don't know what's caused the change in behaviour but we should change those two code blocks (this and the calDAV one) anyway.

Assignee: nobody → geoff
Summary: Adding a new calendar crashes on trunk - yet another eval crash → Stop using eval in calendar

Creating a fresh profile and only enable Lightning (but not google calendar) allows to start the create-a-calendar-wizard. but still crashes, when selecting caldav on the second screen.

Same issue?

Yes.

Attach the debugger and see where it crashes, that's what I did.

Attached patch 1558986-no-eval-calendar-1.diff (obsolete) (deleted) — Splinter Review
Attachment #9071832 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Version: Trunk → Lightning 7.1

May I assume that only debug builds are affected and release builds work OK?
The debug assertion was added in Bug 1541858 to check for eval() in worker code too in Thunderbird 68.

Yes, only debug builds.

I'd say some of the "TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed" in Mozmill debug runs are caused by this, too. I tried
mozmake -C comm/calendar/test/mozmill SOLO_TEST=testLocalICS.js mozmill-one
and it crashed immediately.

BTW, why did you remove the whitelisting? Have M-C removed that feature? pref("security.allow_eval_with_system_principal", true); still works.

Without these two files, there's no reason for us to have our own version of the pref. In theory.

I see. Maybe we should leave pref("security.allow_eval_with_system_principal", false); as a comment so we can tweak it if necessary instead of having to recall what it was.

Comment on attachment 9071832 [details] [diff] [review] 1558986-no-eval-calendar-1.diff Review of attachment 9071832 [details] [diff] [review]: ----------------------------------------------------------------- Do you have a script to generate these, or did you do it by hand? Some of the function names are still pretty readable, maybe we can just use \xNN everywhere. If you could share (privately) any script you used that would be great, or otherwise create one so it is easy to replace the keys in the future. I'm going to r+ this since I generally agree with the concept, but maybe you could run the final version past me.
Attachment #9071832 - Flags: review?(philipp) → review+

Not really a script as such, but I did use the javascript console to replace characters at random with hex codes. I'll hide a few more where randomness has failed to do a good job. Unfortunately even then it's fairly trivial to find the information hidden here.

Attached patch 1558986-no-eval-calendar-2.diff (deleted) — Splinter Review

Obscured a few more things. I wish there was a better way to do this.

Attachment #9071832 - Attachment is obsolete: true
Attachment #9072736 - Flags: review+
Attachment #9072736 - Flags: feedback?(philipp)
Comment on attachment 9072736 [details] [diff] [review] 1558986-no-eval-calendar-2.diff Review of attachment 9072736 [details] [diff] [review]: ----------------------------------------------------------------- Ok. I was asking about the script, since I used to have a thing where when I needed to change the key I could just run it to generate the new code. I guess I'll have to defer that until I change the code.
Attachment #9072736 - Flags: feedback?(philipp) → feedback+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d72b6ee0f40f
Stop using eval in calendar. r=philipp

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 7.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: