Closed Bug 1261140 Opened 4 years ago Closed 3 years ago

Stub installer support for attribution

Categories

(Firefox :: Installer, enhancement)

All
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: mhowell, Assigned: mhowell)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 7 obsolete files)

2.65 KB, text/plain
Details
2.49 KB, application/x-x509-ca-cert
Details
81.70 KB, patch
Details | Diff | Splinter Review
10.95 KB, patch
mhowell
: review+
Details | Diff | Splinter Review
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).
Attached file Prototype installer editing script (obsolete) —
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).
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.
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.
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.
Attachment #8737247 - Attachment is obsolete: true
Attachment #8738657 - Flags: feedback?(robert.strong.bugs)
(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 on attachment 8738657 [details] [diff] [review]
Prototype 7-Zip SFX patch for retriving attribution data

Looks good
Attachment #8738657 - Flags: feedback?(robert.strong.bugs) → feedback+
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.
Attached file Method 3 POC (obsolete) —
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.)
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.
Attachment #8738657 - Attachment is obsolete: true
Attachment #8748190 - Flags: review?(robert.strong.bugs)
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.
Attachment #8737240 - Attachment is obsolete: true
Attachment #8748190 - Attachment description: bug1261140.diff → Allow stub installer to read post-signing data and leave it where Firefox can find it
Attached file Dummy Certificate
Attachment #8744471 - Attachment is obsolete: true
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.
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
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 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+
Attachment #8748190 - Flags: review?(robert.strong.bugs) → review-
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
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.
Attachment #8748190 - Attachment is obsolete: true
Attachment #8754444 - Flags: review?(robert.strong.bugs)
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
Attachment #8754444 - Flags: review?(robert.strong.bugs) → review-
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.
Also, let's just go with the 1st line of the file for the ping.
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.
At least that is how I read that comment... would be good to verify.
(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.
Attachment #8754444 - Attachment is obsolete: true
Attachment #8754530 - Flags: review?(robert.strong.bugs)
(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 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
Attachment #8754530 - Flags: review?(robert.strong.bugs) → review+
Would be good to verify everything is ok locally before landing this. If you want I can do it later.
You also should add
> !insertmacro GetParameters
> !insertmacro GetOptions
!insertmacro LineFind
> !insertmacro StrFilter
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.
Attachment #8754530 - Attachment is obsolete: true
Attachment #8754555 - Flags: review+
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.
https://hg.mozilla.org/mozilla-central/rev/06d9362a57a1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
Attachment #8754555 - Flags: approval-mozilla-aurora?
Comment on attachment 8754472 [details] [diff] [review]
patch - binary compiled with VC6

Approval Request Comment
See comment 33.
Attachment #8754472 - Flags: approval-mozilla-aurora?
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.
Attachment #8754472 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8754555 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.