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

RESOLVED FIXED in Firefox 48

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: Josh Aas)

Tracking

(Blocks: 1 bug, {assertion, sec-high, testcase})

Trunk
mozilla50
Unspecified
Mac OS X
assertion, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox47 wontfix, firefox48+ fixed, firefox49+ fixed, firefox-esr4548+ fixed, firefox50+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main48+][adv-esr45.3+])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8732034 [details]
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
(Reporter)

Comment 1

2 years ago
Created attachment 8732035 [details]
stacks
(Reporter)

Comment 2

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

Updated

2 years ago
Component: DOM → Widget: Cocoa
(Reporter)

Comment 3

2 years ago
I split off the "recovering incorrectly" part into bug 1258945.
Summary: [Mac] "###!!! ASSERTION: This is unsafe! Fix the caller!" after caught Obj-C exception → [Mac] Obj-C exception: [NSInvalidArgumentException: *** -[_NSConcreteUserNotification setUserInfo:]. UserInfo too big. Encoded data greater than 16k....]
(Assignee)

Comment 4

2 years ago
I think I know how to fix this.
Assignee: nobody → jaas
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
Created attachment 8733930 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Attachment #8733930 - Flags: review?(spohl.mozilla.bugs)
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/
Attachment #8733930 - Flags: review?(spohl.mozilla.bugs) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8734170 [details] [diff] [review]
fix v1.1

Fixes comment typo, makes literal a named macro, reduces max len from 10k to 5k chars.
Attachment #8733930 - Attachment is obsolete: true
Attachment #8734170 - Flags: review?(spohl.mozilla.bugs)
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
Attachment #8734170 - Flags: review?(spohl.mozilla.bugs) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8734212 [details] [diff] [review]
fix v1.2

Nits fixed, ready for checkin.
Comment on attachment 8734212 [details] [diff] [review]
fix v1.2

Thank you!
Attachment #8734212 - Flags: review+
Attachment #8734170 - Attachment is obsolete: true
Keywords: checkin-needed
(Reporter)

Comment 12

2 years ago
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.
Keywords: checkin-needed
(Assignee)

Comment 13

2 years ago
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?

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
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?
Flags: needinfo?(jruderman)
Keywords: sec-high
(Reporter)

Comment 18

2 years ago
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.
Flags: needinfo?(jruderman)
(Reporter)

Comment 19

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

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-macosx64/1461837738/
Group: dom-core-security → layout-core-security
Please request sec-approval to land the patch.

Let's handle the general Obj-C exception catching issue in bug 1258945.
Flags: needinfo?(jaas)
Stephen, can you take care of requesting sec-approval and landing the patch please?
Flags: needinfo?(spohl.mozilla.bugs)
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?
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(mats)
> 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.
Flags: needinfo?(mats)
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.
Attachment #8734212 - Flags: sec-approval?
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.
status-firefox47: --- → wontfix
status-firefox49: --- → affected
status-firefox-esr45: --- → affected
Whiteboard: [checkin on 6/21]
Attachment #8734212 - Flags: sec-approval? → sec-approval+
We'll want branch patches made and nominated once this is on trunk as well.
status-firefox50: --- → affected
tracking-firefox48: --- → +
tracking-firefox49: --- → +
tracking-firefox50: --- → +
tracking-firefox-esr45: --- → 48+
Stephen, please land the patch.
Flags: needinfo?(jaas) → needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(spohl.mozilla.bugs)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/719cb361178a
Keywords: checkin-needed
Whiteboard: [checkin on 6/21]
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/719cb361178a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: layout-core-security → core-security-release
Hello Josh, could you please nominate patches for uplift to Beta, Aurora and ESR45? Thanks!
Flags: needinfo?(jaas)
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
Flags: needinfo?(jaas)
Attachment #8734212 - Flags: approval-mozilla-beta?
Attachment #8734212 - Flags: approval-mozilla-aurora?
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
Attachment #8757142 - Flags: approval-mozilla-esr45?
Comment on attachment 8734212 [details] [diff] [review]
fix v1.2

Sec-high issue, Aurora49+, Beta48+
Attachment #8734212 - Flags: approval-mozilla-beta?
Attachment #8734212 - Flags: approval-mozilla-beta+
Attachment #8734212 - Flags: approval-mozilla-aurora?
Attachment #8734212 - Flags: approval-mozilla-aurora+
Comment on attachment 8757142 [details] [diff] [review]
Patch for ESR45

Sec-high issues meet the ESR45 uplift bar.
Attachment #8757142 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main48+][adv-esr45.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.