1261140 - Stub installer support for attribution

Status: RESOLVED FIXED

(Firefox :: Installer, enhancement)

Description

[Molly Howell (she/her) (no longer active)]

As part of the installer attribution project (bug 1259607), the stub installer needs to be able to get the attribution data and store it with the installed product so that it can be used in the activation ping (bug 1120370).

Comment 1

[Molly Howell (she/her) (no longer active)]

Attachment: Prototype installer editing script

This is a Python script that adds an arbitrary string to a signed Windows executable without invalidating the signature. I wouldn't call it ready to use right now, but something like this could be how we add the attribution code to the stub installer (once we figure out when that needs to happen).

Comment 2

[Molly Howell (she/her) (no longer active)]

Attachment: Prototype NSIS plugin for retrieving attribution data

This is C++ source for an NSIS plugin that retrieves "custom" (attribution) data from the installer file and gets it into the NSIS logic so we can work with it during installation. This code assumes that the script in attachment 8737240 [details] was used to write the data. This code isn't straightforward because the actual installer is executed indirectly by the 7-zip self-extraction stub, so it has some work to do to locate the original file. After that, finding the data is pretty similar to the process for writing it in the first place.

Comment 3

[Daniel Veditz [:dveditz]]

Note that Microsoft has issued a security advisory against this approach and intended to kill it off. They implemented a registry key that will do so, and although they seem to have backed off from making that the default it's still available and may be enabled in some environments as a security measure. That's why modern installers have moved on to the more complicated certificate-based methods.

https://technet.microsoft.com/library/security/2915720

Before we use this data something needs to confirm that it matches the format we've agreed on for an identifier, so we don't have to worry about a MITM smuggling something malicious in there (that might be interpreted as HTML, etc. in a different context). I'd prefer it be in this low-level routine that gets it out of the file, but if not certainly at the next level up before we do anything like save it or send it somewhere.

Comment 4

[Molly Howell (she/her) (no longer active)]

Attachment: Prototype 7-Zip SFX patch for retriving attribution data

Rather than adding a new NSIS plugin, this is a smaller patch that creates a file containing the attribution code in the 7-Zip temp directory, as if it were part of the archive. The installer can read this file by straightforward use of NSIS instructions. This method is simpler and the overall size of the new code is quite a bit smaller.

The final version of this patch will need to include a rebuilt 7zSD.sfx binary. I left that out because it has to be built using a very old Visual C++ for compatibility with the largest set of Windows versions (including ones Firefox doesn't support, so we can show the message saying we don't support them), and I don't have that available.

Comment 5

[Molly Howell (she/her) (no longer active)]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Note that Microsoft has issued a security advisory against this approach and
> intended to kill it off. They implemented a registry key that will do so,
> and although they seem to have backed off from making that the default it's
> still available and may be enabled in some environments as a security
> measure. That's why modern installers have moved on to the more complicated
> certificate-based methods.
> 
> https://technet.microsoft.com/library/security/2915720

I just tried it, and this approach is indeed blocked if the check mentioned in that advisory is switched on. I knew about the advisory when I started and went with this method anyway because I thought that the risk of the new check actually getting switched on by default did not outweigh the large spike in complexity that a method complying with the check would entail (as in, I think we would end up needing an ASN.1 DER parser on both the storage and retrieval sides). It's entirely possible that I didn't make the right decision; discussion welcome.

> Before we use this data something needs to confirm that it matches the
> format we've agreed on for an identifier, so we don't have to worry about a
> MITM smuggling something malicious in there (that might be interpreted as
> HTML, etc. in a different context). I'd prefer it be in this low-level
> routine that gets it out of the file, but if not certainly at the next level
> up before we do anything like save it or send it somewhere.

That is a very good point. The above patch should be amended to include this once the format is determined.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 8738657 [details] [diff] [review]
Prototype 7-Zip SFX patch for retriving attribution data

Looks good

Comment 7

[Molly Howell (she/her) (no longer active)]

I decided to at least look into how hard it would be to switch to the less brittle method used by Google et al., and I think I've gotten it working. There's a bit of a hack involved, but it's a much less questionable one than the one the other entire method is based around, and for what it's worth there is evidence that the Chrome installer uses it too. I now feel like we should probably just go with this.

This method uses a hand-crafted dummy certificate that intentionally isn't good for signing anything (it's self-signed, and expired), but that includes a big empty space where you can drop in arbitrary data. Add in that certificate at signing time (using osslsigncode's -ac option), and then that space can be filled in later with the attribution code/other data. The real signing certificate is still present as well, and it gets accepted by Authenticode as usual.
The problem is finding where the empty space is when it's time to write or read from there, and that's where the hack comes in. It's not hard to find a big bunch of NUL bytes, but it is hard to find the exact start and end of the ones that you can overwrite without breaking the encoding of the certificate itself and invalidating the whole thing. My hack is to insert a special token in the certificate at the start of the empty area, so that it can be found by a simple string search. That makes the procedures for writing and reading codes not much different than with the other method; all the magic is in the dummy certificate.

Comment 8

[Molly Howell (she/her) (no longer active)]

Attachment: Method 3 POC

This is a proof of concept for the method described above, containing the "magic" certificate, a Python script to fill in an attribution code/other data after signing, and a patch that gets the data to the stub installer for it to start working with. Here's how to use this stuff:

1) Apply the .diff to mozilla-central and build the stub installer normally.
2) Sign the stub installer as usual, but add the osslsigncode option "-ac MozDummy.cer".
3) Run write_attribution_data.py and pass it the installer file name and the string you want to write.
4) Run the stub installer and just leave it open, don't do anything.
5) Open 7-Zip's temp folder (should be at "C:\Users\[your username]\AppData\Local\Temp\7z[random stuff].tmp") and you should see a file called postSigningData which contains the string you supplied. (I don't have the code written yet to do anything else with the data from there.)

Comment 9

[Molly Howell (she/her) (no longer active)]

Attachment: Allow stub installer to read post-signing data and leave it where Firefox can find it

This patch updates the one in attachment 8744471 [details] to include the NSIS code to leave the attribution data in a place where Firefox can find it later, and to send it in the stub install ping.

This patch should be ready to review/land except for the wrinkle with the 7-zip extractor that I mentioned in comment 4; this one was built in VS2015 and probably only runs on XP and up.

Comment 10

[Molly Howell (she/her) (no longer active)]

Attachment: Method 3 installer editing script

Re-attaching the files from attachment 8744471 [details] as individual files instead of a ZIP.

This script obsoletes the one for the old method, which I think we have consensus against using.

Comment 11

[Molly Howell (she/her) (no longer active)]

Attachment: Dummy Certificate

Comment 12

[Robert Strong (they/them - no direct email)]

Note: I'll finish the review after I compile the binary with VC6 which is required so we can show an error message for Win2K users. I will hopefully get this done by end of day Monday 5/16.

Comment 13

[Robert Strong (they/them - no direct email)]

The following errors occurred when compiling on VC6 

D:\7zstub-new\src\7zip\Bundles\SFXSetup-moz\Main.cpp(49) : error C2065: 'strnlen' : undeclared identifier
D:\7zstub-new\src\7zip\Bundles\SFXSetup-moz\Main.cpp(107) : error C2664: 'CreateFileA' : cannot convert parameter 1 from 'class CStringBase<unsigned short>' to 'const char *'
        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

Comment 14

[Robert Strong (they/them - no direct email)]

Note: we compile on VC6 so we can have a small binary size by using a CRT that is always available on Windows systems.

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 8748190 [details] [diff] [review]
Allow stub installer to read post-signing data and leave it where Firefox can find it

>diff --git a/browser/installer/windows/nsis/stub.nsi b/browser/installer/windows/nsis/stub.nsi
>--- a/browser/installer/windows/nsis/stub.nsi
>+++ b/browser/installer/windows/nsis/stub.nsi
>@@ -102,25 +102,26 @@ Var EndFinishPhaseTickCount
> Var InitialInstallRequirementsCode
> Var ExistingProfile
> Var ExistingVersion
> Var ExistingBuildID
> Var DownloadedBytes
> Var DownloadRetryCount
> Var OpenedDownloadPage
> Var DownloadServerIP
>+Var PostSigningData
> 
> Var ControlHeightPX
> Var ControlRightPX
> 
> ; Uncomment the following to prevent pinging the metrics server when testing
> ; the stub installer
> ;!define STUB_DEBUG
> 
>-!define StubURLVersion "v6"
>+!define StubURLVersion "v7"
Thank you!

> ; Successful install exit code
> !define ERR_SUCCESS 0
> 
> /**
>  * The following errors prefixed with ERR_DOWNLOAD apply to the download phase.
>  */
> ; The download was cancelled by the user
>@@ -740,24 +741,25 @@ Function SendPing
>                       $\nExisting Version = $ExistingVersion \
>                       $\nExisting Build ID = $ExistingBuildID \
>                       $\nNew Version = $R5 \
>                       $\nNew Build ID = $R6 \
>                       $\nDefault Install Dir = $R7 \
>                       $\nHas Admin = $R8 \
>                       $\nDefault Status = $R2 \
>                       $\nSet As Sefault Status = $R3 \
>-                      $\nDownload Server IP = $DownloadServerIP"
>+                      $\nDownload Server IP = $DownloadServerIP \
>+                      $\nPost-Signing Data = $PostSigningData"
>     ; The following will exit the installer
>     SetAutoClose true
>     StrCpy $R9 "2"
>     Call RelativeGotoPage
> !else
>     ${NSD_CreateTimer} OnPing ${DownloadIntervalMS}
>-    InetBgDL::Get "${BaseURLStubPing}/${StubURLVersion}${StubURLVersionAppend}/${Channel}/${UpdateChannel}/${AB_CD}/$R0/$R1/$5/$6/$7/$8/$9/$ExitCode/$FirefoxLaunchCode/$DownloadRetryCount/$DownloadedBytes/$DownloadSizeBytes/$IntroPhaseSeconds/$OptionsPhaseSeconds/$0/$1/$DownloadFirstTransferSeconds/$2/$3/$4/$InitialInstallRequirementsCode/$OpenedDownloadPage/$ExistingProfile/$ExistingVersion/$ExistingBuildID/$R5/$R6/$R7/$R8/$R2/$R3/$DownloadServerIP" \
>+    InetBgDL::Get "${BaseURLStubPing}/${StubURLVersion}${StubURLVersionAppend}/${Channel}/${UpdateChannel}/${AB_CD}/$R0/$R1/$5/$6/$7/$8/$9/$ExitCode/$FirefoxLaunchCode/$DownloadRetryCount/$DownloadedBytes/$DownloadSizeBytes/$IntroPhaseSeconds/$OptionsPhaseSeconds/$0/$1/$DownloadFirstTransferSeconds/$2/$3/$4/$InitialInstallRequirementsCode/$OpenedDownloadPage/$ExistingProfile/$ExistingVersion/$ExistingBuildID/$R5/$R6/$R7/$R8/$R2/$R3/$DownloadServerIP/$PostSigningData" \
Need to check if the $PostSigningData needs to be escaped

>                   "$PLUGINSDIR\_temp" /END
> !endif
>   ${Else}
>     ${If} "$IsDownloadFinished" == "false"
>       ; Cancel the download in progress
>       InetBgDL::Get /RESET /END
>     ${EndIf}
>     ; The following will exit the installer
>@@ -1762,16 +1764,18 @@ Function FinishProgressBar
>   IntOp $InstallCounterStep $InstallCounterStep + 1
> 
>   ${If} $InstallCounterStep < 10
>     Return
>   ${EndIf}
> 
>   ${NSD_KillTimer} FinishProgressBar
> 
>+  Call CopyPostSigningData
>+
>   Call LaunchApp
> 
>   Call SendPing
Might as well just remove the empty lines between CopyPostSigningData and SendPing

> FunctionEnd
> 
> Function OnBack
>   StrCpy $WasOptionsButtonClicked "1"
>   StrCpy $R9 "1" ; Goto the next page
>@@ -1992,16 +1996,30 @@ Function LaunchAppFromElevatedProcess
>   ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" ""
>   ${GetPathFromString} "$0" $0
>   ; Set the current working directory to the installation directory
>   ${GetParent} "$0" $1
>   SetOutPath "$1"
>   Exec "$\"$0$\""
> FunctionEnd
> 
>+Function CopyPostSigningData
>+  FileOpen $0 "$EXEDIR\postSigningData" "r"
>+  ${IfNot} ${Errors}
>+    FileRead $0 $PostSigningData
>+    FileClose $0
>+  ${EndIf}
>+  CreateDirectory "$LOCALAPPDATA\mozilla\Firefox"
>+  FileOpen $0 "$LOCALAPPDATA\mozilla\Firefox\postSigningData" "w"
>+  ${IfNot} ${Errors}
>+    FileWrite $0 "$INSTDIR$\n$PostSigningData$\n"
>+    FileClose $0
>+  ${EndIf}
Just copy the file

>diff --git a/other-licenses/7zstub/src/7zip/Bundles/SFXSetup-moz/Main.cpp b/other-licenses/7zstub/src/7zip/Bundles/SFXSetup-moz/Main.cpp
>--- a/other-licenses/7zstub/src/7zip/Bundles/SFXSetup-moz/Main.cpp
>+++ b/other-licenses/7zstub/src/7zip/Bundles/SFXSetup-moz/Main.cpp
>...
>@@ -326,17 +418,34 @@ int APIENTRY WinMain(
>       }
>       if (result != E_ABORT && !errorMessage.IsEmpty())
>         ::MessageBoxW(0, errorMessage, NWindows::MyLoadStringW(IDS_EXTRACTION_ERROR_TITLE), MB_ICONERROR);
>     }
>     return 1;
>   }
> 
>   /* BEGIN Mozilla customizations */
>-  // The code immediately above handles the error case for extraction.
>+  // Retrieve and store any data added to this file after signing.
>+  {
>+    AString postSigningData;
>+    if (ReadPostSigningData(fullPath, postSigningData)) {
>+      NFile::NName::CParsedPath postSigningDataFilePath;
>+      postSigningDataFilePath.ParsePath(tempDirPath);
>+      postSigningDataFilePath.PathParts.Add(L"postSigningData");
>+
>+      NFile::NIO::COutFile postSigningDataFile;
>+      postSigningDataFile.Create(postSigningDataFilePath.MergePath(), true);
>+
>+      UInt32 written = 0;
>+      postSigningDataFile.Write(postSigningData, postSigningData.Length(), written);
>+    }
>+  }
>+
>+  // The code above handles the error case for extraction.
>+
Please just remove this. Reading the code should be sufficient.

>   if (extractOnly) {
>     return 0;
>   }
>   /* END Mozilla customizations */
> 
>   CCurrentDirRestorer currentDirRestorer;
> 
>   if (!SetCurrentDirectory(tempDir.GetPath()))
>diff --git a/other-licenses/7zstub/src/7zip/GuiCommon.rc b/other-licenses/7zstub/src/7zip/GuiCommon.rc
>--- a/other-licenses/7zstub/src/7zip/GuiCommon.rc
>+++ b/other-licenses/7zstub/src/7zip/GuiCommon.rc
>@@ -1,10 +1,9 @@
>-#include <winnt.h>
>-#include <WinUser.h>
>+#include <windows.h>
Is this change required... could you limit the changes to files in
other-licenses/7zstub/src/7zip/Bundles/SFXSetup-moz/ ?

This and the compile errors should be enough for an r+

Comment 16

[Robert Strong (they/them - no direct email)]

Regarding escaping the url, first check if the url is automatically escaped (I think it is) and if it isn't then the following can be used
http://nsis.sourceforge.net/URLEncode

Also, I don't think it will be a problem but the string size limit in NSIS is 1024

Comment 17

[Molly Howell (she/her) (no longer active)]

Attachment: Allow stub installer to read post-signing data and leave it where Firefox can find it

I made my best effort at the compile errors; I was always worried about that since I was just guessing at what VC6 would accept, but I think they're both fixed now.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #15)
> >diff --git a/other-licenses/7zstub/src/7zip/GuiCommon.rc b/other-licenses/7zstub/src/7zip/GuiCommon.rc
> >--- a/other-licenses/7zstub/src/7zip/GuiCommon.rc
> >+++ b/other-licenses/7zstub/src/7zip/GuiCommon.rc
> >@@ -1,10 +1,9 @@
> >-#include <winnt.h>
> >-#include <WinUser.h>
> >+#include <windows.h>
> 
> Is this change required... could you limit the changes to files in
> other-licenses/7zstub/src/7zip/Bundles/SFXSetup-moz/ ?
The VS2015 resource compiler complains in a very obtuse way ("fatal error RC10056", with no message) without that change. Since what the VS2015 resource compiler does with that file makes no difference to anyone but me, I reverted this.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #16)
> Regarding escaping the url, first check if the url is automatically escaped
> (I think it is) and if it isn't then the following can be used
> http://nsis.sourceforge.net/URLEncode
I'm glad you found that, I was not excited about writing that NSIS script.

> Also, I don't think it will be a problem but the string size limit in NSIS
> is 1024
The URL is getting a bit long, not to mention the debug message. I'll try to keep an eye on this.

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: patch - binary compiled with VC6

Comment 19

[Robert Strong (they/them - no direct email)]

Comment on attachment 8754444 [details] [diff] [review]
Allow stub installer to read post-signing data and leave it where Firefox can find it

>diff --git a/browser/installer/windows/nsis/stub.nsi b/browser/installer/windows/nsis/stub.nsi
>--- a/browser/installer/windows/nsis/stub.nsi
>+++ b/browser/installer/windows/nsis/stub.nsi
>...
>@@ -509,21 +510,119 @@ Function .onUserAbort
>     Call DisplayDownloadError
>     ; Aborting the abort will allow SendPing which is called by
>     ; DisplayDownloadError to hide the installer window and close the installer
>     ; after it sends the metrics ping.
>     Abort
>   ${EndIf}
> FunctionEnd
> 

Matt is checking if escaping is necessary or if it is done by wininet.

>+; Taken from http://nsis.sourceforge.net/CharToASCII
>+!define CharToASCII "!insertmacro CharToASCII"
>+!macro CharToASCII AsciiCode Character
>+  Push "${Character}"
>+  Call CharToASCII
>+  Pop "${AsciiCode}"
>+!macroend
>+Function CharToASCII
>+  Exch $0 ; given character
>+  Push $1 ; current character
>+  Push $2 ; current Ascii Code
>+
>+  StrCpy $2 1 ; right from start
>+Loop:
>+  IntFmt $1 %c $2 ; Get character from current ASCII code
>+  ${If} $1 S== $0 ; case sensitive string comparison
>+     StrCpy $0 $2
>+     Goto Done
>+  ${EndIf}
>+  IntOp $2 $2 + 1
>+  StrCmp $2 255 0 Loop ; ascii from 1 to 255
>+  StrCpy $0 0 ; ASCII code wasn't found -> return 0
>+Done:
>+  Pop $2
>+  Pop $1
>+  Exch $0
>+FunctionEnd
>+
>+; Taken from http://nsis.sourceforge.net/URLEncode
>+!define URLEncode "!insertmacro URLEncode"
>+!macro URLEncode String
>+  Push "${String}"
>+  Call URLEncode
>+  Pop "${String}"
>+!macroend
>+Function URLEncode
>+  Pop $0 ;param
>+  StrCpy $1 -1 ;init counter
>+  StrCpy $9 "" ; init buffer
>+
>+NextChar:
>+  IntOp $1 $1 + 1
>+  StrCpy $2 $0 1 $1 ;get char
>+
>+  ${If} $2 == ""
>+      Goto Done
>+  ${EndIf}
>+
>+  ${CharToASCII} $3 $2 ; get charcode
>+
>+  ${If} $3 == 32
>+      StrCpy $9 "$9+"
>+      Goto NextChar
>+  ${EndIf}
>+
>+  ${If} $3 == 95
>+  ${OrIf} $3 == 45
>+      StrCpy $9 $9$2
>+      Goto NextChar
>+  ${EndIf}
>+
>+  ${If} $3 >= 33
>+  ${AndIf} $3 <= 47
>+      IntFmt $4 "%X" $3
>+      StrCpy $9 $9%$4
>+      Goto NextChar
>+  ${EndIf}
>+
>+  ${If} $3 >= 58
>+  ${AndIf} $3 <= 64
>+      IntFmt $4 "%X" $3
>+      StrCpy $9 $9%$4
>+      Goto NextChar
>+  ${EndIf}
>+
>+  ${If} $3 >= 91
>+  ${AndIf} $3 <= 96
>+      IntFmt $4 "%X" $3
>+      StrCpy $9 $9%$4
>+      Goto NextChar
>+  ${EndIf}
>+
>+  ${If} $3 >= 123
>+      IntFmt $4 "%X" $3
>+      StrCpy $9 $9%$4
>+      Goto NextChar
>+  ${EndIf}
>+
>+  StrCpy $9 $9$2
>+  Goto NextChar
>+
>+Done:
>+  Push $9
>+FunctionEnd
>+
> Function SendPing
>   HideWindow
>   ; Try to send a ping if a download was attempted
>   ${If} $CheckboxSendPing == 1
>   ${AndIf} $IsDownloadFinished != ""
>+    ; Since this is an arbitrary string, it needs to be properly escaped.
>+    ${URLEncode} $PostSigningData
>+
>     ; Get the tick count for the completion of all phases.
>     System::Call "kernel32::GetTickCount()l .s"
>     Pop $EndFinishPhaseTickCount
> 
>     ; When the value of $IsDownloadFinished is false the download was started
>     ; but didn't finish. In this case the tick count stored in
>     ; $EndFinishPhaseTickCount is used to determine how long the download was
>     ; in progress.
>@@ -740,24 +839,25 @@ Function SendPing
>                       $\nExisting Version = $ExistingVersion \
>                       $\nExisting Build ID = $ExistingBuildID \
>                       $\nNew Version = $R5 \
>                       $\nNew Build ID = $R6 \
>                       $\nDefault Install Dir = $R7 \
>                       $\nHas Admin = $R8 \
>                       $\nDefault Status = $R2 \
>                       $\nSet As Sefault Status = $R3 \
>-                      $\nDownload Server IP = $DownloadServerIP"
>+                      $\nDownload Server IP = $DownloadServerIP \
>+                      $\nPost-Signing Data = $PostSigningData"
>     ; The following will exit the installer
>     SetAutoClose true
>     StrCpy $R9 "2"
>     Call RelativeGotoPage
> !else
>     ${NSD_CreateTimer} OnPing ${DownloadIntervalMS}
>-    InetBgDL::Get "${BaseURLStubPing}/${StubURLVersion}${StubURLVersionAppend}/${Channel}/${UpdateChannel}/${AB_CD}/$R0/$R1/$5/$6/$7/$8/$9/$ExitCode/$FirefoxLaunchCode/$DownloadRetryCount/$DownloadedBytes/$DownloadSizeBytes/$IntroPhaseSeconds/$OptionsPhaseSeconds/$0/$1/$DownloadFirstTransferSeconds/$2/$3/$4/$InitialInstallRequirementsCode/$OpenedDownloadPage/$ExistingProfile/$ExistingVersion/$ExistingBuildID/$R5/$R6/$R7/$R8/$R2/$R3/$DownloadServerIP" \
>+    InetBgDL::Get "${BaseURLStubPing}/${StubURLVersion}${StubURLVersionAppend}/${Channel}/${UpdateChannel}/${AB_CD}/$R0/$R1/$5/$6/$7/$8/$9/$ExitCode/$FirefoxLaunchCode/$DownloadRetryCount/$DownloadedBytes/$DownloadSizeBytes/$IntroPhaseSeconds/$OptionsPhaseSeconds/$0/$1/$DownloadFirstTransferSeconds/$2/$3/$4/$InitialInstallRequirementsCode/$OpenedDownloadPage/$ExistingProfile/$ExistingVersion/$ExistingBuildID/$R5/$R6/$R7/$R8/$R2/$R3/$DownloadServerIP/$PostSigningData" \
>                   "$PLUGINSDIR\_temp" /END
> !endif
>   ${Else}
>     ${If} "$IsDownloadFinished" == "false"
>       ; Cancel the download in progress
>       InetBgDL::Get /RESET /END
>     ${EndIf}
>     ; The following will exit the installer
>@@ -1992,16 +2092,21 @@ Function LaunchAppFromElevatedProcess
>   ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" ""
>   ${GetPathFromString} "$0" $0
>   ; Set the current working directory to the installation directory
>   ${GetParent} "$0" $1
>   SetOutPath "$1"
>   Exec "$\"$0$\""
> FunctionEnd
> 
>+Function CopyPostSigningData
You still need to read the file and place the contents in $PostSigningData

>+  CreateDirectory "$LOCALAPPDATA\mozilla\Firefox"
>+  CopyFiles /SILENT "$EXEDIR\postSigningData" "$LOCALAPPDATA\mozilla\Firefox"
>+FunctionEnd

No need to include the binary in future patches.

minusing since $PostSigningData still needs to be populated with the contents of the file

Comment 20

[Robert Strong (they/them - no direct email)]

Matt, when we don't have attribution data what should happen with the value of $PostSigningData? I'm tempted to go with the value 0 instead of an empty string.

Comment 21

[Robert Strong (they/them - no direct email)]

Also, let's just go with the 1st line of the file for the ping.

Comment 22

[Robert Strong (they/them - no direct email)]

bug 1259612 comment #10 also shows what this line should look like and no escaping should be necessary. This value will also need to be appended to the download url for the complete installer per this comment.

Comment 23

[Robert Strong (they/them - no direct email)]

At least that is how I read that comment... would be good to verify.

Comment 24

[Molly Howell (she/her) (no longer active)]

Attachment: Allow stub installer to read post-signing data and leave it where Firefox can find it

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #22)
> bug 1259612 comment #10 also shows what this line should look like and no
> escaping should be necessary. This value will also need to be appended to
> the download url for the complete installer per this comment.

I don't see anything in there to indicate to me that the full installer URL needs to change.

It does look like the URL encoding is handled before it reaches us, so I've taken that out of this patch. I found out by experimentation that wininet doesn't do URL encoding for you, so if any escaping does turn out to be necessary, then we do need to be doing it here.

I've removed the compiled 7-Zip stub from this patch, because the one in attachment 8754472 [details] [diff] [review] is the one that needs to land.

Comment 25

[Robert Strong (they/them - no direct email)]

(In reply to Matt Howell [:mhowell] from comment #24)
> Created attachment 8754530 [details] [diff] [review]
> Allow stub installer to read post-signing data and leave it where Firefox
> can find it
> 
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #22)
> > bug 1259612 comment #10 also shows what this line should look like and no
> > escaping should be necessary. This value will also need to be appended to
> > the download url for the complete installer per this comment.
> 
> I don't see anything in there to indicate to me that the full installer URL
> needs to change.
Yep, I misread it. Thanks!

Comment 26

[Robert Strong (they/them - no direct email)]

Comment on attachment 8754530 [details] [diff] [review]
Allow stub installer to read post-signing data and leave it where Firefox can find it

>diff --git a/browser/installer/windows/nsis/stub.nsi b/browser/installer/windows/nsis/stub.nsi
>--- a/browser/installer/windows/nsis/stub.nsi
>+++ b/browser/installer/windows/nsis/stub.nsi
> !include "nsDialogs.nsh"
> !include "LogicLib.nsh"
> !include "FileFunc.nsh"
Add (it is added when not present by common.nsh which is included by this file but I prefer it to be explicit)
!include TextFunc.nsh

> !include "WinVer.nsh"
> !include "WordFunc.nsh"

>@@ -1992,16 +1994,26 @@ Function LaunchAppFromElevatedProcess
>   ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" ""
>   ${GetPathFromString} "$0" $0
>   ; Set the current working directory to the installation directory
>   ${GetParent} "$0" $1
>   SetOutPath "$1"
>   Exec "$\"$0$\""
> FunctionEnd
> 
>+Function CopyPostSigningData
>+  ${LineRead} "$EXEDIR\postSigningData" "1" $PostSigningData
>+  ${If} ${Errors}
>+    StrCpy $PostSigningData "0"
Add
ClearErrors

>+  ${Else}
>+    CreateDirectory "$LOCALAPPDATA\Mozilla\Firefox"
>+    CopyFiles /SILENT "$EXEDIR\postSigningData" "$LOCALAPPDATA\Mozilla\Firefox"
>+  ${Endif}
>+FunctionEnd

Comment 27

[Robert Strong (they/them - no direct email)]

Would be good to verify everything is ok locally before landing this. If you want I can do it later.

Comment 28

[Robert Strong (they/them - no direct email)]

You also should add
> !insertmacro GetParameters
> !insertmacro GetOptions
!insertmacro LineFind
> !insertmacro StrFilter

Comment 29

[Molly Howell (she/her) (no longer active)]

Attachment: Allow stub installer to read post-signing data and leave it where Firefox can find it

Fixed nits, carrying over r+.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #27)
> Would be good to verify everything is ok locally before landing this. If you
> want I can do it later.

I just ran the whole procedure (build the installer, sign it, add attribution data, run it) and I get the expected data in the expected places. Looks like everything is working.

Comment 30

[Robert Strong (they/them - no direct email)]

One thing I recall discussing with you is identifying the install directory for the file but I'm not sure if it would provide any value though. There is also the case where someone performs multiple installs. At this time I *think* it should be up to Firefox to deal with it on launch. Let's discuss this further tomorrow.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/06d9362a57a1

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/06d9362a57a1

Comment 33

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8754555 [details] [diff] [review]
Allow stub installer to read post-signing data and leave it where Firefox can find it

Approval Request Comment
[Feature/regressing bug #]:
N/A, this is a new feature

[User impact if declined]:
From an e-mail by cmore:
I know Winston's team [Firefox desktop marketing] really needs retention data by
source/medium/campaign and they have been blind for pretty much ever.
When we talked about this idea at the start of the year, we hoped to
have it in the wild on the release channel by Q3.

[Describe test coverage new/current, TreeHerder]:
This patch has been on m-c since May 23 with no issues reported, and I manually checked it out on all supported Windows versions before I ever submitted it.

[Risks and why]:
Low risk, given testing described above.

[String/UUID change made/needed]:
N/A

Comment 34

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8754472 [details] [diff] [review]
patch - binary compiled with VC6

Approval Request Comment
See comment 33.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8754472 [details] [diff] [review]
patch - binary compiled with VC6

Good to have the stub installer data for 49, tested manually on m-c.

Comment 36

[[e-mail] (:bhearsum)]

(In reply to Molly Howell (she/her) [:mhowell] from comment #8)

This is a proof of concept for the method described above, containing the
"magic" certificate, a Python script to fill in an attribution code/other
data after signing, and a patch that gets the data to the stub installer for
it to start working with. Here's how to use this stuff:

1. Apply the .diff to mozilla-central and build the stub installer normally.
2. Sign the stub installer as usual, but add the osslsigncode option "-ac
   MozDummy.cer".
3. Run write_attribution_data.py and pass it the installer file name and the
   string you want to write.
4. Run the stub installer and just leave it open, don't do anything.
5. Open 7-Zip's temp folder (should be at "C:\Users[your
   username]\AppData\Local\Temp\7z[random stuff].tmp") and you should see a
   file called postSigningData which contains the string you supplied. (I don't
   have the code written yet to do anything else with the data from there.)

I had need to test this for https://bugzilla.mozilla.org/show_bug.cgi?id=1616355, and modified these instructions a bit to make them work these days. Now, it's:

1. Push to try and select one of the repackage-signing tasks (eg: repackage-signing-win64-shippable/opt)
2. Download target.installer.exe after the Task completes
3. Run write_attribution_data.py to inject the attribution data. Eg: python3 write_attribution_data.py target.installer.exe 'source%3Dgoogle.com%26medium%3Dorganic%26campaign%3D(not%20set)%26content%3Dbhearsum234'

It's also easier to verify these days -- attribution data should show up in about:telemetry shortly after starting Firefox.

Comment 37

[[e-mail] (:bhearsum)]

Attachment: GenerateDummyCert.ps1

While reviewing this bug again yesterday I realized that we never wrote down how to generate the dummy cert used here. Thankfully, Molly still had a copy of this script that she used to do so. It must be hex edited afterwards to add the __MOZCUSTOM__ tag, but that's trivial enough. Thanks Molly!