ubuntu-appstore-developers team mailing list archive

[Ricardo Kirkner <ricardo.kirkner@xxxxxxxxxxxxx>]

On Wed, Sep 17, 2014 at 10:44 AM, Jamie Strandboge <jamie@xxxxxxxxxxxxx>
wrote:

> 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": {}
>     }
>   }
> }
>
>
All of this sounds pretty reasonable, however I'm now somewhat confused. I
think we're talking about different things here. So to disambiguate,

1. the process is to override errors so to not automatically reject, but
force a manual review, and *not* to override errors to allow an automatic
approval, right?
2. the json data proposed in this thread, where does that live? does this
belong in each click package metadata? is this the json returned by the
click-reviewers-tools scripts?


> --
> Jamie Strandboge                 http://www.ubuntu.com/
>
>
> --
> Mailing list: https://launchpad.net/~ubuntu-appstore-developers
> Post to     : ubuntu-appstore-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~ubuntu-appstore-developers
> More help   : https://help.launchpad.net/ListHelp
>
>