Closed Bug 1501498 Opened 1 year ago Closed 1 year ago

Crash in AddonContentPolicy::ShouldLoad

Categories

(Core :: DOM: Security, defect, P1, critical)

64 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 --- fixed
firefox65 --- fixed

People

(Reporter: lizzard, Assigned: ckerschb)

Details

(Keywords: crash, regression, Whiteboard: [domsecurity-active])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-33e99786-ec5f-4948-8e1b-0cc650181023.
=============================================================

This looks like a new crash signature in the 20181022220734 nightly 65 build. 

Top 10 frames of crashing thread:

0 XUL AddonContentPolicy::ShouldLoad toolkit/mozapps/extensions/AddonContentPolicy.cpp:119
1 XUL nsContentPolicy::CheckPolicy dom/base/nsContentPolicy.cpp:151
2 XUL nsContentPolicy::ShouldLoad dom/base/nsContentPolicy.cpp:211
3 XUL NS_CheckContentLoadPolicy dom/base/nsContentPolicyUtils.h:227
4 XUL nsContentSecurityManager::doContentSecurityCheck dom/security/nsContentSecurityManager.cpp:606
5 XUL mozilla::net::HttpChannelChild::AsyncOpen2 netwerk/protocol/http/HttpChannelChild.cpp:2753
6 XUL NS_InvokeByIndex 
7 XUL XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1723
8 XUL XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1019
9 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:468

=============================================================
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
Baku, in other ::ShouldLoad implementations we first check aContentLocation before dereferencing it - we should do the same for AddonContentPolicy.
Attachment #9019994 - Flags: review?(amarchesini)
Arr, the correct patch uses aShouldLoad instead of aDecision, stupid copy/paste.
Comment on attachment 9019994 [details] [diff] [review]
bug_1501498_regression.patch

Review of attachment 9019994 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp
@@ +97,5 @@
>                                 const nsACString& aMimeTypeGuess,
>                                 int16_t* aShouldLoad)
>  {
> +  if (!aContentLocation) {
> +    *aDecision = REJECT_REQUEST;

if (!aContentLocation || !aLoadInfo) {
Attachment #9019994 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/e8b53fa4ae7e
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Is this worth uplifting to Beta or can it ride the trains?
Flags: needinfo?(ckerschb)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> Is this worth uplifting to Beta or can it ride the trains?

Well, it's a fix for a crash and the Null pointer check is small enough in my opinion that we should uplift it - I'll file the request.
Flags: needinfo?(ckerschb)
Comment on attachment 9019994 [details] [diff] [review]
bug_1501498_regression.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: unknown

User impact if declined: Firefox crash.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The risk is low because this patch only introduces a null pointer check before dereferencing the pointer.

String changes made/needed: no
Attachment #9019994 - Flags: approval-mozilla-beta?
Comment on attachment 9019994 [details] [diff] [review]
bug_1501498_regression.patch

Simple null check crash fix. Approved for 64.0b5.
Attachment #9019994 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.