1257765 - [Mac] Obj-C exception: [NSInvalidArgumentException: *** -[_NSConcreteUserNotification setUserInfo:]. UserInfo too big. Encoded data greater than 16k....]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

Attachment: testcase (reload me)

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

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[Jesse Ruderman]

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

Comment 3

[Jesse Ruderman]

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

Comment 4

[Josh Aas]

I think I know how to fix this.

Comment 5

[Josh Aas]

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.

Comment 6

[Josh Aas]

Attachment: fix v1.0

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 7

[Stephen A Pohl [:spohl]]

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/

Comment 8

[Josh Aas]

Attachment: fix v1.1

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

Comment 9

[Stephen A Pohl [:spohl]]

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

Comment 10

[Josh Aas]

Attachment: fix v1.2

Nits fixed, ready for checkin.

Comment 11

[Stephen A Pohl [:spohl]]

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

Thank you!

Comment 12

[Jesse Ruderman]

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.

Comment 13

[Josh Aas]

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.

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Jet Villegas (inactive)]

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.

Comment 16

[Josh Aas]

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.

Comment 17

[Andrew McCreight [:mccr8]]

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

Comment 18

[Jesse Ruderman]

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.

Comment 19

[Jesse Ruderman]

Ordinary opt build crashes [@ js::gc::GCRuntime::updateMallocCounter]

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-macosx64/1461837738/

Comment 20

[Mats Palmgren (inactive)]

Please request sec-approval to land the patch.

Let's handle the general Obj-C exception catching issue in bug 1258945.

Comment 21

[Mats Palmgren (inactive)]

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

Comment 22

[Stephen A Pohl [:spohl]]

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?

Comment 23

[Mats Palmgren (inactive)]

> 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 24

[Stephen A Pohl [:spohl]]

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.

Comment 25

[Stephen A Pohl [:spohl]]

Attachment: Patch for ESR45

Comment 26

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

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

Comment 28

[Mats Palmgren (inactive)]

Stephen, please land the patch.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/719cb361178a

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/719cb361178a

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 32

[Stephen A Pohl [:spohl]]

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 33

[Stephen A Pohl [:spohl]]

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 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Sec-high issue, Aurora49+, Beta48+

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8757142 [details] [diff] [review]
Patch for ESR45

Sec-high issues meet the ESR45 uplift bar.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/b6ffa501bd27

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d219f175e180

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9ce7d735756d