1. Load the testcase 2. Reload 2016-03-17 23:13:28.512 firefox[3164:7228384] Mozilla has caught an Obj-C exception [NSInvalidArgumentException: *** -[_NSConcreteUserNotification setUserInfo:]. UserInfo too big. Encoded data greater than 16k. (32835 bytes)] ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file dom/events/EventDispatcher.cpp, line 553

(Are there two bugs here? One that we trip the Obj-C exception, and one that we recover incorrectly?)

I split off the "recovering incorrectly" part into bug 1258945.

I think I know how to fix this.

The exception here is being thrown in this line in 'OSXNotificationCenter::ShowAlertWithIconData': notification.userInfo = [NSDictionary dictionaryWithObjects:[NSArray arrayWithObjects:alertName, nil] forKeys:[NSArray arrayWithObjects:@"name", nil]]; The exception is: UserInfo too big. Encoded data greater than 16k. (32835 bytes) The basic fix would be to limit the size of the name being assigned to userInfo. Pretty simple. When the exception is thrown it should be caught by these macros which wrap the contents of 'OSXNotificationCenter::ShowAlertWithIconData': NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT; ... NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT; These macros are defined in 'xpcom/base/nsObjCExceptions.h': #define NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT @try { #define NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT } @catch(NSException *_exn) { \ nsObjCExceptionLog(_exn); \ } \ return NS_ERROR_FAILURE; This will catch the exception, log it, try to log stack info only if in a debug build, and return NS_ERROR_FAILURE from the C++ method.

This limits the length of notification strings to 10k characters. Limits it before we waste time converting to NSString. Was able to repro the exception without this patch, it goes away with this patch. The exception, when I repro'd, seemed to be handled properly. I didn't look very closely though.

Comment on attachment 8733930 [details] [diff] [review] fix v1.0 Review of attachment 8733930 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/OSXNotificationCenter.mm @@ +344,5 @@ > } > nsAutoString name; > rv = aAlert->GetName(name); > + // Don't let an alert name be more than 10,000 characters. More than that shouldn't be necessary > + // and userInfo (assigned to below) has a length limit of 16k. Exception thrown if limit exceded. nit: s/exceded/exceeded/

Fixes comment typo, makes literal a named macro, reduces max len from 10k to 5k chars.

Comment on attachment 8734170 [details] [diff] [review] fix v1.1 Review of attachment 8734170 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/OSXNotificationCenter.mm @@ +345,5 @@ > } > } > nsAutoString name; > rv = aAlert->GetName(name); > + // Don't let an alert name be more than 10,000 characters. More than that shouldn't be necessary nit: this comment still refers to 10,000 characters @@ +346,5 @@ > } > nsAutoString name; > rv = aAlert->GetName(name); > + // Don't let an alert name be more than 10,000 characters. More than that shouldn't be necessary > + // and userInfo (assigned to below) has a length limit of 16k. Exception thrown if limit exceeded. nit: both lines of this comment exceed 80 characters

Nits fixed, ready for checkin.

Comment on attachment 8734212 [details] [diff] [review] fix v1.2 Thank you!

Per bug 1017685, a lower limit is needed for older versions of OS X. Sorry for missing this connection earlier. There should be a comment explaining why it's safe to check for 5000 sixteen-bit code units (not "characters"!) instead of 10000 eight-bit units. I'd really like to understand why this can result in "ASSERTION: This is unsafe! Fix the caller!", despite the @catch being in OSXNotificationCenter::ShowAlertWithIconData. Were you ever able to reproduce that? Btw, that assertion only happened for me when reloading the testcase, while the ObjC exception itself only required loading it once.

I tested with unpatched opt and debug builds. In the opt build I see the assertion print to the console, but that's all I see even after reloading the test case a few times. Firefox continues to work fine. In the debug build Firefox hangs up permanently when I load the test case. The assertion prints to the console, followed by information about trying to capture a stack trace. I've never sees anything like "ASSERTION: This is unsafe! Fix the caller!". As far as I can tell there is something wrong with our attempt to capture a stack trace in debug builds but the rest is fixed by the patch here.

What should the security rating for this bug be? Is there an actual security issue here?

In bug 1258945, Jesse reports that this exception crashes and puts the stack pointer in a random location. Seems at least sec-high to me. It's unclear whether the giant string in this bug is the overflow condition that causes bug 1258945 or if there's something else at play here.

We need to know if Jesse is just seeing the crash in debug builds due to the attempt to get a stack trace, or if he can reproduce the issue in an opt build. If he can't reproduce in an opt build (I can't) then there is probably no security issue here. My hunch is that the problems he's seeing come from the attempt to get a stack trace in debug builds.

Jesse, per comment 16, can you reproduce this in an opt build?

For this testcase, I'm now seeing: * Debug: shows ObjC exception, hangs with low CPU (instead of crashing) * ASan: shows ObjC exception, shows the desktop notification, continues fine For the testcase in bug 1258945, I'm now seeing: * Debug: shows ObjC exception, "Assertion failure: activation->isInterpreter()" * ASan: shows ObjC exception, crashes [@ js::SavedStacks::insertFrames] So yes, I can reproduce "ObjC exception seems to lead to bad things happening" in non-debug builds.

Ordinary opt build crashes [@ js::gc::GCRuntime::updateMallocCounter] https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-macosx64/1461837738/

Please request sec-approval to land the patch. Let's handle the general Obj-C exception catching issue in bug 1258945.

Stephen, can you take care of requesting sec-approval and landing the patch please?

I don't currently have permission to access bug 1258945 and there seems to be quite a bit of overlap between the two. Also, just like Josh (comment 16), I can't reproduce the issue that Jesse mentions in comment 18 and comment 19. I don't feel very comfortable requesting sec-approval under these circumstances. Do you want me to proceed anyway?

> I don't currently have permission to access bug 1258945 Sorry, didn't realize that. I've CC:ed you there. >Do you want me to proceed anyway? Yes, please. As the reviewer you know more about this code than I do so you can probably give better answers to sec-approval questions than me.

Comment on attachment 8734212 [details] [diff] [review] fix v1.2 [Security approval request comment] How easily could an exploit be constructed based on the patch? Unknown Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, we added a comment that makes it very clear why we've added a limitation on the string lengths for OSX notifications. Which older supported branches are affected by this flaw? This issue affects central, aurora, beta, release and esr45. If not all supported branches, which bug introduced the flaw? Native OSX notifications were introduced in bug 852648. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch applies successfully on central, aurora, beta and release. I've created a separate patch for esr45 in case it is desired. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions and only straightforward testing required since it is minimally invasive.

This is too late to go into 47, which means we need to delay checkin until two weeks into the next cycle. So I'm giving sec-approval but only for checkin on June 21 or later.

We'll want branch patches made and nominated once this is on trunk as well.

Stephen, please land the patch.

Hello Josh, could you please nominate patches for uplift to Beta, Aurora and ESR45? Thanks!

Comment on attachment 8734212 [details] [diff] [review] fix v1.2 Approval Request Comment [Feature/regressing bug #]: Native OSX notifications were introduced in bug 852648. [User impact if declined]: Security issue [Describe test coverage new/current, TreeHerder]: local testing [Risks and why]: Unlikely to cause regressions and only straightforward testing required since it is minimally invasive. [String/UUID change made/needed]: none

Comment on attachment 8757142 [details] [diff] [review] Patch for ESR45 Approval Request Comment [Feature/regressing bug #]: Native OSX notifications were introduced in bug 852648. [User impact if declined]: Security issue [Describe test coverage new/current, TreeHerder]: local testing [Risks and why]: Unlikely to cause regressions and only straightforward testing required since it is minimally invasive. [String/UUID change made/needed]: none

Comment on attachment 8734212 [details] [diff] [review] fix v1.2 Sec-high issue, Aurora49+, Beta48+

Comment on attachment 8757142 [details] [diff] [review] Patch for ESR45 Sec-high issues meet the ESR45 uplift bar.