Closed Bug 797477 Opened 9 years ago Closed 6 years ago

Enable loading certificates and MAR verification in updater code for B2G

Categories

(Firefox OS Graveyard :: General, defect, P1)

x86
macOS
defect

Tracking

(blocking-b2g:-, blocking-basecamp:-, b2g18 affected, b2g18-v1.0.0 affected, b2g18-v1.0.1 affected)

RESOLVED INVALID
blocking-b2g -
blocking-basecamp -
Tracking Status
b2g18 --- affected
b2g18-v1.0.0 --- affected
b2g18-v1.0.1 --- affected

People

(Reporter: bbondy, Unassigned)

References

Details

(Keywords: feature, Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-19][madrid])

Attachments

(8 files, 10 obsolete files)

3.01 KB, patch
Details | Diff | Splinter Review
2.50 KB, patch
Details | Diff | Splinter Review
1.45 KB, text/plain
Details
1.34 KB, text/plain
Details
6.33 KB, patch
bbondy
: feedback+
Details | Diff | Splinter Review
2.50 KB, patch
jgriffin
: review-
Details | Diff | Splinter Review
14.70 KB, patch
Details | Diff | Splinter Review
4.46 KB, patch
bbondy
: review-
Details | Diff | Splinter Review
- Add code to deploy the certificates for new device setups
- Add code to load those certificates inside updater code
- Add code to perform verifications when on B2G inside updater code
- Do build related config
Depends on: 793709
bsmith can you provide the bug number for the bug you're working on to get the test certs on the B2G NSS readonly config db?  I'd like to add it to the depends on list of this bug.
Attached patch Patch v0.9 - libmar changes (obsolete) — Splinter Review
Dependencies need to be implemented first, but here is most of the code needed for this bug.
Attached patch Patch v0.9 - build config (obsolete) — Splinter Review
Summary: Enable loading certificates and MAR verification code for those certificates in updater code for B2G → Enable loading certificates and MAR verification in updater code for B2G
Whiteboard: [WebAPI:P0] → [WebAPI:P0][Blocked by bug 793709 which is a P3]
We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.
Target Milestone: --- → B2G C1 (to 19nov)
What is the Exec Review?

Please note that this bug has been on hold since Oct 11th due to it's dependency mentioned in the whiteboard flag which was a P3. catlee did start working on this dependency today though so it shouldn't be long.

bsmith also has a dependency which must be completed first, but I'm not sure if there's a bug for that yet. That is to load the certs into the read only NSS db of the devices.

Also I haven't heard any clarification on updater related work with B2G. In particular I *think* we are only doing FOTA updates but I'm not sure.  And I *think* those updates will be packaged inside MARs as they are today, but I'm not sure if everyone is on board with that.  If people are not on board with that then this shouldn't even be a blocker to begin with.
(In reply to Brian R. Bondy [:bbondy] from comment #6)
> Also I haven't heard any clarification on updater related work with B2G. In
> particular I *think* we are only doing FOTA updates but I'm not sure.  And I
> *think* those updates will be packaged inside MARs as they are today, but
> I'm not sure if everyone is on board with that.  If people are not on board
> with that then this shouldn't even be a blocker to begin with.

The answer to this greatly affects the timeline of bug 793709. If RelEng doesn't need to sign gecko update mars, then there's very little work to be done. FOTA updates will have to be created and signed manually given the current processes.
Update: I think catlee got us what we need in bug 793709. 
Status: The next steps is for bsmith to load those certs in a read only NSS db.

To be clear: The November 19th deadline is completely dependent on bsmith's work being done. Without that this work cannot continue.

Also I'd like clarification on what an Exec Review is.
Do we need separate mar signing keys for dep/try builds, nightly builds, and release builds, like we have for desktop updates?
Maybe eventually but since this is only used for testing internal builds I don't think it's needed.  I'll defer to bsmith though.
(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Update: I think catlee got us what we need in bug 793709. 
> Status: The next steps is for bsmith to load those certs in a read only NSS
> db.
> 
> To be clear: The November 19th deadline is completely dependent on bsmith's
> work being done. Without that this work cannot continue.

Which bug # is bsmith's work that blocks this bug? It's not clear in the dependency tree.

Also, are the patches here ready for review? Or need new version after bsmith and catlee's work lands?

> Also I'd like clarification on what an Exec Review is.

It means that we'll likely need broader in-Mozilla as well as multi-partner approval to land feature work, since the risk of new code at that point puts the whole release schedule at significant risk.
(In reply to Dietrich Ayala (:dietrich) from comment #11)
> (In reply to Brian R. Bondy [:bbondy] from comment #8)
> > Update: I think catlee got us what we need in bug 793709. 
> > Status: The next steps is for bsmith to load those certs in a read only NSS
> > db.
> > 
> > To be clear: The November 19th deadline is completely dependent on bsmith's
> > work being done. Without that this work cannot continue.
> 
> Which bug # is bsmith's work that blocks this bug? It's not clear in the
> dependency tree.

I'm not sure off hand. bsmith can you chime in?


> Also, are the patches here ready for review? Or need new version after
> bsmith and catlee's work lands?

It will need extra fixes after catlee and bsmith's work lands, but not significant. I prepared most of the work without these dependencies to speed up any wait time on me.
Flags: needinfo?(bsmith)
I am planning to add the patches to this bug.
Flags: needinfo?(bsmith)
Got an "End of C1 tomorrow" email, asking questions.  Answers can be found below:

> * A comment providing the following details will need to be included:
> ** Why the bug should remain blocking-basecamp (layman's terms, 
> especially if the summary isn't human readable)

I'm not sure it should be blocking-basecamp.  This was only marked as blocking-basecamp originally based on a speculative need.  The requirements are still not known for updates.  If they are known, they are only recently known and have not been communicated yet. The need for such clarification has been known for over a month.

> ** Level of effort and risk involved

There is a real risk of breaking updates for b2g builds only.  Additional work from RelEng and QA may be able to lower that risk.

** Lower risk or lower effort alternatives to resolving the bug

What I think should be done, is to not sign MARs for now and to offer updates through HTTPS. We don't sign MARs today on neither of OSX nor Linux.  We could then do this work for a future release.  Updates are secure already as long as our update servers have not been compromised.

> ** ETA for resolution

I estimate a couple days of work from me once the dependencies are done.  The last I heard in bug 793709 was that this should not be enabled yet, I asked for clarification on when it should be enabled in that bug.  Also there is a task that bsmith mentioned he'd do in this bug (See Comment 13).  My ETA does not include bsmith's work / estimation.

> ** Whether the work is blocked

Yes.  By bsmith's work and by bug 793709 Comment 40. I think bsmith should probably add a different bug for his work instead of using this bug and it should have its C1-blocking comment like this. 

> * The whiteboard should then have the following added: [c1-ex:commentY] where
> Y is the comment where the exception request can be found.

Done.
Whiteboard: [WebAPI:P0][Blocked by bug 793709 which is a P3] → [WebAPI:P0][Blocked by bug 793709 which is a P3][c1-ex:comment14]
(In reply to Brian R. Bondy [:bbondy] from comment #14)
> ** Lower risk or lower effort alternatives to resolving the bug
> 
> What I think should be done, is to not sign MARs for now and to offer
> updates through HTTPS. We don't sign MARs today on neither of OSX nor Linux.
> We could then do this work for a future release.  Updates are secure already
> as long as our update servers have not been compromised.

Setting the b-b flag to ? given this proposed alternative.
blocking-basecamp: + → ?
Target Milestone: B2G C1 (to 19nov) → ---
We discussed this at length in triage today and we decided we don't want to release without signed MARs.  If there's information we don't have that this isn't as big of a security issue as we think it could be, please provide it here.
blocking-basecamp: ? → +
OK, to make it clear I cannot proceed with this work. See Comment 14 for details.
Lucky me!
Assignee: netzen → bsmith
:) I'd prefer if you created a new bug as a dependency and added the same tracking flags, but I won't hold you to it.
Can we get a priority and target milestone for this>
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
Whiteboard: [WebAPI:P0][Blocked by bug 793709 which is a P3][c1-ex:comment14] → [c1-ex:comment14]
What's the status here, bbondy/bsmith?
Flags: needinfo?
Same as on 2012-11-19, See Comment 14.  Also see Comment 17. Deferring to bsmith for an update beyond that.
Flags: needinfo?
The lack of action and the ambiguity of ownership in this bug is a problem.

Brian Smith, can you please file a separate bug for the blocking work you mention in comment #13 and add the patches there per comment #19? That would really help clear up the picture.
Whats going on here? bsmith?
Flags: needinfo?(bsmith)
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Do we have any update for this case? 
We don't have update for few days. Do we need someone to help?
I will be working on this starting tonight. I am not sure I need any help with it.

We are confident that all other aspects of the client-side MAR updating (other than signature checking) work right?
Flags: needinfo?(bsmith)
Whats the status here?
I am still actively working on it now, still trying to get it done by the end of the afternoon. Will require review from people in the US though.
Let's not block basecamp on this but we'll do it for v1 for sure.
blocking-b2g: --- → tef+
blocking-basecamp: + → -
Whiteboard: [c1-ex:comment14] → [c1-ex:comment14] [NPOTB] [Triaged:1/17]
I don't think [NPOTB] is appropriate because this stuff does get built as part of Gecko, but also I don't know exactly how we're using [NPOTB], so I won't remove it.
Brian, what is the latest on this bug?  This is still TEF+ so please let us know if this will not make the TEF+ deadline (this Friday Feb 1st).
Flags: needinfo?(bsmith)
(In reply to Lucas Adamski from comment #31)
> Brian, what is the latest on this bug?  This is still TEF+ so please let us
> know if this will not make the TEF+ deadline (this Friday Feb 1st).

Our next handoff is 15feb... any progress on this?
blocking-b2g: tef+ → shira?
akeybl: bah - sorry, I changed TEF+ to shira? by accident. And i dont have privs to undo. Can you correct for me?
back to tef+
blocking-b2g: shira? → tef+
Whiteboard: [c1-ex:comment14] [NPOTB] [Triaged:1/17] → [c1-ex:comment14][Triaged:1/17]
Brian, what is going on here?
Sorry for the lack of status update here. It is scarily close to being done but I've been getting distracted by other things. Will concentrate on getting this wrapped up.
Flags: needinfo?(bsmith)
Brian, 7 days without any changes. What's going on ?
Flags: needinfo?(bsmith)
From https://mail.mozilla.org/pipermail/packagedapps/2013-February/000137.html (Thursday 14 February):

"Last night I wrote some tests for this and surprised myself when one of them failed this morning. It is likely that we will have to redo the certs. Consequently, I doubt we will be able to do the migration to the new certs before MWC, and so we should rely on the old test certs for any MWC demos."
(In reply to Andrew Overholt [:overholt] from comment #38)
> From
> https://mail.mozilla.org/pipermail/packagedapps/2013-February/000137.html
> (Thursday 14 February):
> 
> "Last night I wrote some tests for this and surprised myself when one of
> them failed this morning. It is likely that we will have to redo the certs.
> Consequently, I doubt we will be able to do the migration to the new certs
> before MWC, and so we should rely on the old test certs for any MWC demos."

I think this is related to the packaged app signing bugs, but not this bug. This bug talks about general OTA updates with certs I believe.
(In reply to Andrew Overholt [:overholt] from comment #38)
> From
> https://mail.mozilla.org/pipermail/packagedapps/2013-February/000137.html
> (Thursday 14 February):
> 
> "Last night I wrote some tests for this and surprised myself when one of
> them failed this morning. It is likely that we will have to redo the certs.
> Consequently, I doubt we will be able to do the migration to the new certs
> before MWC, and so we should rely on the old test certs for any MWC demos."

Jason is right, this is about other bugs regarding Marketplace app signing, not this bug.
Flags: needinfo?(bsmith)
Whiteboard: [c1-ex:comment14][Triaged:1/17] → [c1-ex:comment14][Triaged:1/17][target 28/2]
Attached patch WIP Part 3: libmar changes (obsolete) — Splinter Review
Attachment #670489 - Attachment is obsolete: true
Attached patch WIP Part 4: updater changes (obsolete) — Splinter Review
These four patches are what I have so far for getting the tests in toolkit/mozapps/update/test/unit to mostly function correctly.

I say "mostly" because test_0010_general.js fails because it cannot write to /system/b2g/update_write_access_test, and because the change to runtestsb2g.py to add /system/b2g/updater_xpcshell_cert.der doesn't actually work. Oddly, the tset_0010_general.js test seemed to be working just fine for me until today, so I am not sure what I broke exactly. I will figure it out. That is a relatively minor problem.

So far, I've been testing this by manually "adb remount && adb push $(GECKO_DIR)/toolkit/mozapps/update/updater/xpcshellCertificate.der /system/b2g/updater_xpcshell_cert.der && adb reboot". I will probably need some help from ATeam people to get the runtestsb2g.py changes to actually work. The good thing about the problem is that it makes it easier to check that the tests correctly fail with a certificate verification error when the updater_xpcshell_cert.der file is missing.

I do not have the tests in modules/libmar/tests/unit working correctly yet. Also, there are some tests that are currently Windows-only that I need to enable for B2G too. Right now, I am punting on both of these issues until after I can get these four patches ready for review, because these two issues need a different kind of work. However, at least five of the already-enabled tests do exercise the signature verification functionality already.

bbondy: I am going to clean these patches up tonight before I ask for review. I will explain in detail why I had to make so many changes to your original patches. In short, it wasn't because you did anything wrong, but rather I found that it was very problematic for various reasons to support the OEM and carrier customization of the keys in the build process when relying on the NSS database, so I switched to storing the files in the filesystem. Also, I rearranged the control flow for the B2G case to match as closely as possible the Windows control flow, in order to make it clearer that the code is doing the correct thing (since we already know your Windows code is doing the correct thing).
Attachment #727405 - Attachment description: WIP 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests → WIP Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests
Attachment #727416 - Attachment description: Script for running the updater xpcshell tests (only) on B2G, based on dhyland's work → Script for running the updater xpcshell tests (only) on a device, based on dhyland's work
Brian, in your original patch you tried to do the MOZ_WIDGET_TOOLKIT check within b2g/confvars.sh to differentiate between B2G device builds and B2G desktop builds. But, I couldn't get that to work, because MOZ_WIDGET_TOOLKIT is set after $PRODUCT/confvars.sh is sourced. So, I just put the logic directly in configure.in. Suggestions welcome.

The way this works is that the person who runs B2G must put export MOZ_MAR_CERT_FILENAME_PREFIX=something in their .userconfig when building B2G. This will cause the build system to put $(MOZ_MAR_CERT_FILENAME_PREFIX)_primary.der and $(MOZ_MAR_CERT_FILENAME_PREFIX)_secondary.der into /system/b2g, and then the updater will use those certs as the primary and secondary certs for signature verification.

For example, you can set MOZ_MAR_CERT_FILENAME_PREFIX=export MOZ_MAR_CERT_FILENAME_PREFIX=nightly_aurora_level3 to use the Nighty/Aurora key, export MOZ_MAR_CERT_FILENAME_PREFIX=release to use the release keys, or export MOZ_MAR_CERT_FILENAME_PREFIX=/full/path/to/my/own/keys to use your own keys.
Attachment #727403 - Attachment is obsolete: true
Attachment #728006 - Flags: review?(ted)
Attachment #728006 - Flags: review?(netzen)
This modifies the xpcshell testing scripts for B2G so that they push the test certificate to the device so that the xpcshell tests can succeed. Unfortunately, it only works if you have done an "adb remount" before running the xpcshell tests. That is bad for two reasons:

1. There shouldn't be such a manual step required.
2. The xpcshell tests need to run in the *normal* configuration where /system is read-only.

One option would be to do a "adb remount" before writing the cert and an "adb reboot" after writing the cert, but that will slow down the running of the xpcshell tests significantly. Presumably, the addition of the xpcshell cert could happen elsewhere, like in whatever script actually flashes the device, or we could add a B2G build system option to make this part of the image. I worry that making this a build system option will be really dangerous because it would be easy to accidentally set that option in a release build and ruin the security of this whole system. Suggestions?
Attachment #728008 - Flags: feedback?(netzen)
Attachment #728008 - Flags: feedback?(marshall)
Attached patch Part 3: libmar changes (obsolete) — Splinter Review
Brian, these changes are different enough from your original patch that I think you can review them without worrying about reviewing too much of your own work.

As you can see, I basically just tried to make the non-Windows code paths for verification be the same as the Windows code paths. In general, I tried to make sure that there are as few distinct code paths as possible. I think this is helpful since both you and I are primarily Windows developers, so any effort we spend that works for both Windows and non-Windows is a savings in the long run. In the course of doing this, I simplified the code. Perhaps I over-simplified it beyond the intended scope of this bug. It made sense at the time, though.
Attachment #727408 - Attachment is obsolete: true
Attachment #728009 - Flags: review?(netzen)
Attached patch Part 3: libmar changes (obsolete) — Splinter Review
Attachment #728009 - Attachment is obsolete: true
Attachment #728009 - Flags: review?(netzen)
Attachment #728008 - Attachment description: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests → Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests
Attached patch Part 4: updater changes (obsolete) — Splinter Review
First, this patch adds some logic to make debugging the updater easier. You can make the updater print its PID and sleep for few seconds so that you can attach GDB to it to step through the code. And, all the LOG() output will go to logcat in addition to the log file. This made debugging MUCH simpler.

This patch generally tries to emulate the resource loading stuff that Windows does so that we can use the exact same code path that the Windows updater uses for signature verification. Instead of storing the certs in updater.exe, the certs are stored in /system/b2g/. This has the same effect since the updater itself lives in /system/b2g, so if you can write to that directory then you own the system.
Attachment #727414 - Attachment is obsolete: true
Attachment #728017 - Flags: review?(netzen)
Attachment #728017 - Flags: review?(marshall)
I found a problem with part 3 of the patch, which seems to still be breaking Windows. I am going to try to minimize (undo) some my simplificatoins of the code which should make things more obviously non-breaky.
Attachment #728008 - Attachment is patch: true
Comment on attachment 728017 [details] [diff] [review]
Part 4: updater changes

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

::: toolkit/mozapps/update/common/errors.h
@@ +66,5 @@
>  #define FOTA_UNKNOWN_ERROR 45
>  #define WRITE_ERROR_SHARING_VIOLATION_SIGNALED 46
>  #define WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID 47
>  #define WRITE_ERROR_SHARING_VIOLATION_NOPID 48
> +#define NSS_INIT_CONFIG_ERROR 49

We should also add this error code to toolkit\mozapps\update\nsUpdateService.js.  Check for the type of error handling you want to mirror when this happens. For example search that file for FOTA_GENERAL_ERROR if you want it to do the same as that.

::: toolkit/mozapps/update/common/updatelogging.cpp
@@ +79,5 @@
>    va_start(ap, fmt);
> +#if defined(MOZ_WIDGET_GONK)
> +  __android_log_print (ANDROID_LOG_INFO, "updater", "*** Warning: ");
> +  __android_log_vprint(ANDROID_LOG_INFO, "updater", fmt, ap);
> +  __android_log_print (ANDROID_LOG_INFO, "updater", "***\n");

ANDROID_LOG_WARN?

::: toolkit/mozapps/update/updater/Makefile.in
@@ +174,5 @@
>  	cp $< $@
>  libs:: updater_xpcshell_cert.der
>  
>  endif
> +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))

you should probably pull out this file for the buildconfig patch

@@ +180,5 @@
> +ifneq (,$(MOZ_MAR_CERT_FILENAME_PREFIX))
> +updater_primary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_primary.der
> +	cp $< $@
> +updater_secondary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_secondary.der
> +	cp $< $@

Is there only one channel for B2G?
Otherwise we need one (primary,secondary) for each channel.

@@ +186,5 @@
> +	$(NSINSTALL) $< $(dir $@)
> +$(DIST)/bin/updater_secondary_cert.der: updater_secondary_cert.der
> +	$(NSINSTALL) $< $(dir $@)
> +libs:: $(DIST)/bin/updater_primary_cert.der \
> +       $(DIST)/bin/updater_secondary_cert.der

I think these need to be added to some package manifest too. Are you sure they will also be installed to exiting devices?

@@ +194,5 @@
> +# can find it, but we don't want to copy it to $(DIST)/bin because we
> +# don't want it to be distributed to end users.
> +updater_xpcshell_cert.der: xpcshellCertificate.der
> +	cp $< $@
> +libs:: updater_xpcshell_cert.der

Has this passed try tests? As far as I understand the machine that builds is not the same machine that builds tests and I'm not sure if this will be included in the zip downloaded by the machine that does tests or not.

::: toolkit/mozapps/update/updater/archivereader.cpp
@@ +60,5 @@
> +  if (!f) {
> +    return false;
> +  }
> +
> +  fclose(f);

Use stat instead unless you want to be sure read access is granted.  If that is the case add a comment here describing that.

@@ +108,2 @@
>    size = SizeofResource(handle, resourceInfoBlockHandle);
>    data = static_cast<const uint8_t*>(::LockResource(resourceHandle));

The SizeOfResource API can fail (returns 0 on failure).
The LockResource API can fail (returns NULL on failure).
While you're here please check for these values and return false.
I think that mar verify signatures would just error out but it's best to check it here because the values get passed to CryptoX which is library dependent.

@@ +113,5 @@
> +  LOG(("Before GetResourceFilePath"));
> +
> +  const char * src = GetResourceFilePath(name);
> +  if (!src) {
> +    LOG(("GetResourceFilePath failed: %d", (int) name));

name is already an int

@@ +119,5 @@
> +  }
> +
> +  LOG(("After GetResourceFilePath: %s", src));
> +
> +  int rv = mar_read_entire_file(src, 16 * 1024, &data, &size);

Will certs always be <16K? (I think so just checking)
Maybe you can pull out this value to a header file.

@@ +121,5 @@
> +  LOG(("After GetResourceFilePath: %s", src));
> +
> +  int rv = mar_read_entire_file(src, 16 * 1024, &data, &size);
> +
> +  return rv == 0;

nit: rv == OK

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2195,5 @@
>        gArchiveReader.Close();
>      }
>    }
>  
> +  // TODO: is this the right thing to do for B2G?

Check with rstrong or ehsan to be sure please

@@ +2287,5 @@
> +  if (NSS_NoDB_Init(NULL) != SECSuccess) {
> +    PRErrorCode error = PR_GetError();
> +    LOG(("Could not initialize NSS: %s (%d)", PR_ErrorToName(error), (int) error));
> +    _exit(1);
> +  }

#if defined(MOZ_WIDGET_GONK) around this block.
Attachment #728017 - Flags: review?(netzen) → review-
Comment on attachment 728017 [details] [diff] [review]
Part 4: updater changes

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

::: toolkit/mozapps/update/updater/Makefile.in
@@ +180,5 @@
> +ifneq (,$(MOZ_MAR_CERT_FILENAME_PREFIX))
> +updater_primary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_primary.der
> +	cp $< $@
> +updater_secondary_cert.der: $(MOZ_MAR_CERT_FILENAME_PREFIX)_secondary.der
> +	cp $< $@

Ignore the comment about a (primary, secondary) for each channel I seen the comment explaining it after writing this.
Attachment #728006 - Flags: review?(netzen) → feedback+
Comment on attachment 728008 [details] [diff] [review]
Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests

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

> One option would be to do a "adb remount" before writing the cert and an "adb reboot" after writing the cert, 
> but that will slow down the running of the xpcshell tests significantly.
Maybe we can do it at most once for all xpcshell tests and not on each xpcshell test.
Attachment #728008 - Flags: feedback?(netzen)
Comment on attachment 728017 [details] [diff] [review]
Part 4: updater changes

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

::: toolkit/mozapps/update/common/errors.h
@@ +66,5 @@
>  #define FOTA_UNKNOWN_ERROR 45
>  #define WRITE_ERROR_SHARING_VIOLATION_SIGNALED 46
>  #define WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID 47
>  #define WRITE_ERROR_SHARING_VIOLATION_NOPID 48
> +#define NSS_INIT_CONFIG_ERROR 49

I removed this error code because now I am not using it.

::: toolkit/mozapps/update/common/updatelogging.cpp
@@ +79,5 @@
>    va_start(ap, fmt);
> +#if defined(MOZ_WIDGET_GONK)
> +  __android_log_print (ANDROID_LOG_INFO, "updater", "*** Warning: ");
> +  __android_log_vprint(ANDROID_LOG_INFO, "updater", fmt, ap);
> +  __android_log_print (ANDROID_LOG_INFO, "updater", "***\n");

Thanks! Changed.

::: toolkit/mozapps/update/updater/Makefile.in
@@ +174,5 @@
>  	cp $< $@
>  libs:: updater_xpcshell_cert.der
>  
>  endif
> +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))

Let's not worry about that. I am going to fold all of these patches into a single commit when landing it anyway. The important thing is that changes to configure.in and b2g/confvars.sh and similar should be in the first patch and that patch should change as little as possible to prevent needing to completely rebuild everything during development.

@@ +186,5 @@
> +	$(NSINSTALL) $< $(dir $@)
> +$(DIST)/bin/updater_secondary_cert.der: updater_secondary_cert.der
> +	$(NSINSTALL) $< $(dir $@)
> +libs:: $(DIST)/bin/updater_primary_cert.der \
> +       $(DIST)/bin/updater_secondary_cert.der

I did this in the buildconfig part of the patch (Part 1).

@@ +194,5 @@
> +# can find it, but we don't want to copy it to $(DIST)/bin because we
> +# don't want it to be distributed to end users.
> +updater_xpcshell_cert.der: xpcshellCertificate.der
> +	cp $< $@
> +libs:: updater_xpcshell_cert.der

Right. In general it isn't clear how to get the xpcshell cert onto the device for testing purposes. I will find a solution for that with ATeam on Monday.

::: toolkit/mozapps/update/updater/archivereader.cpp
@@ +60,5 @@
> +  if (!f) {
> +    return false;
> +  }
> +
> +  fclose(f);

I will add a comment explaining the reasoning.

@@ +108,2 @@
>    size = SizeofResource(handle, resourceInfoBlockHandle);
>    data = static_cast<const uint8_t*>(::LockResource(resourceHandle));

Done.

@@ +119,5 @@
> +  }
> +
> +  LOG(("After GetResourceFilePath: %s", src));
> +
> +  int rv = mar_read_entire_file(src, 16 * 1024, &data, &size);

Done.

@@ +121,5 @@
> +  LOG(("After GetResourceFilePath: %s", src));
> +
> +  int rv = mar_read_entire_file(src, 16 * 1024, &data, &size);
> +
> +  return rv == 0;

This function is defined to return 0 or -1, so I think comparison to zero is better.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2287,5 @@
> +  if (NSS_NoDB_Init(NULL) != SECSuccess) {
> +    PRErrorCode error = PR_GetError();
> +    LOG(("Could not initialize NSS: %s (%d)", PR_ErrorToName(error), (int) error));
> +    _exit(1);
> +  }

I moved this to the top of the function and removed the logging. NSS_NoDB_Init should never fail because it doesn't need to open any files or anything except shared libraries. If we can't open the NSS shared libraries then we're really screwed probably didn't even get to main() due to dynamic linker failures.

@@ +2416,5 @@
> +  LOG(("updater process PID: %d", (int) getpid()));
> +  if (getenv("MOZ_UPDATER_WAIT_FOR_DEBUGGER")) {
> +    sleep(10);
> +  }
> +#endif

I found out after I wrote this that the reason the Windows tests were failing is that they are looking for exact matches of the LOG() output. So, we should avoid using LOG() for these types of things because it would make it harder to adapt the tests to B2G. Instead, I now call __android_log_print directly.
> I moved this to the top of the function and removed the logging. 
>NSS_NoDB_Init should never fail because it doesn't need to open any 
> files or anything except shared libraries

Even if it can't fail I don't see any point running it on other platforms.  Doing extra things that may load in extra libraries isn't good in general because on Windows this code runs through the system account and is susceptible to DLL injection.
(In reply to Brian R. Bondy [:bbondy] from comment #57)
> > I moved this to the top of the function and removed the logging. 
> >NSS_NoDB_Init should never fail because it doesn't need to open any 
> > files or anything except shared libraries
> 
> Even if it can't fail I don't see any point running it on other platforms. 
> Doing extra things that may load in extra libraries isn't good in general
> because on Windows this code runs through the system account and is
> susceptible to DLL injection.

Sorry, I forgot to point this out already: There isn't enough context in the patch to see it, but this is already within #ifdef MOZ_WIDGET_GONK, so there are no worries here. (Also, AFAICT the updater only links to NSS when building for gonk so this code wouldn't build if it weren't ifdef'd.)

I found a memory leak in my code. I am going to finish cleaning this up today and submit it for re-review, including part 3.
Attached patch Part 3, v2: libmar changes. (obsolete) — Splinter Review
Attachment #728015 - Attachment is obsolete: true
Attachment #729123 - Flags: review?(netzen)
Attached patch Part 4, v3: updater changes (obsolete) — Splinter Review
Attachment #728017 - Attachment is obsolete: true
Attachment #728017 - Flags: review?(marshall)
Attachment #729124 - Flags: review?(netzen)
LOG(...) -> __android_log_print(...) in the case where NSS initialization failed, removed // TODO comment.
Attachment #729124 - Attachment is obsolete: true
Attachment #729124 - Flags: review?(netzen)
Attachment #729140 - Flags: review?(netzen)
There is a test that tries to verify that the directory containing the updater executable is writable. I do not know the purpose of that test, but that is not a valid test for B2G because the updater is in /system/b2g which is not writable until the updater remounts for filesystem to make it so. I split that test into two tests, one which tests for the writable updater directory, and another test that tests the other things, and I disabled the writable updater directory test for B2G.

I also enabled the version downgrade and channel tests for B2G.
Attachment #729153 - Flags: review?(netzen)
Severity: normal → critical
Comment on attachment 729140 [details] [diff] [review]
Part 4, v3: updater changes

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

::: toolkit/mozapps/update/updater/archivereader.cpp
@@ +109,3 @@
>    }
>  
> +  // XXX: can LockResource or SizeOfResource fail?

This comment can be removed now.

@@ +116,2 @@
>    FreeLibrary(handle);
> +  return true;

Since you went through all the trouble of computing `result` you should maybe return it here? ;)
Attachment #729140 - Flags: review?(netzen) → review+
Comment on attachment 729123 [details] [diff] [review]
Part 3, v2: libmar changes.

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

::: modules/libmar/moz.build
@@ +12,1 @@
>      # On Windows we don't verify with NSS and updater needs to link to it

I think when the moz.build conversion happened this comment was misplaced, please remove.

::: modules/libmar/src/mar.h
@@ +129,5 @@
>   * @return          A non-zero value if an error occurs.
>   */
>  int mar_extract(const char *path);
>  
> +#define MAR_MAX_CERT_SIZE (16*1024) // Way larger than necessary

nit: spaces before and after multiplication
#define MAR_MAX_CERT_SIZE (16 * 1024) // Way larger than necessary

::: modules/libmar/tool/mar.c
@@ +336,5 @@
> +
> +    if (!rv) {
> +      MarFile *mar = mar_open(argv[2]);
> +      if (mar) {
> +        rv = mar_verify_signatures(mar, certBuffers, fileSizes, certCount);

I like this change, thanks.

::: modules/libmar/verify/mar_verify.c
@@ +33,5 @@
> +  if (!f) {
> +    return -1;
> +  }
> +
> +  result = -1;

nit: just initialize result above to be -1.
Attachment #729123 - Flags: review?(netzen) → review+
Comment on attachment 729153 [details] [diff] [review]
Enable two additional tests for B2G, and disable part of another

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

::: toolkit/mozapps/update/test/unit/test_0010_general.js
@@ -15,5 @@
> -  var testFile = getCurrentProcessDir();
> -  testFile.append("update_write_access_test");
> -  logTestInfo("trying to create file " + testFile.path);
> -  testFile.create(AUS_Ci.nsIFile.NORMAL_FILE_TYPE, 0644);
> -  do_check_true(testFile.exists());

I don't think removing this is OK but please check with rstrong.
Perhaps just only skip it for b2g.

@@ -18,5 @@
> -  testFile.create(AUS_Ci.nsIFile.NORMAL_FILE_TYPE, 0644);
> -  do_check_true(testFile.exists());
> -  testFile.remove(false);
> -  do_check_false(testFile.exists());
> -

I don't think removing this is OK but please check with rstrong.
Perhaps just only skip it for b2g.

::: toolkit/mozapps/update/test/unit/xpcshell.ini
@@ +3,5 @@
>  tail = 
>  
>  [test_0010_general.js]
> +[test_0011_updaterDirectoryWritable.js]
> +skip-if = toolkit = "gonk"

Please check with rstrong on this

::: toolkit/mozapps/update/test/unit/xpcshell_updater.ini
@@ +4,5 @@
>  [test_0113_general.js]
>  skip-if = os == "mac" || toolkit == "gonk"
>  reason = bug 820380
> +[test_0113_versionDowngradeCheck.js]
> +[test_0114_productChannelCheck.js]

I think we want these to be run-if = os == 'win' || toolkit == "gonk"
Attachment #729153 - Flags: review?(netzen) → review-
Comment on attachment 728006 [details] [diff] [review]
Part 1: build configuration changes.

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

::: configure.in
@@ +6305,5 @@
>      MOZ_VERIFY_MAR_SIGNATURE=1,
>      MOZ_VERIFY_MAR_SIGNATURE= )
>  
>  if test -n "$MOZ_VERIFY_MAR_SIGNATURE"; then
> +  if test "$OS_ARCH" = "WINNT" -o -n "$gonkdir"; then

Can you test "$MOZ_WIDGET_TOOLKIT" = "gonk" instead here?

::: toolkit/mozapps/update/updater/Makefile.in
@@ +163,5 @@
> +	$(NSINSTALL) $< $(dir $@)
> +$(DIST)/bin/updater_secondary_cert.der: updater_secondary_cert.der
> +	$(NSINSTALL) $< $(dir $@)
> +libs:: $(DIST)/bin/updater_primary_cert.der \
> +       $(DIST)/bin/updater_secondary_cert.der

I really don't want to r+ adding more random rules like this to Makefile.in when we're trying to get rid of them in the transition to moz.build. Can you rewrite these in terms of INSTALL_TARGETS instead? Example:
http://mxr.mozilla.org/mozilla-central/source/browser/branding/aurora/Makefile.in#63
Attachment #728006 - Flags: review?(ted)
wonder what's the latest on this bug? and if there is anything blocking this bug from moving along? thanks
Comment on attachment 728008 [details] [diff] [review]
Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests

Jonathan, does this look right to you?

As I mentioned to Clint, we need to stick this file in /system/b2g which is normally not a writable location. It seems like this is the function that writes other stuff into that location. I guess at some point /system is remounted to be read+write so we can write the files there.

But, when we run the xpcshell tests, we should be running them with /system mounted as read-only, because the updater code that is being tested here is actually responsible for ensuring that system/b2g is writable as part of its execution; if system/b2g is already writable then that part of the code isn't really being tested. I don't know if/how the B2G xpcshell test framework is handling this. Perhaps dealing with this should be a follow-up issue.
Attachment #728008 - Flags: review?(jgriffin)
Comment on attachment 728008 [details] [diff] [review]
Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests

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

::: testing/xpcshell/runtestsb2g.py
@@ +46,5 @@
>  
>      # Overridden
>      def pushLibs(self):
> +        updater_xpcshell_cert_dest = os.path.join(self.options.deviceRoot,
> +                                 '../../../system/b2g/updater_xpcshell_cert.der')

We can probably just use '/system/b2g/updater_xpcshell_cert.der', no?  That path is not related to deviceRoot, so no need to os.path.join().

@@ +50,5 @@
> +                                 '../../../system/b2g/updater_xpcshell_cert.der')
> +        print 'pushing %s' % updater_xpcshell_cert_dest
> +        updater_xpcshell_cert_src = os.path.join(
> +                         os.path.split(os.path.abspath(__file__))[0],
> +                         '../../../toolkit/mozapps/update/updater/updater_xpcshell_cert.der')

Where does this file come from?  It's going to be in gecko, under /toolkit/mozapps/update/updater?

This logic won't work for running the tests in many environments (including TBPL), since those tests only have access to the contents of tests.zip (i.e., the test-package-stage directory), and not the src or obj directory this was built from.  We probably want to copy the .der file into xpcshell dir here, http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#61, and then remove this os.path.join() logic.

@@ +52,5 @@
> +        updater_xpcshell_cert_src = os.path.join(
> +                         os.path.split(os.path.abspath(__file__))[0],
> +                         '../../../toolkit/mozapps/update/updater/updater_xpcshell_cert.der')
> +        print 'from %s' % updater_xpcshell_cert_src
> +        self.device.pushFile(updater_xpcshell_cert_src, updater_xpcshell_cert_dest, retryLimit=10)

Does this work without an 'adb remount'?
Attachment #728008 - Flags: review?(jgriffin) → review-
Comment on attachment 728008 [details] [diff] [review]
Part 2: Have runtestsb2g.py push updater xpcshell cert before running xpcshell tests

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

Deferring to jgriffin's comments.
Attachment #728008 - Flags: feedback?(marshall)
Been over a week since last comment, so checking in - what needs to happen to move this forward?
Today I am finishing up the updated patches and testing them. I will king jgriffin if I have any more problems with the testing part.

Also, when this lands, Gecko/Gaia updates created by releng will probably not apply on the device unless/until they reconfigure the build infrastructure. I will work on documenting what they need to do.
Whiteboard: [c1-ex:comment14][Triaged:1/17][target 28/2] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-15]
Thanks Brian. What is the new ETA for this?
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-15] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-15][status: needs new ETA]
I got a clean run of this last night on Try on B2G:
https://tbpl.mozilla.org/?tree=Try&rev=e54951568c44

I want to re-run it on try with all the platforms and all tests enabled to make sure I didn't break anything on non-B2G platforms. I will do so as soon as Try starts working again. (Currently I cannot push to try.)

So, I am expecting to post all the patches for review this afternoon. Then, it will be up to the reviewers' availability.
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-15][status: needs new ETA] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-17]
https://tbpl.mozilla.org/?tree=Try&rev=67d198a01de2

It took a very long time to get these try results today, but the try run is all green. I will post the updated patches in the morning (Pacific time). New ETA is Pacific time too, to allow time for reviews.
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-17] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-19]
It looks like that try job didn't contain any commits.
(In reply to Jonathan Griffin (:jgriffin) from comment #77)
> It looks like that try job didn't contain any commits.

If you pull that revision, you will see that there are five patches.

The first time I pushed this set of patches, I did so with only B2G Mochitests selected. Then, I re-pushed the same set of patches with all tests enabled on all platforms. TBPL only shows the changesets that are new since the last time you pushed, which is why it only shows the "new" try changeset.
Whiteboard: [c1-ex:comment14][Triaged:1/17][eta 2013-04-19] → [c1-ex:comment14][Triaged:1/17][eta 2013-04-19][madrid]
Per gal and clee, we are not going to implement OTA for 1.0.1 or 1.1 or in the near feature.  FOTA works fine for our needs.  Until there is a product need or free cycles, this work does need to continue.
blocking-b2g: tef+ → -
You also told us in comment 28 that this would be landed 3 months ago.
Target Milestone: B2G C4 (2jan on) → Madrid (19apr)
(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #81)
> You also told us in comment 28 that this would be landed 3 months ago.

Six months ago in Comment 6, and again five months ago in Comment 14 I expressed that clarification on requirements was needed, and that I didn't think this should be done in the first place.   That is not counting several emails.  Based on things being unclear I'm not surprised that this was not Brian Smith's #1 priority.  But who's counting.
Target Milestone: 1.0.1 Madrid (19apr) → ---
Comment on attachment 729123 [details] [diff] [review]
Part 3, v2: libmar changes.

I'll be taking bsmith's already reviewed patch for libmar changes into bug 903135.  Marking it as obsolete here and uploading it there.
Attachment #729123 - Attachment is obsolete: true
Comment on attachment 729140 [details] [diff] [review]
Part 4, v3: updater changes

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

Some of this patch is superseded by work being done in bug 903135, and some of it would need to be changed now as well if we ever revisit this bug.
Attachment #729140 - Flags: review+
Assignee: brian → nobody
I don't think this is valid anymore, re-open if you disagree.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.