882624 - Remove "URL Protocol" from FirefoxURL key

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Masatoshi Kimura [:emk]]

We have never intended to expose "firefoxurl://" as a URL protocol. It caused a security vulnerability in the past (bug 384384).
"IE.FTP", "IE.HTTP", and "IE.HTTPS" keys don't have the "URL Protocol" named value.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

Deleting the value explicitly because old installers may set the value.

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 761982 [details] [diff] [review]
patch

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

Please wait to implement the nit as there will likely be some more comments.

I won't be able to finish reviewing this until next week because I need to setup some clean registry VMs.
Leaving the review request until then.

As far as I recall this is needed but I'll verify. If we do make changes here, once this lands we should also post a bug for seamonkey.

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +1495,5 @@
>  
>        StrCmp "$R9" "true" +1 +2
>        WriteRegStr SHCTX "$R3\$R5" "URL Protocol" ""
> +      StrCmp "$R9" "delete" +1 +2
> +      DeleteRegValue SHCTX "$R3\$R5" "URL Protocol"

nit: Please add to this comment at the top of this function:
Exch $R9 ; true if a protocol handler

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 761982 [details] [diff] [review]
patch

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

Seems like it's only needed for custom URL protocols. So this looks good.

I tried it on XP, Vista, and Win8 and it works fine.

On Vista and Win8 (where there's a defaults control panel item) I could still set the default protocol handlers for Firefox and Firefox showed up in the list of default handlers for HTTP, HTTPS, and FTP protocols.

Comment 4

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5aea44ac54ce

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5aea44ac54ce

Comment 6

[Masatoshi Kimura [:emk]]

Sorry, I forgot to reflect the review nit.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef58eae5f12

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Why was checkin-needed set?

Comment 9

[Masatoshi Kimura [:emk]]

Sorry, I missed the bug number (I meant bug 884301).

Comment 10

[hunreal]

Remove "URL Protocol" from FirefoxURL key cause pinned shortcut fail to open in win7/8 jumplist.

This works in FF 23, fail in FF 24.

Comment 11

[Brian R. Bondy [:bbondy]]

(In reply to hunreal from comment #10)
> Remove "URL Protocol" from FirefoxURL key cause pinned shortcut fail to open
> in win7/8 jumplist.
> 
> This works in FF 23, fail in FF 24.

hunreal, could you please post a new bug describing this problem? You can use the link at the bottom of the page named Clone this bug.  Please put exact steps on how to pin a shortcut if it is different from normal shortcuts that are autopopulated by the app. Thanks!

Comment 12

[Brian R. Bondy [:bbondy]]

The removal of this registry value is causing some problems for some people (see bug 911158).
Emk, are you OK with me reverting this? I think that this bug was originally just a precaution and not an actual attack.

Comment 13

[Masatoshi Kimura [:emk]]

OK.