1058836 - nsMixedContentBlocker.cpp doens't always set the content policy Decision when returning

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

nsMixedContentBlocker.cpp sometimes returns without setting the aDecision parameter:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#342

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#346

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#593

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#613

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#615

It must use the default decision, which is probably accept.  Otherwise the blocker wouldn't work today.  In most of these cases, the decision is accept.  For the others, we return an error.

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Set aDecision parameter in MCB

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #1)
> Created attachment 8479312 [details] [diff] [review]
> Set aDecision parameter in MCB

Probably set it to ACCEPT here as well:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#592

Comment 3

[Tanvi Vyas[:tanvi]]

Comment on attachment 8479312 [details] [diff] [review]
Set aDecision parameter in MCB

Compiles and passes mixed content mochitests.  Asking for review and then will push to try afterwards.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8479312 [details] [diff] [review]
Set aDecision parameter in MCB

The code is a bit error prone. I wonder if we could somehow ensure that the
out param is set to some value, maybe using a stack variable.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Bug1058336-08-28-14.patch

Revised patch to address smaug's comment - now we set a default aDecision of REJECT_REQUEST or block at the beginning of ShouldLoad().  Carrying over r+ and I pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=255cfe85898e

Comment 6

[Tanvi Vyas[:tanvi]]

Try looks good.  This is ready to push but inbound is closed, so marking as checkin-needed.

Comment 7

[Tanvi Vyas[:tanvi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7eccf7c09f1

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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