ubuntu-appstore-developers team mailing list archive

[Jamie Strandboge <jamie@xxxxxxxxxxxxx>]

On 08/29/2014 09:11 AM, Daniel Holbach wrote:
> Hello everybody,
> 
> it'd make Alan and my lives easier (until there's automatic
> reviews/rejections/etc.), if we could somehow track current overrides.
> Currently there's mainly the core apps and things such as the PayUI
> which override some of the errors and warnings which would normally
> block apps from going through.
> 
> Is there a way you would like something like this to be implemented?
> 
> My idea would have been to change the output of click-review to simply show:
> 
>  - Known overrides
>  - new errors/warnings
>  - general info messages
> 
> If there are no new errors or warnings, it would simply say "pass" and
> let us approve the app.
> 
> To track known overrides, we could maybe just store a piece of json like
> this somewhere:
> 
> {
>   "com.canonical.payui":
>   {
>     "security_template_valid (payui.json)":
>       "(MANUAL REVIEW) 'unconfined' not allowed",
>     "lint_hooks_redflag_payui":
>       "(MANUAL REVIEW) 'pay-ui' not allowed"
>   }
> }
> 
> What do you think?
> 

Sorry for not responding earlier-- as you'll see, I have a lot to say on the
manner. :)

In general, I am not opposed to something like this, but we need to be very
careful. What I am most concerned about is the malicious developer that games
the system. Eg, the developer uploads an otherwise legitimate app that is
reviewed, requires a manual review (eg unconfined, reserved policy groups, etc)
and it gets the manual review. We update the json to remember for next time, and
then the next upload, the upload contains malicious code that takes advantage of
the overrides and the app sails through. Another example is my app permy--
currently it uses "read_path" in its security policy but (intentionally) does
not include the "networking" policy group. If permy were to be overridden, we
would want to prompt for manual review if the networking policy group is added
in the future.

In addition to that, we need to make sure the overrides are maintained. Eg, if
an app uses a reserved policy group, has an override added, then drops the
policy group, the override should be removed such that if the app starts to use
the policy group again, it triggers a manual review (policy group is but one
example).

To spark the conversation, I'll toss out some ideas that might be worth considering:
 - including the reviewer who added the override to the json
 - including the date the override was added
 - including a note justifying the override
 - including an optional vendor tag
 - including an optional requires_new_review tag
 - only allow 'pass'es on apps with only warning for overrides
 - require manual review for errors, but display the json data to the
   reviewer
 - new errors and warnings should always prompt a new review (eg, the override
   can't mask new warnings)
 - if there is a security policy override and if security policy changes in any
   way (including policy_version), prompt for a new review

The idea behind the vendor tag is that we can perhaps do different things
depending on the vendor or if it is omitted. Eg, perhaps if the vendor is:
 - "Canonical" (ie, official Canonical apps), both errors and warnings get a
   pass
 - "BQ" (ie, official apps from a trusted vendor), both errors and warning get
   a pass
 - "Ubuntu" (eg, not really official Canonical apps, but not apps from
   developers either), warnings get a pass, errors are displayed
 - omitted (ie, normal app without special consideration), warnings get a pass,
   errors are displayed

The idea behind the 'requires_new_review' tag is similar, we can do different
things depending on the value. If it is present and true, we will always trigger
for manual review. The idea here is that we can add the override data so it can
be shown to the reviewer, but it will never get an automatic pass for what is
being overridden. This might be useful for apps that need a full code review to
verify if their use of a particular policy group is safe.

Process wise, I think it is important that overrides are the exception and their
justification must be sound. We should have guidelines for adding overrides (eg,
"apps with Canonical in the vendor field of the override should use the
'com.canonical' namespace") and ee should not add overrides for things just to
push them through. Furthermore, addition of overrides must be peer reviewed.
Using a bzr branch seems an easy way to achieve this (it can include the
guidelines as well).

Possible json using examples from above:
{
  "com.canonical.payui":
  {
    "reviewer": "dholbach",
    "date": "2014-09-07",
    "note": "Canonical supported. unconfined ok per jdstrand, pay-ui redflagged
ok per conversation with ted, dholbach and jdstrand",
    "vendor": "Canonical",
    "overrides": {
      "error": {
        "security_template_valid (payui.json)":
          "(MANUAL REVIEW) 'unconfined' not allowed",
        "lint_hooks_redflag_payui":
          "(MANUAL REVIEW) 'pay-ui' not allowed"
      },
      "warn": {}
    }
  },
  "com.bq.super-contacts":
  {
    "reviewer": "dholbach",
    "date": "2014-09-07",
    "note": "Special (official) contacts App from BQ. Necessarily needs access
to global contacts",
    "vendor": "BQ",
    "overrides": {
      "error": {
        "security_policy_groups_safe_contacts (super-contacts.json)":
          "(MANUAL REVIEW) reserved policy group 'contacts': vetted applications
only"
      },
      "warn": {}
    }
  },
  "com.ubuntu.music":
  {
    "reviewer": "popey",
    "date": "2014-09-07",
    "note": "Unconfined ok per jdstrand, but should move to confined with
music-files-read policy group in the future",
    "overrides": {
      "error": {
        "security_template_valid (apparmor.json)":
          "(MANUAL REVIEW) 'unconfined' not allowed"
      },
      "warn": {
        "lint_click_local_extensions":
          "found unofficial extensions: x-source, x-test"
        }
      }
    }
  },
  "com.ubuntu.developer.jdstrand.permy":
  {
    "reviewer": "popey",
    "date": "2014-09-07",
    "note": "App permissions viewer-- requires access to click security json.
Should not include networking",
    "overrides": {
      "error": {
        "security_redflag_fields (permy.json)":
          "found redflagged fields (needs human review): read_path"
      },
      "warn": {}
    }
  },
  "com.ubuntu.developer.foo.bar":
  {
    "reviewer": "jdstrand",
    "date": "2014-09-07",
    "note": "Uses global contacts list in read only. Code review shown to abuse
access. New code should be verified this is still true.",
    "requires_new_review": true,
    "overrides": {
      "error": {
        "security_policy_groups_safe_contacts (apparmor.json)":
          "(MANUAL REVIEW) reserved policy group 'contacts': vetted applications
only"
      },
      "warn": {}
    }
  }
}

-- 
Jamie Strandboge                 http://www.ubuntu.com/

Attachment: signature.asc
Description: OpenPGP digital signature