AU: Restrict URL/version change over D-Bus to non-official builds only.

AU: Restrict URL/version change over D-Bus to non-official builds only.

BUG=7159
TEST=unit tests, test on device

Change-Id: I7dd6293d3e61db123b2be2f1e3ee283fcb5ba968

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=a07586b

[petkov, 2010-10-20 19:55:32]

This tightens security a bit. We can add support for dev mode, if necessary, in
a separate CL. Unfortunately, for that we'll need to invoke an external utility
-- it might be nice if there was a boot mode library.

[adlr, 2010-10-20 20:02:14]

Can't we detect the dev switch by looking in a file and knowing some magic
numbers? (I guess we might like to embed those in a library...)

On Wed, Oct 20, 2010 at 12:55 PM, <petkov@chromium.org> wrote:

> Reviewers: adlr,
>
> Message:
> This tightens security a bit. We can add support for dev mode, if
> necessary, in
> a separate CL. Unfortunately, for that we'll need to invoke an external
> utility
> -- it might be nice if there was a boot mode library.
>
>
> Description:
> AU: Restrict URL/version change over D-Bus to non-official builds only.
>
> BUG=7159
> TEST=unit tests, test on device
>
> Change-Id: I7dd6293d3e61db123b2be2f1e3ee283fcb5ba968
>
> Please review this at http://codereview.chromium.org/3965001/show
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/update_engine.git
>
> Affected files:
>  M dbus_service.cc
>  M utils.h
>  M utils.cc
>
>
> Index: dbus_service.cc
> diff --git a/dbus_service.cc b/dbus_service.cc
> index
>
677dbc85990469fdb8e1a293bd833b1acf5268ea..63fbf501da1f2c841d4166e63c3211ef782be261
> 100644
> --- a/dbus_service.cc
> +++ b/dbus_service.cc
> @@ -3,9 +3,13 @@
>  // found in the LICENSE file.
>
>  #include "update_engine/dbus_service.h"
> +
>  #include <string>
> -#include "base/logging.h"
> +
> +#include <base/logging.h>
> +
>  #include "update_engine/marshal.glibmarshal.h"
> +#include "update_engine/utils.h"
>
>  using std::string;
>
> @@ -51,8 +55,18 @@ gboolean
> update_engine_service_attempt_update(UpdateEngineService* self,
>                                               gchar* app_version,
>                                               gchar* omaha_url,
>                                               GError **error) {
> -  const string update_app_version = app_version ? app_version : "";
> -  const string update_omaha_url = omaha_url ? omaha_url : "";
> +  string update_app_version;
> +  string update_omaha_url;
> +  // Only non-official (e.g., dev and test) builds can override the
> current
> +  // version and update server URL over D-Bus.
> +  if (!chromeos_update_engine::utils::IsOfficialBuild()) {
> +    if (app_version) {
> +      update_app_version = app_version;
> +    }
> +    if (omaha_url) {
> +      update_omaha_url = omaha_url;
> +    }
> +  }
>   LOG(INFO) << "Attempt update: app_version=\"" << update_app_version <<
> "\" "
>             << "omaha_url=\"" << update_omaha_url << "\"";
>   self->update_attempter_->CheckForUpdate(app_version, omaha_url);
> Index: utils.cc
> diff --git a/utils.cc b/utils.cc
> index
>
c8a4da64e29cf1d23037a4b9c7c329ccc593ef72..162fe7b8d9ea51c59c19491beee4a861707bd080
> 100644
> --- a/utils.cc
> +++ b/utils.cc
> @@ -39,14 +39,10 @@ namespace chromeos_update_engine {
>  namespace utils {
>
>  static const char kOOBECompletedMarker[] =
> "/home/chronos/.oobe_completed";
> +static const char kDevImageMarker[] = "/root/.dev_mode";
>
>  bool IsOfficialBuild() {
> -  OmahaRequestDeviceParams params;
> -  if (!params.Init("", "")) {
> -    return true;
> -  }
> -  return params.app_track != "buildbot-build" &&
> -      params.app_track != "developer-build";
> +  return !file_util::PathExists(FilePath(kDevImageMarker));
>  }
>
>  bool IsOOBEComplete() {
> Index: utils.h
> diff --git a/utils.h b/utils.h
> index
>
9beae04b9bb025ed1782c57db4efc7562e49c4e0..57fb3bb36adc735f32cbccc0615f14b065bcf3e3
> 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -21,10 +21,7 @@ namespace chromeos_update_engine {
>
>  namespace utils {
>
> -// Returns true if this is an official Chrome OS build, false
> -// otherwise. Currently, this routine errs on the official build side
> -// -- if it doesn't recognize the update track as non-official, it
> -// assumes the build is official.
> +// Returns true if this is an official Chrome OS build, false otherwise.
>  bool IsOfficialBuild();
>
>  // Returns true if the OOBE process has been completed and EULA accepted,
> false
>
>
>

[petkov, 2010-10-20 20:06:18]

This is what wad's bootmode binary is doing or supposed to do. Ultimately, it
might be nice for it to provide a library.

However, I don't think we need to open the URL/version change over D-Bus in
dev-mode any way.

[adlr, 2010-10-20 20:23:00]

LGTM

I'm just worried about how we will test updates. I guess we can do that by
modifying /mnt/stateful_partition/etc/lsb-release, so when we plug that hole
we'll need a workaround.

On Wed, Oct 20, 2010 at 1:06 PM, <petkov@chromium.org> wrote:

> This is what wad's bootmode binary is doing or supposed to do. Ultimately,
> it
> might be nice for it to provide a library.
>
> However, I don't think we need to open the URL/version change over D-Bus in
> dev-mode any way.
>
>
>
> http://codereview.chromium.org/3965001/show
>

[petkov, 2010-10-20 20:24:15]

On 2010/10/20 20:23:00, adlr wrote:
> LGTM
> 
> I'm just worried about how we will test updates. I guess we can do that by
> modifying /mnt/stateful_partition/etc/lsb-release, so when we plug that hole
> we'll need a workaround.

That's right -- I believe the tracker issue regarding
/mnt/stateful.../lsb-release says that we need to enable it only in dev mode.
It's good to have as few possible doors as possible.

> 
> On Wed, Oct 20, 2010 at 1:06 PM, <mailto:petkov@chromium.org> wrote:
> 
> > This is what wad's bootmode binary is doing or supposed to do. Ultimately,
> > it
> > might be nice for it to provide a library.
> >
> > However, I don't think we need to open the URL/version change over D-Bus in
> > dev-mode any way.
> >
> >
> >
> > http://codereview.chromium.org/3965001/show
> >
>