Closed Bug 322206 (StubInstaller) Opened 19 years ago Closed 12 years ago

Firefox net / stub installer for x86

Categories

(Firefox :: Installer, defect)

Product:
Firefox
Component:
Installer
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 18

People

(Reporter: benjamin, Assigned: robert.strong.bugs)

References

(Depends on 3 open bugs,
URL
)

Details

(Keywords: sec-want, Whiteboard: [sg:want])

Attachments

(20 files, 21 obsolete files)

3. New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders)
1.08 MB, patch
bbondy
: review+
Details | Diff | Splinter Review
5. binary resource containing the new ui for checkin
2.56 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
binary resource containing the new ui
6.00 KB, application/octet-stream
Details
4. Makefile changes for new plugins and artwork rev2
4.01 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
6. locale files (generic) and Makefile changes for new ui
2.38 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
artwork zip file for reference
196.73 KB, application/octet-stream
Details
7. Common code
14.45 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
10. stub patch in progress rev2
39.43 KB, patch
Details | Diff | Splinter Review
8. Locale support
3.73 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
5. binary resource containing the new ui for checkin rev2
2.57 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
9. configure changes
1.25 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.extra CertCheck NSIS plugin code rev3
19.92 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
1. CertCheck NSIS plugin dll for checkin
2.47 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
2.extra InetBgDL NSIS plug-in code rev3
20.61 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
2. InetBgDL NSIS plug-in dll for checkin
2.71 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
11. main patch rev4
48.53 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
12. Remove DownloadDir and use $PLUGINSDIR rev2
8.19 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
Fixes 2 rev1
3.88 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
Fixes 1 rev2
11.12 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
Missed !insertmacro GetPathFromString
815 bytes, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
We want/need a new installer for Firefox 2.0 which allows for stub downloads as well as ditching the xpinstall system in favor of flatter archives. Thunderbird should probably have something similar; the amount of shared code depends on the architecture of the new installer. Beltzner's initial-cut UI spec is wiki.mozilla.org/User:Beltzner/Specification_of_Stub_Installer_User_Interface Rob Strong will take the lead here and decide whether to use existing technologies such as NSIS, or even switch entirely to a MSI-based solution (using WIX?). This installer will also lead us to the Firefox 3 installer which is XULRunner+app... we're going to try and develop a shared solution to reduce development overhaul.
URL: wiki.mozilla.org/User:Beltzner/Specif... → http://wiki.mozilla.org/User:Beltzner...
OS: MacOS X → All
Hardware: Macintosh → All
Depends on: 263446
Depends on: 326580
*** Bug 238212 has been marked as a duplicate of this bug. ***
Summary: Firefox 2.0 stub installer → Firefox stub installer
Assignee: robert.bugzilla → nobody
We're hearing more and more software vendors would like to bundle (an unmodified version of) Firefox or Thunderbird with their application. Google Chrome also has a net installer. This should be seriously considered.
Summary: Firefox stub installer → Firefox net (stub) installer
Jim Mathies made one for Firefox distributions a couple of years ago... cc'ing him
This is pretty straightforward to do with Windows Installer (you can pass a URL to Windows Installer and it will do "The Right Thing") but would be a lot more difficult if we wanted it to be cross platform. The bug is marked All but the description and Beltzner's spec both refer to Windows ...
jmathies: do you have a sample of net installer you made before?
(In reply to comment #6) > jmathies: do you have a sample of net installer you made before? No, but the app wouldn't be hard to recreate. I'm not sure a stand alone exe downloader is the best approach these days though. Do we have a basic list of requirements from vendors? How would they like to integrate our net installer with their software?
Although we don't have specific requirements, we'd like to have a generic net installer that other executables are also able to invoke. Consider some cases: * PC makers which would like to bundle Firefox/Thunderbird * external HDD/USB makers would like to bundle Firefox/Thunderbird * ISPs which would like to add "Install Thunderbird" button on their own setup tool Personally I like ChromeSetup.exe -- simple and fast.
Summary: Firefox net (stub) installer → Firefox net / stub installer
Blocks: 358384
what's holding this up from happening.
Whiteboard: [sg:want]
No longer depends on: 263446
Other work that has been given a higher priority. It would be helpful to those that have to do the work if the security issue (e.g. sg:want) this helps with or resolves was provided. Thanks
The primary security benefit is letting us fix bug 358384 without spending gobs of money. See bug 358384 comment 19. A secondary security benefit is letting us get 64-bit Windows builds to computers that support them. 64-bit builds have much stronger ASLR, and ASLR has been a key line of defense against exploitation for the last few years. But this also depends on us actually supporting 64-bit builds ;)
We aren't going to have the same stub installer for all platforms so limiting this bug to Windows only.
OS: All → Windows 7
Hardware: All → x86
Depends on: 651965
Let me get it out of my system, I despise net installers (why to wait for download again after I've "downloaded" the installer, there's people out there that still doesn't have always on connectivity) (In reply to comment #12) > A secondary security benefit is letting us get 64-bit Windows builds to > computers that support them. Wouldn't sniffing for "WOW64" in the user agent determine 64-bit Windows "reliably"? Non scientific test, I went to http://whatsmyuseragent.com/ on 32-bit IE8 on Windows 7 64-bit and it reports " Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64 [...]"
Depends on: 675970
Have you guys considered the use of TUF ( https://www.updateframework.com/ ) as a way to make this happen?
I've looked at it and it doesn't seem like a good fit for what we want to accomplish with this bug.
TUF seems to cover all of the likely security issues that are known and it covers them in a safe way. What are the objections? Another that is basically TUF: https://gitweb.torproject.org/thandy.git
For starters, this is an installer and not an updater and we will be using the same ui and the majority of the same code when installing with a full installer as with the stub.
Thandy is an installer and an updater. TUF is the same. They both place a heavy focus on secure updating but they both also care very much about the initial install. This seems pretty relevant to MoCo. What are the other issues?
Can you provide details as to the specific benefits that TUF will provide? As a matter if fact, please do so in Bug 696472.
Yeah, I'll put it in the other bug. For anyone reading along at home, please read these: https://www.updateframework.com/wiki/About https://www.updateframework.com/browser/specs/tuf-spec.txt http://freehaven.net/~arma/tuf-ccs2010.pdf Just to crib from the spec and why TUF is so important as a bare minimum: 1.5.2. Goals for specific attacks to protect against Note: When saying the framework protects against an attack, this means that the attack will not be successful. It does not mean that a client will always be able to successfully update during an attack. Fundamentally, an attacker positioned to intercept and modify a client's communication will always be able to perform a denial of service. The part we have control over is not allowing an inability to update to go unnoticed. Rollback attacks. Attackers should not be able to trick clients into installing software that is older than that which the client previously knew to be available. Indefinite freeze attacks. Attackers should not be able respond to client requests with the same, outdated metadata without the client being aware of the problem. Endless data attacks. Attackers should not be able to respond to client requests with huge amounts of data (extremely large files) that interfere with the client's system. Slow retrieval attacks. Attackers should not be able to prevent clients from being aware of interference with receiving updates by responding to client requests so slowly that automated updates never complete. Extraneous dependencies attacks. Attackers should not be able to cause clients to download or install software dependencies that are not the intended dependencies. Mix-and-match attacks. Attackers should not be able to trick clients into using a combination of metadata that never existed together on the repository at the same time. Malicious repository mirrors should not be able to prevent updates from good mirrors.
From the TUF docs, TUF is a security framework for installing installers. For example: https://gitweb.torproject.org/thandy.git/blob/HEAD:/specs/thandy-spec.txt#l119 states that the source could be an MSI which *IS* an installer. So, with what we are trying to accomplish with the stub installer we would need to implement it to install TUF if we were to go with TUF. We can evaluate whether there is value in using TUF in bug 696472 but TUF does not negate our need for a stub installer.
(In reply to Robert Strong [:rstrong] (do not email) from comment #24) > From the TUF docs, TUF is a security framework for installing installers. > For example: > https://gitweb.torproject.org/thandy.git/blob/HEAD:/specs/thandy-spec. > txt#l119 > > states that the source could be an MSI which *IS* an installer. So, with > what we are trying to accomplish with the stub installer we would need to > implement it to install TUF if we were to go with TUF. Yes, you have to install the TUF installer to install all the other components securely. > We can evaluate > whether there is value in using TUF in bug 696472 but TUF does not negate > our need for a stub installer. The stub installer needs to solve all of the issues that TUF addresses - otherwise a network attacker can serve an old firefox to a new installer, no? It sounds like an MSI wrapper around a TUF binary is what is needed.
You can wrap it in an MSI, download a TUF installer with a stub, etc. and the point I am making is that TUF doesn't remove the need for a stub installer, etc. since TUF itself needs to be installed. Also, the stub installer needs to securely and safely handle a subset of what TUF does. btw: the reason I commented here was to let the people cc'd on this bug know that TUF itself doesn't solve what this bug is going to solve and further advocacy comments regarding TUF should be made in bug 696472.
(In reply to Robert Strong [:rstrong] (do not email) from comment #26) > You can wrap it in an MSI, download a TUF installer with a stub, etc. and > the point I am making is that TUF doesn't remove the need for a stub > installer, etc. since TUF itself needs to be installed. Also, the stub > installer needs to securely and safely handle a subset of what TUF does. > That's the part where we disagree. I suggest that you take a TUF installer and wrap that in a signed MSI. TUF seems like it will be a fine stub. > btw: the reason I commented here was to let the people cc'd on this bug know > that TUF itself doesn't solve what this bug is going to solve and further > advocacy comments regarding TUF should be made in bug 696472. As far as I can tell, TUF would meet the needs of any reasonable stub. In any case, I see in wiki.mozilla.org/User:Beltzner/Specification_of_Stub_Installer_User_Interface that it's all UI and no security concerns. Seems unwise. Is there a real list of requirements that talks about anything other than the UI?
Mozilla China already implemented a stub installer and uses it by default for the China edition of Firefox. I do not know if/how much we could reuse what they have done, but it seems like a good goal to get the China edition to use the same stub installer as regular Firefox, so that we know how the China edition installer works and so Mozilla China, secteam, and the installer team are all on the same page.
Using the "net installer" isn't going to be default from the Mozilla site, correct? (I understand the full installer will use the same underlying code, just without the downloader part) Would another option be to have the application binary in the core download, and then have it pull just language packs? (This could give many other advantages, less data to replicate between mirrors, etc)
Because of the way Mozilla currently builds Firefox, the languages are built into the big DLL and not separated.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
On the topic of a stub installer: * will Mozilla continue to provide a standaline .exe installer for Windows (for offline installs, etc.) once the stub installer is released or will it be 'hidden away' from the main download page? * will a stub installer trigger any _runtime_ behaviour alerts of popular anti-virus engines (eg. an .exe downloads and runs another .exe -> raise alert)? I'd suggest some testing in that area.
(In reply to John Hopkins (:jhopkins) from comment #31) > On the topic of a stub installer: > * will Mozilla continue to provide a standaline .exe installer for Windows > (for offline installs, etc.) once the stub installer is released or will it > be 'hidden away' from the main download page? The plan is to also provide a full installer and I don't expect that to change since deployments will need this. > * will a stub installer trigger any _runtime_ behaviour alerts of popular > anti-virus engines (eg. an .exe downloads and runs another .exe -> raise > alert)? I'd suggest some testing in that area. We have done various stub installers over the years and it I am fairly certain it won't anymore than any other stub installer (or for that matter our current installer which is also based on NSIS). One example, some Firewalls can be configured to require the user to allow network access for the stub. And of course we will test.
Status: ASSIGNED → NEW
Depends on: 776440, 597536, 398366
FYI - Proposal for testing this feature is here - https://etherpad.mozilla.org/stub-installer-testing.
QA Contact: jsmith
Whiteboard: [sg:want] → [sg:want], [qa+]
Keywords: verifyme
Whiteboard: [sg:want], [qa+] → [sg:want]
Attached patch CertCheck NSIS plugin code (obsolete) — — Splinter Review
I considered going with just including certificatecheck.h / certificatecheck.cpp but decided against it since I'd like to make this available on the NSIS site as well. With that in mind, this wouldn't need to land if I make it available on the NSIS site.
Attachment #527466 - Attachment is obsolete: true
Attachment #661692 - Flags: review?(netzen)
Attached file CertCheck NSIS dll (obsolete) —
Attachment #661692 - Attachment description: CertCheck NSIS plugin → CertCheck NSIS plugin code
Attached file 1. CertCheck dll for checkin (obsolete) —
This is the diff of the dll for checkin if the code passes review
Depends on: 791613
Attachment #661695 - Attachment description: dll for checkin → CertCheck dll for checkin
Brian, could you take a look at the code in the following plugin? http://nsis.sourceforge.net/InetBgDL_plug-in
Attachment #661701 - Flags: review?
Attachment #661700 - Attachment description: InetBgDL NSIS plug-in → InetBgDL NSIS plug-in dll
Comment on attachment 661702 [details] [diff] [review] 3. New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders) For now the official artwork will be used for all branding until the new artwork is available.
Attachment #661702 - Attachment description: New artwork → New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders)
Attachment #661701 - Flags: review? → review?(netzen)
also, generic locale config files
Attachment #661694 - Attachment mime type: text/plain → application/octet-stream
Awesome! I'll be reviewing in the morning.
Robert: Maybe you already mentioned it somewhere, but I can't find it atm: Do I need to include new artwork in the SeaMonkey code as well so that the NSIS build process won't break or is this optional for now (not sure if we want/need a stub installer, we'll see)?
You don't. Artwork is entirely app specific and copied by app Makefiles, etc. http://mxr.mozilla.org/mozilla-central/source/browser/branding/nightly/
Attachment #661701 - Flags: review?(netzen) → review+
Comment on attachment 661702 [details] [diff] [review] 3. New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders) Review of attachment 661702 [details] [diff] [review]: ----------------------------------------------------------------- I *think* these could fit into a color table with 16 colors (4bpp), which would half the size of the bmps. But that can be done/investigate in a follow up bug after this all lands.
Attachment #661702 - Flags: review?(netzen) → review+
Comment on attachment 661705 [details] [diff] [review] 4. Makefile changes for new plugins and artwork Review of attachment 661705 [details] [diff] [review]: ----------------------------------------------------------------- Impressive that you even flipped the pencil bmp for RTL :) I assume these files are coming in a different patch? locale.nlf \ locale-rtl.nlf \
Attachment #661705 - Flags: review?(netzen) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #47) > Comment on attachment 661705 [details] [diff] [review] > Makefile changes for new plugins and artwork > > Review of attachment 661705 [details] [diff] [review]: > ----------------------------------------------------------------- > > Impressive that you even flipped the pencil bmp for RTL :) > > I assume these files are coming in a different patch? > locale.nlf \ > locale-rtl.nlf \ Forgot to hg add them. Since I accidentally deleted the ui resource file and had to recreate it so I'll add them there.
Comment on attachment 661701 [details] [diff] [review] 2. InetBgDL NSIS plug-in for checkin Review of attachment 661701 [details] [diff] [review]: ----------------------------------------------------------------- I think it's scary to add binaries without source, could we also include the source for the matching binary too? other-licenses\nsis\Contrib http://nsis.sourceforge.net/InetBgDL_plug-in
Actually, they are in attachment #661705 [details] [diff] [review] but they are utf-16le. I'll resubmit attachment #661705 [details] [diff] [review] without it with the r+ and submit it separately as utf-8.
(In reply to Brian R. Bondy [:bbondy] from comment #49) > Comment on attachment 661701 [details] [diff] [review] > InetBgDL NSIS plug-in for checkin > > Review of attachment 661701 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think it's scary to add binaries without source, could we also include the > source for the matching binary too? > other-licenses\nsis\Contrib > http://nsis.sourceforge.net/InetBgDL_plug-in We could but we typically only do that when we modify source and just use the existing source. I also see this as little different than the binaries we use in Mozilla Build for this case.
I like to include source for any binary in case the code ever disappears 10 years from now and someone needs to make a change. But if you're against it, that's OK :D
Attachment #661718 - Attachment description: binary resource containing the new ui → binary resource containing the new ui for checkin
Attachment #661719 - Attachment mime type: text/plain → application/octet-stream
Attachment #661718 - Flags: review?(netzen) → review+
Comment on attachment 661721 [details] [diff] [review] generic locale files and Makefile changes Missed a couple of things
Attachment #661721 - Attachment is obsolete: true
Attachment #661721 - Flags: review?(netzen)
Removed references to locale.nlf and locale-rtl.nlf. Carrying forward r+
Attachment #661724 - Flags: review+
Attachment #661695 - Attachment description: CertCheck dll for checkin → 1. CertCheck dll for checkin
Attachment #661701 - Attachment description: InetBgDL NSIS plug-in for checkin → 2. InetBgDL NSIS plug-in for checkin
Attachment #661702 - Attachment description: New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders) → 3. New artwork (still need artwork for Aurora, Nightly, and unofficial... these are placeholders)
Attachment #661705 - Attachment description: Makefile changes for new plugins and artwork → 4. Makefile changes for new plugins and artwork
Attachment #661705 - Attachment is obsolete: true
Attachment #661725 - Flags: review?(netzen) → review+
Attachment #661724 - Attachment description: Makefile changes for new plugins and artwork rev2 → 4. Makefile changes for new plugins and artwork rev2
Attachment #661718 - Attachment description: binary resource containing the new ui for checkin → 5. binary resource containing the new ui for checkin
(In reply to Brian R. Bondy [:bbondy] from comment #52) > I like to include source for any binary in case the code ever disappears 10 > years from now and someone needs to make a change. But if you're against > it, that's OK :D I'm ok with it... I'll put together a patch tomorrow for that.
Attachment #661725 - Attachment description: locale files (generic) and Makefile changes for new ui → 6. locale files (generic) and Makefile changes for new ui
Attachment #661695 - Flags: review?(netzen)
Attachment #661692 - Attachment description: CertCheck NSIS plugin code → 1-extra. CertCheck NSIS plugin code
Comment on attachment 661692 [details] [diff] [review] CertCheck NSIS plugin code Review of attachment 661692 [details] [diff] [review]: ----------------------------------------------------------------- Since this code is used in 2 places, it would be good to have security pass through this patch as well to make sure we're using WinVerifyTrust as they expect we should. They did do a review of the service, but that was a ton of code. I think a targeted review of CertCheck.cpp only would be worth the hassle (but should not hold up landing). ::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp @@ +203,5 @@ > + > +/** > + * Verifies the trust of the specified file path. > + * > + * @param stacktop A pointer to the top of the stack nit: Pls add what you're expecting to be pushed on the stack, i.e. filepath, certname, and certIssuer, where file path is the top of the stack. @@ +205,5 @@ > + * Verifies the trust of the specified file path. > + * > + * @param stacktop A pointer to the top of the stack > + * @param variables A pointer to the NSIS variables > + * @return 1 if the trust was verified, 0 otherwise nit: Also indicate that the stack will be modified to contain "1" if successful @@ +278,5 @@ > + trustData.pFile = &fileToCheck; > + > + GUID policyGUID = WINTRUST_ACTION_GENERIC_VERIFY_V2; > + // Check if the file is signed by something that is trusted. > + LONG ret = WinVerifyTrust(NULL, &policyGUID, &trustData); nit: just return ret here, the code below doesn't matter since there is no more logging in this code. @@ +290,5 @@ > + > +/** > + * Verifies the trust of the specified file path. > + * > + * @param stacktop A pointer to the top of the stack nit: document how the stack will be used and modified as above.
Attachment #661692 - Attachment description: 1-extra. CertCheck NSIS plugin code → CertCheck NSIS plugin code
Attachment #661692 - Flags: review?(netzen) → review+
Comment on attachment 661695 [details] 1. CertCheck dll for checkin r+ assuming this was built with VC6
Attachment #661695 - Flags: review?(netzen) → review+
Attachment #661694 - Flags: review+
Attachment #661700 - Flags: review+
Comment on attachment 661695 [details] 1. CertCheck dll for checkin Didn't have time to setup my VC6 environment. Would you by chance still have yours setup?
Attachment #661695 - Attachment is obsolete: true
I think I have a VM with it setup, I'll check into it tomorrow when I'm at that laptop.
Comment on attachment 661692 [details] [diff] [review] CertCheck NSIS plugin code Ian, this is essentially the same code as used by the maintenance service's certificate check. It doesn't include those files so this can be a stand alone NSIS plugin that will be submitted to the NSIS site. Could you review it just in case?
Attachment #661692 - Flags: review?(imelven)
(In reply to Robert Strong [:rstrong] (do not email) from comment #63) > Comment on attachment 661695 [details] > 1. CertCheck dll for checkin > > Didn't have time to setup my VC6 environment. Would you by chance still have > yours setup? Crap, I don't have it anymore. I think it was on my win2k VM. I think we should just file one or two follow ups to reduce the installer file size via 4bpp bmps and also compiling this dll with vc6.
Btw - Feel free to drop a reference to any builds for testing here. I'll start testing it when I get a reference.
(In reply to Robert Strong [:rstrong] (do not email) from comment #65) > Comment on attachment 661692 [details] [diff] [review] > CertCheck NSIS plugin code > > Ian, this is essentially the same code as used by the maintenance service's > certificate check. It doesn't include those files so this can be a stand > alone NSIS plugin that will be submitted to the NSIS site. Could you review > it just in case? I will take a look ASAP. bsmith, I know you are very busy but you might want to have a look as well.
Comment on attachment 661692 [details] [diff] [review] CertCheck NSIS plugin code Review of attachment 661692 [details] [diff] [review]: ----------------------------------------------------------------- It might be good to make sure the stub installer binaries have ASLR and /GS and all that goodness turned on, but that's a separate/followup bug IMO. A few nits, but the string NULL termination stuff are the only real potential issues, I think. ::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp @@ +33,5 @@ > + * @param certContext The certificate context of the file > + * @param infoToMatch The acceptable information to match > + * @return FALSE if the info does not match or if any error occurs in the check > + */ > +BOOL nit: trailing whitespace @@ +34,5 @@ > + * @param infoToMatch The acceptable information to match > + * @return FALSE if the info does not match or if any error occurs in the check > + */ > +BOOL > +DoCertificateAttributesMatch(PCCERT_CONTEXT certContext, nit : trailing whitespace @@ +42,5 @@ > + LPTSTR szName = NULL; > + > + if (infoToMatch.issuer) { > + // Pass in NULL to get the needed size of the issuer buffer. > + dwData = CertGetNameString(certContext, nit : trailing whitespace @@ +97,5 @@ > + return FALSE; > + } > + > + // If the issuer does not match, return a failure. > + if (!infoToMatch.name || nit: trailing whitespace @@ +108,5 @@ > + LocalFree(szName); > + } > + > + // If there were any errors we would have aborted by now. > + return TRUE; still not sure about returning TRUE in the case when neither a name or issue is passed in @@ +120,5 @@ > + * @return ERROR_SUCCESS if successful, ERROR_NOT_FOUND if the info > + * does not match, or the last error otherwise. > + */ > +DWORD > +CheckCertificateForPEFile(LPCWSTR filePath, nit: trailing whitespace @@ +124,5 @@ > +CheckCertificateForPEFile(LPCWSTR filePath, > + CertificateCheckInfo &infoToMatch) > +{ > + HCERTSTORE certStore = NULL; > + HCRYPTMSG cryptMsg = NULL; nit: trailing whitespace @@ +132,5 @@ > + > + // Get the HCERTSTORE and HCRYPTMSG from the signed file. > + DWORD encoding, contentType, formatType; > + BOOL result = CryptQueryObject(CERT_QUERY_OBJECT_FILE, > + filePath, nit: trailing whitespace @@ +134,5 @@ > + DWORD encoding, contentType, formatType; > + BOOL result = CryptQueryObject(CERT_QUERY_OBJECT_FILE, > + filePath, > + CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED_EMBED, > + CERT_QUERY_CONTENT_FLAG_ALL, nit: trailing whitespace @@ +144,5 @@ > + } > + > + // Pass in NULL to get the needed signer information size. > + DWORD signerInfoSize; > + result = CryptMsgGetParam(cryptMsg, CMSG_SIGNER_INFO_PARAM, 0, nit: trailing whitespace this assumes there's only one signer, which i assume there always will be for now for our signed packages/updates @@ +160,5 @@ > + } > + > + // Get the signer information (PCMSG_SIGNER_INFO). > + // In particular we want the issuer and serial number. > + result = CryptMsgGetParam(cryptMsg, CMSG_SIGNER_INFO_PARAM, 0, nit: trailing whitespace @@ +168,5 @@ > + goto cleanup; > + } > + > + // Search for the signer certificate in the certificate store. > + CERT_INFO certInfo; nit: trailing whitespace @@ +171,5 @@ > + // Search for the signer certificate in the certificate store. > + CERT_INFO certInfo; > + certInfo.Issuer = signerInfo->Issuer; > + certInfo.SerialNumber = signerInfo->SerialNumber; > + certContext = CertFindCertificateInStore(certStore, ENCODING, 0, nit: trailing whitespace @@ +204,5 @@ > +/** > + * Verifies the trust of the specified file path. > + * > + * @param stacktop A pointer to the top of the stack > + * @param variables A pointer to the NSIS variables nit : trailing whitespace @@ +208,5 @@ > + * @param variables A pointer to the NSIS variables > + * @return 1 if the trust was verified, 0 otherwise > + */ > +extern "C" void __declspec(dllexport) > +VerifyCertNameIssuer(HWND hwndParent, int string_size, nit: trailing whitespace @@ +229,5 @@ > + MultiByteToWideChar(CP_ACP, 0, tmp3, -1, certIssuer, MAX_PATH); > +#else > + wcscpy(filePath, tmp1); > + wcscpy(certName, tmp2); > + wcscpy(certIssuer, tmp3); would prefer wcsncpy and explicit null termination of all these @@ +234,5 @@ > +#endif > + > + CertificateCheckInfo allowedCertificate = { > + certName, > + certIssuer, nit: trailing whitespace after both lines above @@ +267,5 @@ > + trustData.cbStruct = sizeof(trustData); > + trustData.pPolicyCallbackData = NULL; > + trustData.pSIPClientData = NULL; > + trustData.dwUIChoice = WTD_UI_NONE; > + trustData.fdwRevocationChecks = WTD_REVOKE_NONE; nit: trailing whitespace this seems to leave whether revocation checks are done up to the authenticode policy provider's default, which seems ok. @@ +278,5 @@ > + trustData.pFile = &fileToCheck; > + > + GUID policyGUID = WINTRUST_ACTION_GENERIC_VERIFY_V2; > + // Check if the file is signed by something that is trusted. > + LONG ret = WinVerifyTrust(NULL, &policyGUID, &trustData); the use of WinVerifyTrust looks ok to me - we should test to make sure we're correct about what we can do with respect to revocation etc as part of shipping this feature, IMO @@ +291,5 @@ > +/** > + * Verifies the trust of the specified file path. > + * > + * @param stacktop A pointer to the top of the stack > + * @param variables A pointer to the NSIS variables nit: trailing whitespace @@ +295,5 @@ > + * @param variables A pointer to the NSIS variables > + * @return 1 if the trust was verified, 0 otherwise > + */ > +extern "C" void __declspec(dllexport) > +VerifyCertTrust(HWND hwndParent, int string_size, nit: trailing whitespace @@ +304,5 @@ > + > + popstring(stacktop, tmp, MAX_PATH); > + > +#if !defined(UNICODE) > + MultiByteToWideChar(CP_ACP, 0, tmp, -1, filePath, MAX_PATH); MultiByteToWideChar does not guarantee terminating strings properly @@ +306,5 @@ > + > +#if !defined(UNICODE) > + MultiByteToWideChar(CP_ACP, 0, tmp, -1, filePath, MAX_PATH); > +#else > + wcscpy(filePath, tmp); would prefer wcsncpy @@ +317,5 @@ > + pushstring(stacktop, TEXT("0"), 2); > + } > +} > + > +BOOL WINAPI nit: trailing whitespace @@ +340,5 @@ > + return 1; > + } > + > + th = (*stacktop); > + lstrcpyn(str,th->text, len); might be safer to explicitly null terminate the string always @@ +357,5 @@ > +*/ > +void pushstring(stack_t **stacktop, const TCHAR *str, int len) > +{ > + stack_t *th; > + if (!stacktop) { nit: trailing whitespace @@ +362,5 @@ > + return; > + } > + > + th = (stack_t*)GlobalAlloc(GPTR, sizeof(stack_t) + len); > + lstrcpyn(th->text, str, len); might be safer to explicitly null terminate the string always
Comment on attachment 661692 [details] [diff] [review] CertCheck NSIS plugin code r+ but i would like the string termination stuff to be more bulletproof.
Attachment #661692 - Flags: review?(imelven) → review+
Flags: sec-review?(yboily)
Comment on attachment 661692 [details] [diff] [review] CertCheck NSIS plugin code Review of attachment 661692 [details] [diff] [review]: ----------------------------------------------------------------- ::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp @@ +216,5 @@ > + TCHAR tmp2[MAX_PATH] = { L'\0' }; > + TCHAR tmp3[MAX_PATH] = { L'\0' }; > + WCHAR filePath[MAX_PATH] = { '\0' }; > + WCHAR certName[MAX_PATH] = { '\0' }; > + WCHAR certIssuer[MAX_PATH] = { '\0' }; nit: The WCHAR variations should be L'\0', the TCHAR ones _T('\0') Also changing all of these declarations to MAX_PATH + 1 I think would solve the potential null termination issues
Brian, could you check if I made the requested changes. I went with your suggestion in comment #71. I didn't change pushstring or popstring either since it is used pretty consistently in the NSIS plugin code and if it is changed we can just create a helper that all of our NSIS plugin code uses.
Attachment #661692 - Attachment is obsolete: true
Attachment #662018 - Flags: review?(netzen)
I'm pretty certain I have VC 6 around somewhere and I'll recompile the plugin after I find it.
Attachment #662018 - Attachment description: CertCheck NSIS plugin code rev2 → 1.extra CertCheck NSIS plugin code rev2
Attached patch 7. Common code (obsolete) — — Splinter Review
Attachment #662081 - Flags: review?(netzen)
Attached patch 8. Locale support (obsolete) — — Splinter Review
I'm still working on some of the one off UI and if I have to add more strings I'll do so in a future patch.
Attachment #662088 - Flags: review?(netzen)
Attached patch 9. stub patch in progress (obsolete) — — Splinter Review
Brian, there are still a couple of more things to do such as providing a link to download the full installer if the cert checks fail... I'm submitting it for your reference.
Brian, though bug 791613 won't help with the stub initially I'd like to land it with this at the same time and possibly try to jump a couple of trains as well.
Comment on attachment 662018 [details] [diff] [review] 1.extra CertCheck NSIS plugin code rev2 Review of attachment 662018 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: other-licenses/nsis/Contrib/CertCheck/CertCheck.cpp @@ +249,5 @@ > + MultiByteToWideChar(CP_ACP, 0, tmp3, -1, certIssuer, MAX_PATH); > +#else > + wcscpy(filePath, tmp1); > + wcscpy(certName, tmp2); > + wcscpy(certIssuer, tmp3); nit: might as well change these to wcsncpy(..., MAX_PATH); x3 @@ +315,5 @@ > +VerifyCertTrust(HWND hwndParent, int string_size, > + TCHAR *variables, stack_t **stacktop, void *extra) > +{ > + TCHAR tmp[MAX_PATH] = { _T('\0') }; > + WCHAR filePath[MAX_PATH] = { L'\0' }; nit: Make these MAX_PATH + 1 to guarantee null termination below @@ +322,5 @@ > + > +#if !defined(UNICODE) > + MultiByteToWideChar(CP_ACP, 0, tmp, -1, filePath, MAX_PATH); > +#else > + wcscpy(filePath, tmp); nit: Might as well change this to wcsncpy(..., MAX_PATH);
Attachment #662018 - Flags: review?(netzen) → review+
Comment on attachment 662088 [details] [diff] [review] 8. Locale support Review of attachment 662088 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/installer/nsisstrings.properties @@ +22,5 @@ > + > +INTRO_BLURB=Thanks for choosing $BrandFullName, the browser that chooses you above everything else. Click to install. > +INSTALL_BLURB1=You're about to enjoy the very latest in speed, flexibility and security so you're always in control. > +INSTALL_BLURB2=That’s because $BrandShortName is made by a non-profit to make browsing and the Web better for you. > +INSTALL_BLURB3=You’re also joining a global community of users, contributors and developers working to make the best browser in the world. nit: INSTALL_BLURB3=You’re also joining a global community of users, contributors, and developers working to make the best browser in the world. (extra comma before and is preferred IMHO) @@ +50,5 @@ > +DOWNLOADING_DONE=Downloaded > +INSTALLING_TO_BE_DONE=Installing > +INSTALLING_IN_PROGRESS=Installing… > + > +SELECT_FOLDER=Select the folder to install Firefox in. nit: SELECT_FOLDER=Select the folder to install Firefox in: (may go over them all to see which ones should have colons and which ones shouldn't. It looks inconsistent but it's hard to tell just from this)
Attachment #662088 - Flags: review?(netzen) → review+
They can be seen in these screenshots http://exchangecode.com/robert/work/screenshots.png cc'ing shorlander and Madhava for input on the string changes in comment #79
Comment on attachment 662081 [details] [diff] [review] 7. Common code Review of attachment 662081 [details] [diff] [review]: ----------------------------------------------------------------- r=me with changes ::: toolkit/mozapps/installer/windows/nsis/common.nsh @@ +6985,5 @@ > +!define /math PBM_SETRANGE32 ${WM_USER} + 6 > + > +!define SHACF_FILESYSTEM 1 > + > +!define MOZ_LOADTRANSPARENT ${LR_LOADFROMFILE}|${LR_LOADTRANSPARENT}|${LR_LOADMAP3DCOLORS} nit: You may want to document somewhere that since LR_LOADMAP3DCOLORS is used, the loaded bitmaps must be at most 8bpp. @@ +7016,5 @@ > + > + ${If} $0 <> 0 > + System::Call 'user32::GetClientRect(i R0, i r0)' > + System::Call '*$0(i, i, i .s, i .s)' > + System::Free $0 I think here and on many other calls you can just have System::Call '*(i, i, i .s, i .s)' and remove the System::Free. Please verify with one call though first if you do this change. @@ +7048,5 @@ > + Push $0 > + > + System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_STYLE}) i .r0' > + IntOp $0 $0 | ${_STYLE} > + IntOp $0 $0 - ${_STYLE} (neat trick ORing to ensure it's there then subtract.) @@ +7049,5 @@ > + > + System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_STYLE}) i .r0' > + IntOp $0 $0 | ${_STYLE} > + IntOp $0 $0 - ${_STYLE} > + System::Call 'user32::SetWindowLongW(i ${_HANDLE}, i ${GWL_STYLE}, i r0)' nit: user32::SetWindowLong @@ +7067,5 @@ > + > + System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_EXSTYLE}) i .r0' > + IntOp $0 $0 | ${_EXSTYLE} > + IntOp $0 $0 - ${_EXSTYLE} > + System::Call 'user32::SetWindowLongW(i ${_HANDLE}, i ${GWL_EXSTYLE}, i r0)' user32::SetWindowLong @@ +7093,5 @@ > +!define GetTextExtent "!insertmacro GetTextExtentCall" > +!define un.GetTextExtent "!insertmacro GetTextExtentCall" > + > +!macro GetTextExtent > +!macroend I didn't think these empty macros were needed for artificial function calls, but you mentioned they are. Would be nice to verify they are indeed needed, but if you don't have time I'll take your word for it. @@ +7098,5 @@ > + > +!macro un.GetTextExtent > +!macroend > + > +!macro GetTextExtent_ nit: Would be good to document here what you expect on the stack and what you'll be returning. I know we don't call it externally, but it's nice to have :) @@ +7110,5 @@ > + Push $6 > + Push $7 > + > + ; reuse the existing control instead of creating a new one > + GetDlgItem $2 $HWNDPARENT 1028 What control ID does 1028 associate to? Please either comment or do a define that is used here and below @@ +7113,5 @@ > + ; reuse the existing control instead of creating a new one > + GetDlgItem $2 $HWNDPARENT 1028 > + > + System::Call 'user32::GetDC(i r2) i .r3' > + System::Call 'gdi32::SelectObject(i r3, i r0)' This already exists in SysFunc.nsh, I'm ok with you leaving it as is but just pointing it out. Could be called with System::Call "${sysSelectObject} (R3, R0) @@ +7161,5 @@ > + > +!macro un.GetDlgItemWidthHeight > +!macroend > + > +!macro GetDlgItemWidthHeight_ nit: Document what you expect on the stack and what you're returning. @@ +7167,5 @@ > + Push $1 > + Push $2 > + > + System::Call '*(i, i, i, i) i .r2' > + System::Call 'user32::GetClientRect(i r0, i r2)' nit: Add a comment that GetClientRect always has top and left values of 0 so this works. @@ +7201,5 @@ > + > +!macro un.GetDlgItemEndPX > +!macroend > + > +!macro GetDlgItemEndPX_ nit: doc stack usage @@ +7206,5 @@ > + Exch $0 ; handle of control > + Push $1 > + Push $2 > + > + FindWindow $1 "#32770" "" $HWNDPARENT Comment that #32770 is the dialog class @@ +7283,5 @@ > + ${If} ${Errors} > + ; When there aren't newlines in the text calculate the size of the > + ; rectangle needed for the text with a minimum of three lines of text. > + ClearErrors > + System::Call 'user32::DrawTextW(i r4, t $\"$2$\", i r5, i r6, \ user32::DrawText right? (and the 2 below cases) @@ +7366,5 @@ > + !verbose pop > +!macroend > + > +!macro un.GetTextWidthHeight > + !ifndef un.GetTextWidthHeight Shouldn't this ifndef go above the !macro? or I think it can just be removed.
Attachment #662081 - Flags: review?(netzen) → feedback+
Comment on attachment 662096 [details] [diff] [review] 9. stub patch in progress FYI, to get the stub uploaded by 'make upload' you'll need to add it to http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#980
Depends on: 792265
> ::: toolkit/mozapps/installer/windows/nsis/common.nsh > @@ +6985,5 @@ > > +!define /math PBM_SETRANGE32 ${WM_USER} + 6 > > + > > +!define SHACF_FILESYSTEM 1 > > + > > +!define MOZ_LOADTRANSPARENT ${LR_LOADFROMFILE}|${LR_LOADTRANSPARENT}|${LR_LOADMAP3DCOLORS} > > nit: You may want to document somewhere that since LR_LOADMAP3DCOLORS is > used, the loaded bitmaps must be at most 8bpp. Will do > @@ +7016,5 @@ > > + > > + ${If} $0 <> 0 > > + System::Call 'user32::GetClientRect(i R0, i r0)' > > + System::Call '*$0(i, i, i .s, i .s)' > > + System::Free $0 > > I think here and on many other calls you can just have System::Call '*(i, i, > i .s, i .s)' and remove the System::Free. Please verify with one call though > first if you do this change. I'll check though I don't think so. Also, it is allocated earlier in the code. > @@ +7049,5 @@ > > + > > + System::Call 'user32::GetWindowLong(i ${_HANDLE}, i ${GWL_STYLE}) i .r0' > > + IntOp $0 $0 | ${_STYLE} > > + IntOp $0 $0 - ${_STYLE} > > + System::Call 'user32::SetWindowLongW(i ${_HANDLE}, i ${GWL_STYLE}, i r0)' > > nit: user32::SetWindowLong There are other cases where this file will only work on Unicode and I intended to just use wide calls so it is obvious. Would you have a problem with just fixing those cases where it isn't explicitly using wide calls? > @@ +7093,5 @@ > > +!define GetTextExtent "!insertmacro GetTextExtentCall" > > +!define un.GetTextExtent "!insertmacro GetTextExtentCall" > > + > > +!macro GetTextExtent > > +!macroend > > I didn't think these empty macros were needed for artificial function calls, > but you mentioned they are. Would be nice to verify they are indeed needed, > but if you don't have time I'll take your word for it. I'm 99% sure that I verified this several weeks ago and I'll check again. > @@ +7098,5 @@ > > + > > +!macro un.GetTextExtent > > +!macroend > > + > > +!macro GetTextExtent_ > > nit: Would be good to document here what you expect on the stack and what > you'll be returning. I know we don't call it externally, but it's nice to > have :) Will do for internal macro documentation inside the macro since it is internal. > @@ +7110,5 @@ > > + Push $6 > > + Push $7 > > + > > + ; reuse the existing control instead of creating a new one > > + GetDlgItem $2 $HWNDPARENT 1028 > > What control ID does 1028 associate to? Please either comment or do a define > that is used here and below An NSIS control we don't use... Will doc it. > @@ +7161,5 @@ > > + > > +!macro un.GetDlgItemWidthHeight > > +!macroend > > + > > +!macro GetDlgItemWidthHeight_ > > nit: Document what you expect on the stack and what you're returning. > > @@ +7167,5 @@ > > + Push $1 > > + Push $2 > > + > > + System::Call '*(i, i, i, i) i .r2' > > + System::Call 'user32::GetClientRect(i r0, i r2)' > > nit: Add a comment that GetClientRect always has top and left values of 0 so > this works. Will do > @@ +7206,5 @@ > > + Exch $0 ; handle of control > > + Push $1 > > + Push $2 > > + > > + FindWindow $1 "#32770" "" $HWNDPARENT > > Comment that #32770 is the dialog class Will do > @@ +7366,5 @@ > > + !verbose pop > > +!macroend > > + > > +!macro un.GetTextWidthHeight > > + !ifndef un.GetTextWidthHeight > > Shouldn't this ifndef go above the !macro? or I think it can just be removed. No, that is for checking if it is defined using !define (see other usage in this file). When checking if a macro is defined !ifmacrodef and ifmacrondef are used.
Attached patch 7. Common code — — Splinter Review
I documented what is on Exch on the stack as appropriate. I didn't document what the individual pushed / popped registers since they are used often used for multiple things and it can be determined by reading the code. I removed the empty macros... not sure why the usage I looked at in the NSIS include directory did this. I went with wide calls since some parts of common.nsh require unicode, already uses wide calls, and I'd prefer it to be explicit.
Attachment #662081 - Attachment is obsolete: true
Attachment #662468 - Flags: review?(netzen)
Attachment #661694 - Attachment is obsolete: true
(In reply to Robert Strong [:rstrong] (do not email) from comment #83) >... > > Shouldn't this ifndef go above the !macro? or I think it can just be removed. > No, that is for checking if it is defined using !define (see other usage in > this file). When checking if a macro is defined !ifmacrodef and ifmacrondef > are used. btw: aiui this allows the calling of a NSIS Function instead of inserting the same macro multiple times which increases code size.
Almost there... still need to do: # Write Access check # Disk space check # Download Error messagebox # test Elevation cancel continue for non-admin (should already work) # alternate non-admin install directory
Attachment #662096 - Attachment is obsolete: true
Comment on attachment 662468 [details] [diff] [review] 7. Common code Review of attachment 662468 [details] [diff] [review]: ----------------------------------------------------------------- > There are other cases where this file will only work on Unicode and I intended > to just use wide calls so it is obvious. Would you have a problem with just > fixing those cases where it isn't explicitly using wide calls? Yup I'm fine with that as long as they are all consistent, which they are in this new patch. Thanks! > re: other clarifications Cool, thanks for the info. I'm still learning some parts of NSIS :)
Attachment #662468 - Flags: review?(netzen) → review+
> re the documentation on the internal macros: I knew they were only internal but I just seen them analogous to private member functions in C++ which I still document even if I'm only calling them once on the actual function definition and not on the function call. NSIS is a bit worse though because you can't have parameters for functions. So to validate that the caller is calling correctly, you need to know the order of how to push, and hence have to read more code. The commenting you did is good, and this doesn't really matter, thanks!
Comment on attachment 662490 [details] [diff] [review] 10. stub patch in progress rev2 Review of attachment 662490 [details] [diff] [review]: ----------------------------------------------------------------- I know this is still in progress, just wanted to add a couple quick notes from a quick skim: - For the configure stuff, someone else needs to review that, maybe khuey. Please split that out and ask for review sooner than later in case it takes longer to review. - win32 calls as widechar variants - Maybe add a "# Required Plugins:" block to the top of the file to be consistent with our other nsis files - For the min OS version stuff, instead of duplicating that code, could we move the common.nsh same code into a macro and then insert it at both places?
Replaced ’ from the strings provided with '... carrying forward r+
Attachment #662088 - Attachment is obsolete: true
Attachment #662678 - Flags: review+
Attached patch 9. configure changes (obsolete) — — Splinter Review
Kyle, I need a new configure option for the stub installer... then it can be ifdef'd so app's that don't want it don't build it when running the Windows make installer.
Attachment #662831 - Flags: review?(khuey)
Attachment #662490 - Attachment description: 9. stub patch in progress rev2 → 10. stub patch in progress rev2
Fixed the tabbing order for the buttons and checkbox. Carrying forward r+
Attachment #662967 - Flags: review+
Comment on attachment 662831 [details] [diff] [review] 9. configure changes FWIW, for options that normal developers don't need to fiddle with we generally prefer to skip the configure option and just require an env var to be set in confvars.sh and then AC_DEFINE/AC_SUBST it. This reduces the number of potential things for people to find in `configure --help` and fiddle.
That works for me
Minimal configure changes per comment #93 Ted, could you review this?
Attachment #662831 - Attachment is obsolete: true
Attachment #662831 - Flags: review?(khuey)
Attachment #663308 - Flags: review?(ted.mielczarek)
Attachment #663308 - Flags: review?(ted.mielczarek) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #62) > Comment on attachment 661695 [details] > 1. CertCheck dll for checkin > > r+ assuming this was built with VC6 Found an old system of mine with VC6 on it. Yay!
I had to fix a couple of bugs in the InetBgDL NSIS plugin code. There is a bunch of functionality I plan on adding to this in the future but that will have to wait until the future. :(
Attachment #663904 - Flags: review?(netzen)
btw: I fixed style here and there but not consistently throughout the files.
Resultant dll only to be checked in after attachment #663904 [details] [diff] [review] is r+
Attachment #663905 - Flags: review?(netzen)
Attachment #661701 - Attachment is obsolete: true
Attachment #661700 - Attachment is obsolete: true
With changes
Attachment #662018 - Attachment is obsolete: true
Attachment #663911 - Flags: review+
Comment on attachment 663905 [details] [diff] [review] 2. InetBgDL NSIS plug-in dll for checkin /me 's binary review skills are a little rusty but looks good :)
Attachment #663905 - Flags: review?(netzen) → review+
As a followup it'd be nice to figure out an easy way to build these plugins without resorting to VC6.
Comment on attachment 663904 [details] [diff] [review] 2.extra InetBgDL NSIS plug-in code Review of attachment 663904 [details] [diff] [review]: ----------------------------------------------------------------- I know most of this is not your code, but several things still seem wrong or needs tweaking. Everything is pretty small though. ::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp @@ +34,5 @@ > + g_N_CCH = N_CCH; \ > + g_N_Vars = N_Vars; \ > + } while(0) > + > +#define Stats_SetWin32Err(ec) g_cbCurrXF=(ec) This doesn't make sense to me, we don't call it often but why would we be setting the current transferred to the last error? I think this should be either removed or set to: #define Stats_SetWin32Err(ec) SetLastError(ec) It looks to me like this was supposed to both SetLastError AND store a different global value that would then get reported inside the stats call to registry, but it seems like it was never implemented. @@ +79,5 @@ > +#ifndef ONELOCKTORULETHEMALL > + StatsLock_AcquireExclusive(); > +#endif > + g_FilesTotal = 0; // This causes the Task thread to exit the transfer loop > + if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000)) Is 10 seconds long enough here? It's not clear to me if this is only called on error (NSPIM_UNLOAD and /RESET) @@ +83,5 @@ > + if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000)) > + { > + TerminateThread(g_hThread, ERROR_OPERATION_ABORTED); > + } > + CloseHandle(g_hThread); if (g_hTHread) { CloseHandle(g_hThread); } @@ +110,5 @@ > + Reset(); > + /* > + ** Should do cleanup here but the process is about to die so we don't care > + ** > + CloseHandle(g_hThread); This can be removed since it's already closed in Reset(); @@ +125,5 @@ > + HINTERNET hInetSes = NULL; > + HANDLE hLocalFile; > + bool completedFile = false; > +startnexttask: > + hLocalFile = NULL; hLocalFile = INVALID_HANDLE_VALUE; @@ +172,5 @@ > + g_Status = STATUS_ERR_GETLASTERROR; > + // if (pFile) DeleteFile(pFile->text); > + StatsLock_ReleaseExclusive(); > + } > + InternetCloseHandle(hInetSes); For the case directly after InternetOpen... if (hInetes) { InternetCloseHandle(hInetSes); } @@ +173,5 @@ > + // if (pFile) DeleteFile(pFile->text); > + StatsLock_ReleaseExclusive(); > + } > + InternetCloseHandle(hInetSes); > + CloseHandle(hLocalFile); For the case after the CreateFile call directly... if (INVALID_HANDLE_VALUE != hLocalFile) { CloseHandle(hLocalFile); } @@ +181,5 @@ > + } > + > + if (!hInetSes) > + { > + hInetSes = InternetOpen(USERAGENT,INTERNET_OPEN_TYPE_PRECONFIG,NULL,NULL,0); I like that we're setting the user agent like this so we can track what's coming from the stub installer. But shouldn't we also be setting the operating system and operating system version in the user agent string? I know we can track the downloads of the actual stub, but it may be interesting to see what the difference is between stub downloading from a browser, and actual installer downloading from the stub. Maybe you'd see for example that on some version of some operating system that it leads to incomplete downloads. @@ +232,5 @@ > + const UINT cbBufXF = 1024 * sizeof(TCHAR); > + BYTE*bufXF = (BYTE*) pURL->text; > +#endif > + DWORD cbio,cbXF; > + BOOL retXF = InternetReadFile(hInetFile, bufXF, cbBufXF, &cbio); Just calling this out, InternetReadFile can return insufficient buffer if the buffer is not large enough to contain 1 line of the returned stream. If we expect more than a 1024 character line, maybe the buffer above should be increased. @@ +251,5 @@ > + { > + // Reading without StatsLock is ok in this thread... > + if (g_cbCurrXF != cbThisFile) > + { > + ec = ERROR_BROKEN_PIPE; This doesn't really make sense to me. If InternetReadFile returns TRUE, which it does in this case, and 0 is returned, then the download is complete. Regardless of wethr or not the file size is known that we are downloading. @@ +277,5 @@ > + } > + > + StatsLock_AcquireExclusive(); > + if (FILESIZE_UNKNOWN != cbThisFile) { > + g_cbCurrTot = cbThisFile; This condition seems like it doesn't matter, since if cbThisFile is set to FILESIZE_UNKNOWN so is g_cbCurrTot, but I'm fine with keeping it. @@ +287,5 @@ > + > + TRACE2(_T("TaskThreadProc completed %s, ec=%u\n"),pURL->text,ec); > + InternetCloseHandle(hInetFile); > + if (ERROR_SUCCESS == ec) { > + CloseHandle(hLocalFile); nit: I don't think this is needed but would be nice just in case if (INVALID_HANDLE_VALUE != hLocalFile) { CloseHandle(hLocalFile); hLocalFile = INVALID_HANDLE_VALUE; } @@ +347,5 @@ > + if (!g_hThread) > + { > + DWORD tid;tid; > + g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0, > + sizeof(TCHAR) > 1 ? NULL : &tid); nit: I heard of a problem on win98 with passing NULL here, but let's just either have the tread id returned or not. Probably just always have the thread id returned. @@ +348,5 @@ > + { > + DWORD tid;tid; > + g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0, > + sizeof(TCHAR) > 1 ? NULL : &tid); > + SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST); if (!g_hThread) { goto freeurlandexit; } SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST); What is the motivation for setting the thread priority as lowest here? I think I'd prefer no such call.
Attachment #663904 - Flags: review?(netzen) → review-
(In reply to Ted Mielczarek [:ted] from comment #103) > As a followup it'd be nice to figure out an easy way to build these plugins > without resorting to VC6. Do you mean as part of the build process? You can build with vs2010 but the only down side is that the file size will be larger.
Ah, ok, is that the sole reason you did that? It'd be nice to document it either way.
(In reply to Ted Mielczarek [:ted] from comment #106) > Ah, ok, is that the sole reason you did that? It'd be nice to document it > either way. Yup, yup.
(In reply to Brian R. Bondy [:bbondy] from comment #104) >... > ::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp > @@ +34,5 @@ > > + g_N_CCH = N_CCH; \ > > + g_N_Vars = N_Vars; \ > > + } while(0) > > + > > +#define Stats_SetWin32Err(ec) g_cbCurrXF=(ec) > > This doesn't make sense to me, we don't call it often but why would we be > setting the current transferred to the last error? > I think this should be either removed or set to: > #define Stats_SetWin32Err(ec) SetLastError(ec) > It looks to me like this was supposed to both SetLastError AND store a > different global value that would then get reported inside the stats call to > registry, but it seems like it was never implemented. Done > @@ +79,5 @@ > > +#ifndef ONELOCKTORULETHEMALL > > + StatsLock_AcquireExclusive(); > > +#endif > > + g_FilesTotal = 0; // This causes the Task thread to exit the transfer loop > > + if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000)) > > Is 10 seconds long enough here? It's not clear to me if this is only called > on error (NSPIM_UNLOAD and /RESET) It returns almost immediately for me now though I found a case where the lock was preventing it which is now fixed. > @@ +83,5 @@ > > + if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000)) > > + { > > + TerminateThread(g_hThread, ERROR_OPERATION_ABORTED); > > + } > > + CloseHandle(g_hThread); > > if (g_hTHread) { > CloseHandle(g_hThread); > } Done > @@ +110,5 @@ > > + Reset(); > > + /* > > + ** Should do cleanup here but the process is about to die so we don't care > > + ** > > + CloseHandle(g_hThread); > > This can be removed since it's already closed in Reset(); Removed the commented out code > @@ +125,5 @@ > > + HINTERNET hInetSes = NULL; > > + HANDLE hLocalFile; > > + bool completedFile = false; > > +startnexttask: > > + hLocalFile = NULL; > > hLocalFile = INVALID_HANDLE_VALUE; Done > @@ +172,5 @@ > > + g_Status = STATUS_ERR_GETLASTERROR; > > + // if (pFile) DeleteFile(pFile->text); > > + StatsLock_ReleaseExclusive(); > > + } > > + InternetCloseHandle(hInetSes); > > For the case directly after InternetOpen... > if (hInetes) { > InternetCloseHandle(hInetSes); > } Done > @@ +173,5 @@ > > + // if (pFile) DeleteFile(pFile->text); > > + StatsLock_ReleaseExclusive(); > > + } > > + InternetCloseHandle(hInetSes); > > + CloseHandle(hLocalFile); > > For the case after the CreateFile call directly... > if (INVALID_HANDLE_VALUE != hLocalFile) { > CloseHandle(hLocalFile); > } Done > @@ +181,5 @@ > > + } > > + > > + if (!hInetSes) > > + { > > + hInetSes = InternetOpen(USERAGENT,INTERNET_OPEN_TYPE_PRECONFIG,NULL,NULL,0); > > I like that we're setting the user agent like this so we can track what's > coming from the stub installer. > But shouldn't we also be setting the operating system and operating system > version in the user agent string? > I know we can track the downloads of the actual stub, but it may be > interesting to see what the difference is between stub downloading from a > browser, and actual installer downloading from the stub. Maybe you'd see for > example that on some version of some operating system that it leads to > incomplete downloads. We will be sending a ping with this info from stub.nsi so I don't really see the need for that here. > @@ +232,5 @@ > > + const UINT cbBufXF = 1024 * sizeof(TCHAR); > > + BYTE*bufXF = (BYTE*) pURL->text; > > +#endif > > + DWORD cbio,cbXF; > > + BOOL retXF = InternetReadFile(hInetFile, bufXF, cbBufXF, &cbio); > > Just calling this out, InternetReadFile can return insufficient buffer if > the buffer is not large enough to contain 1 line of the returned stream. If > we expect more than a 1024 character line, maybe the buffer above should be > increased. Increased to 4096 > @@ +251,5 @@ > > + { > > + // Reading without StatsLock is ok in this thread... > > + if (g_cbCurrXF != cbThisFile) > > + { > > + ec = ERROR_BROKEN_PIPE; > > This doesn't really make sense to me. If InternetReadFile returns TRUE, > which it does in this case, and 0 is returned, then the download is > complete. Regardless of wethr or not the file size is known that we are > downloading. I'm leaving this here for now since we our servers provide file size and I'd like to think about this before removing it. > @@ +277,5 @@ > > + } > > + > > + StatsLock_AcquireExclusive(); > > + if (FILESIZE_UNKNOWN != cbThisFile) { > > + g_cbCurrTot = cbThisFile; > > This condition seems like it doesn't matter, since if cbThisFile is set to > FILESIZE_UNKNOWN so is g_cbCurrTot, but I'm fine with keeping it. Kept > @@ +287,5 @@ > > + > > + TRACE2(_T("TaskThreadProc completed %s, ec=%u\n"),pURL->text,ec); > > + InternetCloseHandle(hInetFile); > > + if (ERROR_SUCCESS == ec) { > > + CloseHandle(hLocalFile); > > nit: I don't think this is needed but would be nice just in case > if (INVALID_HANDLE_VALUE != hLocalFile) { > CloseHandle(hLocalFile); > hLocalFile = INVALID_HANDLE_VALUE; > } Done > @@ +347,5 @@ > > + if (!g_hThread) > > + { > > + DWORD tid;tid; > > + g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0, > > + sizeof(TCHAR) > 1 ? NULL : &tid); > > nit: I heard of a problem on win98 with passing NULL here, but let's just > either have the tread id returned or not. Probably just always have the > thread id returned. Done > > @@ +348,5 @@ > > + { > > + DWORD tid;tid; > > + g_hThread = CreateThread(NULL, 0, TaskThreadProc, NULL, 0, > > + sizeof(TCHAR) > 1 ? NULL : &tid); > > + SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST); > > if (!g_hThread) { > goto freeurlandexit; > } > > SetThreadPriority(g_hThread, THREAD_PRIORITY_LOWEST); > What is the motivation for setting the thread priority as lowest here? I > think I'd prefer no such call. Done
Attachment #663905 - Attachment is obsolete: true
Attachment #664411 - Flags: review?(netzen)
Attachment #663904 - Attachment is obsolete: true
Comment on attachment 664411 [details] [diff] [review] 2.extra InetBgDL NSIS plug-in code rev2 Review of attachment 664411 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the nits. Thanks! ::: other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp @@ +243,5 @@ > + > + TRACE2(_T("InternetReadFile true with 0 cbio, cbThisFile=%d gle=%u\n"), cbThisFile, GetLastError()); > + if (FILESIZE_UNKNOWN != cbThisFile) > + { > + // Reading without StatsLock is ok in this thread... nit: This comment is confusing here, maybe remove it? @@ +244,5 @@ > + TRACE2(_T("InternetReadFile true with 0 cbio, cbThisFile=%d gle=%u\n"), cbThisFile, GetLastError()); > + if (FILESIZE_UNKNOWN != cbThisFile) > + { > + // Reading without StatsLock is ok in this thread... > + if (g_cbCurrXF != cbThisFile) nit 2: Please combine these 2 conditions if (FILESIZE_UNKNOWN != cbThisFile && g_cbCurrXF != cbThisFile) { } @@ +246,5 @@ > + { > + // Reading without StatsLock is ok in this thread... > + if (g_cbCurrXF != cbThisFile) > + { > + ec = ERROR_BROKEN_PIPE; OK I understand this condition now. Please add a comment above the new combined condition above something like: // If we haven't transferred all of the file, and we know how big the file is, and we // have no more data to read from the HTTP request, then set a broken pipe error.
Attachment #664411 - Flags: review?(netzen) → review+
Actually you can keep that comment but combine it with the other suggested ones pls.
> We will be sending a ping with this info from stub.nsi so I > don't really see the need for that here. OK cool I wasn't sure how it was being tracked, that sounds good.
(In reply to Brian R. Bondy [:bbondy] from comment #79) > > +INTRO_BLURB=Thanks for choosing $BrandFullName, the browser that chooses you above everything else. Click to install. > > +INSTALL_BLURB1=You're about to enjoy the very latest in speed, flexibility and security so you're always in control. > > +INSTALL_BLURB2=That’s because $BrandShortName is made by a non-profit to make browsing and the Web better for you. > > +INSTALL_BLURB3=You’re also joining a global community of users, contributors and developers working to make the best browser in the world. > > nit: INSTALL_BLURB3=You’re also joining a global community of users, > contributors, and developers working to make the best browser in the world. > (extra comma before and is preferred IMHO) I, personally, prefer the oxford comma, too. But, from our writing styleguide http://www.mozilla.org/en-US/styleguide/communications/copy-rules/ "we do not use serial commas (also called Oxford commas) in Mozilla communications"
Addressed review comments. I'll recompile the dll with VC6 and attach the patch for it soon.
Attachment #664411 - Attachment is obsolete: true
Attachment #664590 - Flags: review+
Attached patch 11. main patch rev1 (obsolete) — — Splinter Review
Brian, I still need to finish the 1. messagebox to download the full installer on failure 2. set as default (will probably just launch the helper to do this) 3. taskbar / quicklaunch shortcut creation (started but commented out) I'll finish these up in a new patch. The scariest and biggest PITA part of this code is the install directory checks so please pay close attention to it when reviewing. I don't want to use the common functions for this since they are buggy and we can't use the builtin capabilities since that only applies to the instfiles page. I didn't bother adding the urls for the plugins since we might as well bring them all into the tree.
Attachment #665375 - Flags: review?(netzen)
Attachment #665375 - Attachment description: patch rev1 → 11. main patch rev1
btw: I'll submit a followup patch for the additional items and possibly to cover review comments as well in the hope that it makes it easier to review.
Comment on attachment 665375 [details] [diff] [review] 11. main patch rev1 Review of attachment 665375 [details] [diff] [review]: ----------------------------------------------------------------- - There's some win8 CheckboxShortcutInStartMenu handling already on m-c that needs to be brought into this. This will go live while win8 is released so I think it should be added now. - Ditto CheckboxSetAsDefault - You mentioned you were going to launch the helper.exe for defaults handling. This will probably need a secreview since I think it'll be done after being elevated. Whereas when we call it from firefox.exe we're a medium integrity process, so that would be more OK for dll injection. Alternatively call the common code directly to reduce a security risk. - You mentioned these are pending: messagebox, taskbar/quicklaunch - If possible I'd appreciate a zip of your patch queue and series file so I can try this out on my machine, all of my reviews have been code only and without me actually seeing it thus far. I find that actually trying the code makes me think of more places to probe the code on. I would like another pass at this after the changes before r+ing. ::: browser/installer/windows/nsis/stub.nsi @@ +109,5 @@ > +ChangeUI all "nsisui.exe" > +!ifdef HAVE_64BIT_OS > + InstallDir "$PROGRAMFILES64\${BrandFullName}\" > +!else > + InstallDir "$PROGRAMFILES32\${BrandFullName}\" Have you considered using InstallDirRegKey instead of all the custom handling with InitialInstallDir? I haven't tested to see if it works as expected with the back button but it would be simpler. You already have the code in place so I'm not suggesting changing it now, just wondering what the motivation was for the change. @@ +140,5 @@ > + > +Page custom createDummy ; Needed to enable the Intro page's back button > +Page custom createIntro leaveIntro > +Page custom createOptions leaveOptions > +Page custom createInstall leaveInstall nit: Would be nice to have a quick comment description of what each page is. @@ +153,5 @@ > + ${Unless} ${RunningX64} > + ${OrUnless} ${AtLeastWinVista} > + MessageBox MB_OK|MB_ICONSTOP "$(WARN_MIN_SUPPORTED_OS_MSG)" IDOK > + ; Nothing initialized so no need to call OnEndCommon > + Quit nit: Would be nice to have a comment just after !ifdef HAVE_64BIT_OS that says: ; Restrict x64 builds from being installed on x86 and pre Vista @@ +252,5 @@ > +Function createDummy > +FunctionEnd > + > +Function createIntro > + ; If Back is clicked on the options page reset variables Is it better to reset variables here or preserve them from what the user selected. I think if I pressed back I'd be doing it because I wanted to make a slight change of something I did in the last screen. I'm sure you already thought about this in detail though, so I'm fine with it as is. @@ +518,5 @@ > + ${GetLongPath} "$0" $0 > + ${If} "$INSTDIR" == "$0" > + LockWindow off > + MessageBox MB_OK|MB_ICONEXCLAMATION "$(WARN_ROOT_INSTALL)" > + Abort nit: Comment before the Abort call ; Stay on the page @@ +525,5 @@ > + Call CanWrite > + ${If} "$CanWriteToInstallDir" == "false" > + LockWindow off > + MessageBox MB_OK|MB_ICONEXCLAMATION "$(WARN_WRITE_ACCESS)" > + Abort nit: ;Stay on the page @@ +532,5 @@ > + Call CheckSpace > + ${If} "$HasRequiredSpaceAvailable" == "false" > + LockWindow off > + MessageBox MB_OK|MB_ICONEXCLAMATION "$(WARN_DISK_SPACE)" > + Abort nit: ;Stay on the page @@ +701,5 @@ > +FunctionEnd > + > +Function StartDownload > + ${NSD_KillTimer} StartDownload > + InetBgDL::Get "${URLStubDownload}" "$PLUGINSDIR\download.exe" /END Please check with security if you think it's needed on the WinINet flag INTERNET_FLAG_IGNORE_REDIRECT_TO_HTTP that this uses. It allows https to http redirects silently. I don't think it matters though since we're just downloading an installer, but maybe there's a privacy issue there (although I personally think it's fine). Also I think this call will silently handle HTTP redirects because INTERNET_FLAG_NO_AUTO_REDIRECT i not set. And I think that it will automatically handle things like chunked encoding transparently. Please let me know if you think that's not the case. @@ +716,5 @@ > + # $1 = Completed files > + # $2 = Remaining files > + # $3 = Number of downloaded bytes for the current file > + # $4 = Size of current file (Empty string if the size is unknown) > + ${If} $0 > 299 Please add a comment here explaining that 300s and 100s error codes should be handled automatically by the WinINet calls inside InetBgDL. @@ +775,5 @@ > + > + CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe" > + Pop $0 > + CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \ > + "${CertNameDownload}" "${CertIssuerDownload}" Please have security sign off on this, I know there was concern for the maintenanceservice where I had to refuse write access to the file I'd be executing from a high integrity process. In this case you'd open download.exe without write sharing before doing these 2 calls. And keep it open until after it is executed. This is to protect against someone overwritting the binary just after the check but before executing it. A low integrity process could do this in theory and escalate to a high integrity process. I think this is more unlikely for your case though since the installer location is not at a known location beforehand by an attacker. So personally I'd be fine with it. It even went a step further with the maintenanceservice where we had to worry about network drives, in which case we currently just skip using the maintenance service, but I don't think it has to go that far in my opinion for this. I'd be fine if this was posted to a diff bug and landed independently in some given time frame that security signs off on. @@ +830,5 @@ > + Rename "$INSTDIR\freebl3.dll" "$INSTDIR\${TO_BE_DELETED}\freebl3.dll" > + Rename "$INSTDIR\AccessibleMarshal.dll" "$INSTDIR\${TO_BE_DELETED}\AccessibleMarshal.dll" > + ${EndIf} > + > + Exec "$PLUGINSDIR\download.exe /INI=$PLUGINSDIR\${CONFIG_INI}" It's my understanding that this can have a DLL injection attack iff the main installer without stub has one. (Because they're identical) If that's not the case please request a security review for this. @@ +877,5 @@ > +Function OnBack > + StrCpy $WasOptionsButtonClicked "true" > + StrCpy $R9 1 ; Goto the next page > + Call RelativeGotoPage > + Abort Can you comment a bit more on what this does? I don't fully understand what happens with this Abort call. @@ +1033,5 @@ > + ${If} ${FileExists} "$INSTDIR" > + ; Use the existing directory when it exists > + StrCpy $0 "$INSTDIR" > + ${Else} > + ; Get the topmost directory that exists for new installs What is $0 set to in this case, is it always set? @@ +1035,5 @@ > + StrCpy $0 "$INSTDIR" > + ${Else} > + ; Get the topmost directory that exists for new installs > + ${DoUntil} ${FileExists} "$0" > + ${GetParent} "$0" $0 What if this is an invalid path that has no slasshes, maybe a check for errors and a break? @@ +1041,5 @@ > + ${EndIf} > + > + GetTempFileName $2 "$0" > + Delete $2 > + CreateDirectory "$2" I think the inherited security attributes for a directory is the same as for a file, but I'm fine with doing a temp directory + a temp file. @@ +1052,5 @@ > + ${If} ${FileExists} "$3" > + Delete "$3" > + StrCpy $CanWriteToInstallDir "true" > + ${Else} > + StrCpy $CanWriteToInstallDir "false" This should already be set, I think you can remove this Else condition.
Attachment #665375 - Flags: review?(netzen)
Couple of quick replies to comment #118 > @@ +252,5 @@ > > +Function createDummy > > +FunctionEnd > > + > > +Function createIntro > > + ; If Back is clicked on the options page reset variables > > Is it better to reset variables here or preserve them from what the user > selected. > I think if I pressed back I'd be doing it because I wanted to make a slight > change of something I did in the last screen. > I'm sure you already thought about this in detail though, so I'm fine with > it as is. The difference in this case is that we don't have anything on the previous page that can be set that isn't already on that page. > @@ +1041,5 @@ > > + ${EndIf} > > + > > + GetTempFileName $2 "$0" > > + Delete $2 > > + CreateDirectory "$2" > > I think the inherited security attributes for a directory is the same as for > a file, but I'm fine with doing a temp directory + a temp file. Agreed. We had some bug reports with squirrelly permissions for the non-inherited case which is why both are done.
(In reply to Brian R. Bondy [:bbondy] from comment #118) > Comment on attachment 665375 [details] [diff] [review] >... > - There's some win8 CheckboxShortcutInStartMenu handling already on m-c that > needs to be brought into this. This will go live while win8 is released so > I think it should be added now. > - Ditto CheckboxSetAsDefault Agreed > - You mentioned you were going to launch the helper.exe for defaults > handling. This will probably need a secreview since I think it'll be done > after being elevated. Whereas when we call it from firefox.exe we're a > medium integrity process, so that would be more OK for dll injection. > Alternatively call the common code directly to reduce a security risk. It will be done from the non-elevated process.
> ::: browser/installer/windows/nsis/stub.nsi > @@ +109,5 @@ > > +ChangeUI all "nsisui.exe" > > +!ifdef HAVE_64BIT_OS > > + InstallDir "$PROGRAMFILES64\${BrandFullName}\" > > +!else > > + InstallDir "$PROGRAMFILES32\${BrandFullName}\" > > Have you considered using InstallDirRegKey instead of all the custom > handling with InitialInstallDir? I haven't tested to see if it works as > expected with the back button but it would be simpler. You already have the > code in place so I'm not suggesting changing it now, just wondering what the > motivation was for the change. It doesn't work with our install method. See code in NO_INSTDIR_FROM_REG blocks in common.nsh for how this was handled though it turns out that it was disabled by defining NO_INSTDIR_FROM_REG at some point on all branches in defines.nsi.in so I am not implementing that here until sometime later after I understand why.
Attached patch 11. main patch rev2 (obsolete) — — Splinter Review
Should cover everything but the Win7 taskbar shortcut when not setting as default and the following > - You mentioned you were going to launch the helper.exe for defaults > handling. This will probably need a secreview since I think it'll be done > after being elevated. Whereas when we call it from firefox.exe we're a > medium integrity process, so that would be more OK for dll injection. > Alternatively call the common code directly to reduce a security risk. > - You mentioned these are pending: messagebox, taskbar/quicklaunch > - If possible I'd appreciate a zip of your patch queue and series file so I > can try this out on my machine, all of my reviews have been code only and > without me actually seeing it thus far. I find that actually trying the > code makes me think of more places to probe the code on. > > @@ +701,5 @@ > > +FunctionEnd > > + > > +Function StartDownload > > + ${NSD_KillTimer} StartDownload > > + InetBgDL::Get "${URLStubDownload}" "$PLUGINSDIR\download.exe" /END > > Please check with security if you think it's needed on the WinINet flag > INTERNET_FLAG_IGNORE_REDIRECT_TO_HTTP that this uses. > It allows https to http redirects silently. I don't think it matters though > since we're just downloading an installer, but maybe there's a privacy issue > there (although I personally think it's fine). > > Also I think this call will silently handle HTTP redirects because > INTERNET_FLAG_NO_AUTO_REDIRECT i not set. And I think that it will > automatically handle things like chunked encoding transparently. Please let > me know if you think that's not the case. > > @@ +775,5 @@ > > + > > + CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe" > > + Pop $0 > > + CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \ > > + "${CertNameDownload}" "${CertIssuerDownload}" Needs to lock Comment that it uses the temp dir so network drives shouldn't be a concern. > @@ +830,5 @@ > > + Rename "$INSTDIR\freebl3.dll" "$INSTDIR\${TO_BE_DELETED}\freebl3.dll" > > + Rename "$INSTDIR\AccessibleMarshal.dll" "$INSTDIR\${TO_BE_DELETED}\AccessibleMarshal.dll" > > + ${EndIf} > > + > > + Exec "$PLUGINSDIR\download.exe /INI=$PLUGINSDIR\${CONFIG_INI}" > > It's my understanding that this can have a DLL injection attack iff the main > installer without stub has one. (Because they're identical) > If that's not the case please request a security review for this. >
Attachment #665375 - Attachment is obsolete: true
Attachment #665648 - Flags: review?(netzen)
Comment on attachment 665648 [details] [diff] [review] 11. main patch rev2 Review of attachment 665648 [details] [diff] [review]: ----------------------------------------------------------------- Looks good although I'll do another pass tomorrow after trying the code. > Should cover everything but the Win7 taskbar shortcut when not setting as default and the following I think we still need a message box to download the full on failures too. ::: browser/branding/official/branding.nsi @@ +14,5 @@ > !define URLUpdateInfo "http://www.mozilla.com/${AB_CD}/firefox/" > + > +# It isn't possible to get the size of the installation prior to downloading > +# so the stub installer uses an estimate. > +!define APPROXIMATE_REQUIRED_SPACE_MB "42.2" Do you expect this to be significantly different for each brand? Maybe this should be in defines.nsi.in instead? ::: browser/installer/windows/nsis/stub.nsi @@ +789,5 @@ > + > + CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe" > + Pop $0 > + CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \ > + "${CertNameDownload}" "${CertIssuerDownload}" I think you mentioned that you agree that download.exe needs to lock before these 2 calls right? I agree that network drives are not a concern. @@ +1111,5 @@ > + ${EndIf} > +FunctionEnd > + > +Function ExecSetAsDefaultAppUser > + Exec "$\"$INSTDIR\uninstall\helper.exe$\" /SetAsDefaultAppUser" I haven't tested this yet, but if we're executing this unelevated, won't helper.exe prompt with UAC? If not how else could it set the HKLM StartMenuInternetKeys. This would work fine on win8 only as far as I can tell. Maybe I'm overlooking something?
(In reply to Brian R. Bondy [:bbondy] from comment #118) >... > > Should cover everything but the Win7 taskbar shortcut when not setting as default and the following > I think we still need a message box to download the full on failures too. Yes... accidentally removed that from the what needs to be done list. > ::: browser/branding/official/branding.nsi > @@ +14,5 @@ > > !define URLUpdateInfo "http://www.mozilla.com/${AB_CD}/firefox/" > > + > > +# It isn't possible to get the size of the installation prior to downloading > > +# so the stub installer uses an estimate. > > +!define APPROXIMATE_REQUIRED_SPACE_MB "42.2" > > Do you expect this to be significantly different for each brand? Maybe this > should be in defines.nsi.in instead? I had it there originally... I'll move it back and if it is needed it can be split out then. > ::: browser/installer/windows/nsis/stub.nsi > @@ +789,5 @@ > > + > > + CertCheck::VerifyCertTrust "$PLUGINSDIR\download.exe" > > + Pop $0 > > + CertCheck::VerifyCertNameIssuer "$PLUGINSDIR\download.exe" \ > > + "${CertNameDownload}" "${CertIssuerDownload}" > > I think you mentioned that you agree that download.exe needs to lock before > these 2 calls right? > I agree that network drives are not a concern. Right though I haven't delved into that yet. I am also considering using the system's temp directory since I believe it is more secure than the user's. > @@ +1111,5 @@ > > + ${EndIf} > > +FunctionEnd > > + > > +Function ExecSetAsDefaultAppUser > > + Exec "$\"$INSTDIR\uninstall\helper.exe$\" /SetAsDefaultAppUser" > > I haven't tested this yet, but if we're executing this unelevated, won't > helper.exe prompt with UAC? If not how else could it set the HKLM > StartMenuInternetKeys. This would work fine on win8 only as far as I can > tell. Maybe I'm overlooking something? The required HKLM keys I believe are handled on install but I'll double check.
Attachment #665648 - Attachment is obsolete: true
Attachment #665648 - Flags: review?(netzen)
Attached patch 11. main patch rev3 (obsolete) — — Splinter Review
I think this covers everything but the Win7 taskbar shortcut when not setting as default and the UAC prompt when setting as default when installing as a non-admin. I'll see if I can fix these though I don't think it should stop this from landing.
Attachment #665833 - Flags: review?(netzen)
I've changed the following locally Removed !insertmacro InitHashAppModelId Modified System::Call 'kernel32::CreateFileW(t "$DownloadDir\download.exe", to System::Call 'kernel32::CreateFileW(t "$DownloadDir\download.exe", \
Includes fixes from comment #126
Attachment #665833 - Attachment is obsolete: true
Attachment #665833 - Flags: review?(netzen)
Attachment #665839 - Flags: review?(netzen)
> Right though I haven't delved into that yet. I am also considering using the > system's temp directory since I believe it is more secure than the user's. I tried to write to it via an unelevated cmd.exe but I still could. If you do go the route of not locking but instead writing into a new dir at a high integrity location, then that's cool but the dir will be different for the unelevated case.
Latest patch locks it as well.
Attached patch 12. Fix download dir (obsolete) — — Splinter Review
Found an issue with setting the download dir
Attachment #665954 - Flags: review?(netzen)
Comment on attachment 665954 [details] [diff] [review] 12. Fix download dir Is the win temp dir always called temp on all locales of Windows? I think NSIS exposes this value with $TEMP.
(In reply to Brian R. Bondy [:bbondy] from comment #131) > Comment on attachment 665954 [details] [diff] [review] > 12. Fix download dir > > Is the win temp dir always called temp on all locales of Windows? I think > NSIS exposes this value with $TEMP. I believe it is called temp by default (might be possible to change). $TEMP is the user's temp and I could get its name and use it. Then again, I am tempted to go with the user's temp since there is file locking now.
Just use file locking... talked with bbondy about this over irc
Attachment #665954 - Attachment is obsolete: true
Attachment #665954 - Flags: review?(netzen)
Attachment #665968 - Flags: review?(netzen)
For the checkbox in preferences, it says: Install the Nightly background update service. Since this is a global service, and to stay consistent with the base non stub installer, and since it wouldn't match existing support documentation, please make this say instead: Install Maintenance Service
I worked with UX on the text so they need to approve that change.
OK so if UX agrees for a change, then we have to update the text here. If there is not a change, we should also: - update the base installer text to match. - notify support so they can update their documentation http://support.mozilla.org/en-US/kb/what-mozilla-maintenance-service
Perhaps go with "Install the background update service." and change the documentation to include both for now until the Full installer is changed to the new UI.
That same checkbox should also only be shown in these conditions: Call IsUserAdmin Pop $R0 ${If} $R0 == "true" ; Only proceed if we have HKLM write access ${AndIf} $TmpVal == "HKLM" ; On Windows 2000 we do not install the maintenance service. ${AndIf} ${AtLeastWinXP}
(In reply to Brian R. Bondy [:bbondy] from comment #139) > That same checkbox should also only be shown in these conditions: > > Call IsUserAdmin > Pop $R0 > ${If} $R0 == "true" > ; Only proceed if we have HKLM write access > ${AndIf} $TmpVal == "HKLM" > ; On Windows 2000 we do not install the maintenance service. > ${AndIf} ${AtLeastWinXP} Will add the HKLM and IsUserAdmin requirements. Our minimum Windows version supported is now Windows XP SP2.
Attachment #665968 - Flags: review?(netzen) → review+
Comment on attachment 665839 [details] [diff] [review] 11. main patch rev4 Review of attachment 665839 [details] [diff] [review]: ----------------------------------------------------------------- r+ for this patch on condition that in follow up patches we will do: - As discussed on IRC, currently if you install on top it silently installs and launches firefox.exe. Instead we should give an error to close firefox like the base installer does. - As discussed on IRC, the install to dir should be dependent on the brand you're building. You also mentioned you'd update the brnading.nsi links when you do this, although I think they are independent tasks. - With a limited user account, I press no to elevate, it defaults to users app data dir which is good, but then I start to install and it prompts me for elevation again. It should instead not prompt for elevation and only set the HKCU keys. (see below suggestion) I think it would be good if not already done, to loop a QA person into this bug, and provide them with a build to go over any litmus tests that exist, as well as test other things they can think of. ::: browser/installer/windows/nsis/stub.nsi @@ +1173,5 @@ > + > +Function ExecSetAsDefaultAppUser > + ; Using the helper.exe lessens the stub installer size. > + ; This could ask for elevatation when the user doesn't install as admin. > + Exec "$\"$INSTDIR\uninstall\helper.exe$\" /SetAsDefaultAppUser" What about adding a switch to common.nsh here to call directly into the HKCU variant of this function that doesn't prompt for elevation?
Attachment #665839 - Flags: review?(netzen) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #142) > Comment on attachment 665839 [details] [diff] [review] > 11. main patch rev4 > > Review of attachment 665839 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ for this patch on condition that in follow up patches we will do: > - As discussed on IRC, currently if you install on top it silently installs > and launches firefox.exe. Instead we should give an error to close firefox > like the base installer does. Will do > - As discussed on IRC, the install to dir should be dependent on the brand > you're building. You also mentioned you'd update the brnading.nsi links > when you do this, although I think they are independent tasks. Done > - With a limited user account, I press no to elevate, it defaults to users > app data dir which is good, but then I start to install and it prompts me > for elevation again. It should instead not prompt for elevation and only > set the HKCU keys. (see below suggestion) This is difficult for a number of reasons and I'd prefer prompting instead so if someone isn't elevated and can we get the registry keys we want and if they don't elevate we just continue. I'll consider other options at a later date. > I think it would be good if not already done, to loop a QA person into this > bug, and provide them with a build to go over any litmus tests that exist, > as well as test other things they can think of. Jason Smith is looped in.
(In reply to Robert Strong [:rstrong] (do not email) from comment #143) > > - With a limited user account, I press no to elevate, it defaults to users > > app data dir which is good, but then I start to install and it prompts me > > for elevation again. It should instead not prompt for elevation and only > > set the HKCU keys. (see below suggestion) > This is difficult for a number of reasons and I'd prefer prompting instead > so if someone isn't elevated and can we get the registry keys we want and if > they don't elevate we just continue. I'll consider other options at a later > date. I'm fine with either of these 2 things: - Posting a new bug to make sure unelevated limited user account installs from the stub installer do not have the option to set as default. - Posting a new bug to make sure unelevated limited user account installs from the base installer do have the option for default users, and do prompt. I know parity with the old installer is not a requirement, and that's why I'm saying new bug. But I think small differences like this should be avoided when possible.
Attached patch Fixes 1 rev1 (obsolete) — — Splinter Review
Removes the checkbox to set as default when we don't have HKLM write access. Removes the checkbox for the maintenance service for various cases. Another patch on the way for the firefox already running case.
Attachment #666045 - Flags: review?(netzen)
Along with the other requested changes besides the firefox already running case.
Attached patch Fixes 2 rev1 — — Splinter Review
Adds the close app notification. Doesn't use the common.nsh macro because that aborts which has been problematic from the custom nsDialogs pages when I tested it.
Attachment #666052 - Flags: review?(netzen)
Comment on attachment 666045 [details] [diff] [review] Fixes 1 rev1 Review of attachment 666045 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that change ::: browser/installer/windows/nsis/stub.nsi @@ +511,5 @@ > + ClearErrors > + WriteRegStr HKLM "Software\Mozilla" "${BrandShortName}InstallerTest" \ > + "Write Test" > + ${If} ${Errors} > + ${AndIf} $0 != "true" ${OrIf} $0 != "true"
Attachment #666045 - Flags: review?(netzen)
Attached patch Fixes 1 rev2 — — Splinter Review
Attachment #666045 - Attachment is obsolete: true
Attachment #666059 - Flags: review+
Comment on attachment 666052 [details] [diff] [review] Fixes 2 rev1 Review of attachment 666052 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/windows/nsis/stub.nsi @@ +1217,5 @@ > + > + ; Find the installation directory when launching using GetFunctionAddress > + ; from an elevated installer since $INSTDIR will not be set in this installer > + ${StrFilter} "${FileMainEXE}" "+" "" "" $R9 > + ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" "" If the user unchecks set as default this will launch the wrong firefox right?
(In reply to Brian R. Bondy [:bbondy] from comment #150) > Comment on attachment 666052 [details] [diff] [review] > Fixes 2 rev1 > > Review of attachment 666052 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/installer/windows/nsis/stub.nsi > @@ +1217,5 @@ > > + > > + ; Find the installation directory when launching using GetFunctionAddress > > + ; from an elevated installer since $INSTDIR will not be set in this installer > > + ${StrFilter} "${FileMainEXE}" "+" "" "" $R9 > > + ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$R9\DefaultIcon" "" > > If the user unchecks set as default this will launch the wrong firefox right? No, it always sets these keys if the installer has write permissions and in this specific case we are elevated and hence have write permissions.
Attachment #666052 - Flags: review?(netzen) → review+
Robert: Can/should those two lines in makensis.mk also be ifdef_ed with MOZ_STUB_INSTALLER? "+ cp $(CONFIG_DIR)/stub.exe "$(DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME)-stub.exe" + chmod 0755 "$(DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME)-stub.exe"" At least the SeaMonkey build does not like that as it does not build the stub installer. I guess Thunderbird has the same problem.
Depends on: 795580
Definitely and sorry I missed that. I'd prefer to do it in bug 795580 and if you submit a patch I'll review it right now. I'll take care of it tomorrow otherwise.
No longer depends on: 597536
Questions - Is the build for the stub installer still going to show up under http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/ for nightly builds? Or as of right now, they won't until some other bug is fixed to start hosting stub installer builds? Just generally looking to find the build for the code that just landed so that I can start testing immediately.
Here's the log from last night's nightly: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-09-29-03-06-24-mozilla-central/mozilla-central-win32-nightly-bm12-build1-build61.txt.gz So I see the stub installer getting built, but not uploaded. Rob, does it need to get added to UPLOAD_EXTRA_FILES or similar?
Depends on: 795647
Depends on: 795654
Rob, I think you need something like this so that the stub installer is uploaded after the build: https://hg.mozilla.org/try/rev/ae6f597c4a09 https://tbpl.mozilla.org/?tree=Try&rev=ae6f597c4a09
Depends on: 795982
Filed bug 795982 for signing and upload
Depends on: 797032
Depends on: 797157
Depends on: 797183
Depends on: 797208
Depends on: 797989
Depends on: 797998
Depends on: 798005
Depends on: 798239
Blocks: 798255
No longer blocks: 798255
Depends on: 798255
Depends on: 798477
Depends on: 798486
Depends on: 798537
No longer depends on: 798537
Depends on: 798794
Depends on: 798909
Depends on: 798908
Depends on: 798903
Depends on: 798869
Depends on: 798866
Depends on: 798861
Depends on: 798847
Depends on: 798671
Depends on: 798618
No longer depends on: 798239
Depends on: 798944
Depends on: 799180
Depends on: 799202
Depends on: 799057
Depends on: 799147
Depends on: 799172
Depends on: 799215
Depends on: 799289
Depends on: 799611
Depends on: 799710
Depends on: 799718
Depends on: 799774
Depends on: 799814
Depends on: 799861
Depends on: 799902
Depends on: 800059
Depends on: 800112
I've finished off an initial test pass on this on the en-US build that looked at areas such as, but not limited to: - General installation and uninstall use cases, along with registry as a result of install across Win 7 64-bit, Win Vista 32-bit, Win 8, Win XP - Basic quick checks that nothing stops working with Firefox that's expected (startup, crash reporting, updating, shutdown, tab browsing, add-ons) across the Windows operating systems stated above - Nightly and Aurora stub installers - extensively more testing the nightly stub installer, with some sanity checks (e.g. install, uninstall, launch, quit) on the aurora stub installer - More extensive testing in areas such as installing on guest accounts, cutting network connections, custom install, antiviruses (e.g. Norton and Avast), pave-over install use cases, UAC settings - Exploratory testing in areas such installing during a locked screen & hibernation, multiple stubs running at the same time, pulling down and installing the stub across different browsers, and other areas not noted that were developed on the fly - Crowd-source testing with support by Alex by advertising heavily to Mozillians, blogs, twitter, facebook, etc to get outside contributors to try out and test the stub installer to find more bugs in the wild Bugs have been linked to this bug for anything that has been found. Many of the bugs are being triaged with [stub+] for blocking the english deployment of the stub, [stub=] as a want to fix this before we ship, but not blocking, and [stub-] for a non-blocker. A summary report of the current state can be found here - http://wiki.mozilla.org/Stub_Installer/Triage. Any followup work or testing will be tracked on separate bugs. Closing this as verified for the initial test pass being done for the english build.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 794109
Depends on: 774618
Depends on: 801226
Depends on: 801235
No longer depends on: 801235
Depends on: 801841
Depends on: 802295
Depends on: 803274
Depends on: 802734
Depends on: 805575
Depends on: 808720
Depends on: 808118
Depends on: 810509
Depends on: 811646
Depends on: 811025
Depends on: 815938
Depends on: 836044
Depends on: 828455
Alias: StubInstaller
Separating the 32 and 64 bit stub installer implementations since the 32 bit is for alll practical purposes complete while the 64 bit has issues with 64 bit Firefox in general that will hold it up.
No longer depends on: 797032, 797208
Summary: Firefox net / stub installer → Firefox net / stub installer for x86
No longer blocks: 358384
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: