Closed Bug 903135 Opened 6 years ago Closed 4 years ago

Link updater to NSS and enable MAR verification on Linux and OSX

Categories

(Toolkit :: Application Update, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(3 files, 7 obsolete files)

29.38 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
3.91 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
5.21 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
No description provided.
This bug is about turning on verification on OSX and Linux (or just everywhere) and using NSS to do the verification.  We should still favor crypto api for verification on Windows.
See Also: → 903126
See Also: → 902761
I don't have any qualms about the security of doing this given existing tests to make sure it actually works.
Flags: sec-review+
Depends on: 903126
Attachment #789635 - Flags: review?(robert.bugzilla)
Attachment #789636 - Flags: review?(robert.bugzilla)
Attachment #789635 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 789636 [details] [diff] [review]
Multi platform MAR verification updater code. rev1

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>...
>@@ -2311,16 +2316,30 @@ int NS_main(int argc, NS_tchar **argv)
>     // in the parent process, so we do it here.
>     unsetenv("LD_PRELOAD");
>     execv(argv[0], argv);
>     __android_log_print(ANDROID_LOG_INFO, "updater",
>                         "execve failed: errno: %d. Exiting...", errno);
>     _exit(1);
>   }
> #endif
>+
>+#if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN)
>+  // On Windows we rely on CyrptoAPI to do verifications so we don't need to
>+  // initialize NSS at all there.
>+  // Otherwise, minimize the amount of NSS we depend on by avoiding all the NSS
>+  // databases.
>+  if (NSS_NoDB_Init(NULL) != SECSuccess) {
>+   PRErrorCode error = PR_GetError();
>+   fprintf(stderr, "Could not initialize NSS: %s (%d)",
>+           PR_ErrorToName(error), (int) error);
>+    _exit(1);
I suspect it will be a pain but could you change the logging so this error ends up in the log? If so, please submit a new patch for it. Thanks

>+  }
>+#endif
>+
>   InitProgressUI(&argc, &argv);
> 
>   // To process an update the updater command line must at a minimum have the
>   // directory path containing the updater.mar file to process as the first argument
>   // and the directory to apply the update to as the second argument. When the
>   // updater is launched by another process the PID of the parent process should be
>   // provided in the optional third argument and the updater will wait on the parent
>   // process to exit if the value is non-zero and the process is present. This is
Attachment #789636 - Flags: review?(robert.bugzilla) → review+
Blocks: 973933
Attached patch bug903135_missingdylibfix.diff (obsolete) — Splinter Review
I'll send it through a build config peer after your review.
This fixes the OSX failures on Oak which were caused because nss's dylib file couldn't be found.
Attachment #8380942 - Flags: review?(robert.strong.bugs)
Attachment #8380942 - Flags: review?(robert.strong.bugs) → review+
Attachment #8380942 - Flags: superreview?(ted)
Comment on attachment 8380942 [details] [diff] [review]
bug903135_missingdylibfix.diff

I'm going to look into hard linking these or something because it produces a package that is much larger.
Attachment #8380942 - Flags: superreview?(ted)
Comment on attachment 8380942 [details] [diff] [review]
bug903135_missingdylibfix.diff

Obsoleting this since we are using native APIs now and we don't want this dylib files included
Attachment #8380942 - Attachment is obsolete: true
Updated for current trunk. Carrying over r+.
Attachment #789608 - Attachment is obsolete: true
Attachment #8416010 - Flags: review+
Updated for current trunk. Carrying over r+.
Attachment #789635 - Attachment is obsolete: true
Attachment #8416011 - Flags: review+
Updated for current trunk. Carrying over r+.
Attachment #789636 - Attachment is obsolete: true
Attachment #8416012 - Flags: review+
Rebased
Attachment #8416010 - Attachment is obsolete: true
Attachment #8509986 - Flags: review+
Rebased
Attachment #8416011 - Attachment is obsolete: true
Attachment #8509987 - Flags: review+
Rebased
Attachment #8416012 - Attachment is obsolete: true
Attachment #8509988 - Flags: review+
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=883e17fc475f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.