1501498 - Crash in AddonContentPolicy::ShouldLoad

Status: RESOLVED FIXED

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

Description

[Liz Henry (:lizzard)]

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

=============================================================

Comment 1

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Attachment: bug_1501498_regression.patch

Baku, in other ::ShouldLoad implementations we first check aContentLocation before dereferencing it - we should do the same for AddonContentPolicy.

Comment 2

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Arr, the correct patch uses aShouldLoad instead of aDecision, stupid copy/paste.

Comment 3

[Andrea Marchesini [:baku]]

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) {

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b53fa4ae7e
Fix crash in AddonContentPolicy. r=baku

Comment 5

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e8b53fa4ae7e

Comment 6

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e8b53fa4ae7e

Comment 7

[Ryan VanderMeulen (PTO, back July 27)]

Is this worth uplifting to Beta or can it ride the trains?

Comment 8

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

(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.

Comment 9

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

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

Comment 10

[Ryan VanderMeulen (PTO, back July 27)]

Comment on attachment 9019994 [details] [diff] [review]
bug_1501498_regression.patch

Simple null check crash fix. Approved for 64.0b5.

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a8a181f1095f