New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Integrity Check #19036

Closed
MTRichards opened this Issue Sep 14, 2015 · 19 comments

Comments

Projects
None yet
7 participants
@MTRichards
Contributor

MTRichards commented Sep 14, 2015

As an ownCloud Admin, I want to know if my ownCloud apps and core ownCloud code have been altered so that I can ensure security of the code (hacked, or patched), and better predict conflicts during an upgrade process

Acceptance Criteria:

  • An app delivers this capability (enterprise only) and can be enabled or disabled by the admin as any other app
  • The app is shipped with ownCloud Server, and supported for Enterprise customers
  • App is called “Code Integrity Check”
  • A button exists in the admin panel that says “check ownCloud integrity”
  • When button is pressed, ownCloud core integrity is checked against the stored signature
    • A badge is shown Green if core is unchanged (Integrity Check Successful.)
    • Badge is shown Red if changed (integrity check failed. ownCloud core has been modified.)
  • App also includes a signature of the app itself, so that the integrity check is run first against the integrity check app
  • Config file and data directories are excluded from the integrity check
  • Individual apps inside ownCloud each store their own integrity checksum / signature. Any app beyond core is checked against this signature when the check integrity button is pressed, or at upgrade.
  • For each app that is installed (whether enabled or disabled), the checksum is checked and a line is listed: App name – with the integrity check results next to id.
    • A badge is shown Green if core is unchanged (Integrity Check Successful.)
    • Badge is shown Red if changed (integrity check failed. ownCloud core has been modified.)
  • If an app fails, it gets a red light and the check moves onto the next app.
  • Even if the integrity app is disabled, the app is run as part of the upgrade process (unless it has been removed).
  • Before an upgrade, the integrity check is run. If it passes on core, the upgrade is allowed. If it fails on core, the upgrade errors on integrity check “This ownCloud code has been modified from it's original codebase. This can happen for both legitimate and illegitimate reasons, but upgrades with modified code can produce unpredictable results, and even lose data. Please verify before proceeding." Then provide a cancel or continue button. Cancel clearly says upgrade failed.
  • If the code integrity fails on upgrade for a specific app, or apps, then throw a warning "Code Integrity Check has failed on the following ownCloud app(s): list. They will be disabled by the upgrade process as upgrading apps with a failed integrity check can produce unpredictable results."
    then provide a cancel or continue button and disable the apps in question.

Likely there are more cases here, such as what to do after an upgrade is cancelled, because the system is now in a not working state - with the new code, but the old database. the admin then has to replace the new code with old code for example.

@MTRichards MTRichards added this to the 9.0-next milestone Sep 14, 2015

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 15, 2015

Hi,

looks partly like this discussion here: #18309

ghost commented Sep 15, 2015

Hi,

looks partly like this discussion here: #18309

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Sep 15, 2015

Member

As an ownCloud Admin, I want to know if my ownCloud apps and core ownCloud code have been altered so that I can ensure security of the code (hacked, or patched), and better predict conflicts during an upgrade process

You can't ensure the security of the code that way. Not possible as long as config.php is a PHP file which is writable instead of read-only.

Member

LukasReschke commented Sep 15, 2015

As an ownCloud Admin, I want to know if my ownCloud apps and core ownCloud code have been altered so that I can ensure security of the code (hacked, or patched), and better predict conflicts during an upgrade process

You can't ensure the security of the code that way. Not possible as long as config.php is a PHP file which is writable instead of read-only.

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Sep 15, 2015

Member

An app delivers this capability

Please not. This should be part of core. – An app checking the integrity certainly belongs into the core code such as the code checker does etc.

(enterprise only) and can be enabled or disabled by the admin as any other app

Enterprise only? I'd highly advocate against this.

Even if the integrity app is disabled, the app is run as part of the upgrade process (unless it has been removed).

This will lead to problems and code mess. Apps shall not bother that deep with core. Also I don't see any reason why this should be optional. A cancel/continue button as well as a OCC switch for this should be sufficient for any scenario.

Member

LukasReschke commented Sep 15, 2015

An app delivers this capability

Please not. This should be part of core. – An app checking the integrity certainly belongs into the core code such as the code checker does etc.

(enterprise only) and can be enabled or disabled by the admin as any other app

Enterprise only? I'd highly advocate against this.

Even if the integrity app is disabled, the app is run as part of the upgrade process (unless it has been removed).

This will lead to problems and code mess. Apps shall not bother that deep with core. Also I don't see any reason why this should be optional. A cancel/continue button as well as a OCC switch for this should be sufficient for any scenario.

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Sep 15, 2015

Member

Yes. This has to be part of core to work properly.

Member

karlitschek commented Sep 15, 2015

Yes. This has to be part of core to work properly.

@MTRichards

This comment has been minimized.

Show comment
Hide comment
@MTRichards

MTRichards Sep 15, 2015

Contributor

Happy to have ideas @LukasReschke - conceptually do you see what we are trying to do? Make sense?

Contributor

MTRichards commented Sep 15, 2015

Happy to have ideas @LukasReschke - conceptually do you see what we are trying to do? Make sense?

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Sep 15, 2015

Member

conceptually do you see what we are trying to do? Make sense?

Yep :-)

Member

LukasReschke commented Sep 15, 2015

conceptually do you see what we are trying to do? Make sense?

Yep :-)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Sep 25, 2015

Member

What happens if there are apps without the checksum ? (ex: from app store)
Does it block the upgrade with the same warning too ?
Or is that only for shipped apps ? (maybe I mixed this up with app signing on the app store)

Member

PVince81 commented Sep 25, 2015

What happens if there are apps without the checksum ? (ex: from app store)
Does it block the upgrade with the same warning too ?
Or is that only for shipped apps ? (maybe I mixed this up with app signing on the app store)

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Sep 25, 2015

Member

The exact consequences have to be discussed. I think a big red warning with the option to override it is the best. Like it's done in Android

Member

karlitschek commented Sep 25, 2015

The exact consequences have to be discussed. I think a big red warning with the option to override it is the best. Like it's done in Android

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2015

Member

PR is here #20285

Member

PVince81 commented Dec 1, 2015

PR is here #20285

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2015

Member
  • integrity check routines: #20285
  • call it from the upgrade code @VicDeo
  • update build script / release checklist to include code signing at release time
Member

PVince81 commented Dec 1, 2015

  • integrity check routines: #20285
  • call it from the upgrade code @VicDeo
  • update build script / release checklist to include code signing at release time

@PVince81 PVince81 added the p2-high label Dec 11, 2015

@VicDeo

This comment has been minimized.

Show comment
Hide comment
@VicDeo

VicDeo Dec 15, 2015

Member

@LukasReschke what is our policy for unsigned apps?
To be exact for 9.0. And what if integrity is broken - report to user and ask Do you want to proceed on your peril?

I mean upgrade process for sure.

Member

VicDeo commented Dec 15, 2015

@LukasReschke what is our policy for unsigned apps?
To be exact for 9.0. And what if integrity is broken - report to user and ask Do you want to proceed on your peril?

I mean upgrade process for sure.

@jospoortvliet

This comment has been minimized.

Show comment
Hide comment
@jospoortvliet

jospoortvliet Jan 12, 2016

@VicDeo https://doc.owncloud.org/server/9.0/admin_manual/issues/code_signing.html shows what happens on an installed installation.

TL;DR: In general, unsigned apps for experimental or approved apps are just installed, nothing special. Official apps must be signed, that is mandatory.

The admin is warned about Integrity problems with a yellow notification bar on top and a link to the documentation.

In the installer I would indeed report integrity problems, point to the documentation and offer to proceed, noting that we strongly advice against that.

I will make sure the upgrade blog, documentation and forums etc contain information about the code integrity checking and how to deal with problems.

jospoortvliet commented Jan 12, 2016

@VicDeo https://doc.owncloud.org/server/9.0/admin_manual/issues/code_signing.html shows what happens on an installed installation.

TL;DR: In general, unsigned apps for experimental or approved apps are just installed, nothing special. Official apps must be signed, that is mandatory.

The admin is warned about Integrity problems with a yellow notification bar on top and a link to the documentation.

In the installer I would indeed report integrity problems, point to the documentation and offer to proceed, noting that we strongly advice against that.

I will make sure the upgrade blog, documentation and forums etc contain information about the code integrity checking and how to deal with problems.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 15, 2016

Member
  • UX tweaks

@LukasReschke can you fill in the details ? thanks

Member

PVince81 commented Jan 15, 2016

  • UX tweaks

@LukasReschke can you fill in the details ? thanks

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 15, 2016

Member
  • sort out the communication part
Member

PVince81 commented Jan 15, 2016

  • sort out the communication part
@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Jan 22, 2016

Member

@PVince81 @LukasReschke All done - can we close this then?

Member

MorrisJobke commented Jan 22, 2016

@PVince81 @LukasReschke All done - can we close this then?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 22, 2016

Member

Are the communication details sorted out also ?

Member

PVince81 commented Jan 22, 2016

Are the communication details sorted out also ?

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Jan 22, 2016

Member

Are the communication details sorted out also ?

Documentation is done as well, needs some small changes by @jospoortvliet though but that's unrelated.

Member

LukasReschke commented Jan 22, 2016

Are the communication details sorted out also ?

Documentation is done as well, needs some small changes by @jospoortvliet though but that's unrelated.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 10, 2016

Member

Okay, closing then

Member

PVince81 commented Feb 10, 2016

Okay, closing then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment