Skip to content
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

No signature validation for theme apps #29094

Closed
DeepDiver1975 opened this issue Sep 25, 2017 · 48 comments
Closed

No signature validation for theme apps #29094

DeepDiver1975 opened this issue Sep 25, 2017 · 48 comments
Assignees
Labels
blue-ticket enhancement p2-high Escalation, on top of current planning, release blocker
Milestone

Comments

@DeepDiver1975
Copy link
Member

refs owncloud/appstore-issues#105 (comment)

We need a mechanism which allows users to implement their own theme without getting into the signature warning.

Maybe my ignoring all theme apps in the integrity check?

@michaelstingl we have been chatting about this long time ago - right?

@phisch regarding themes

@DeepDiver1975
Copy link
Member Author

or at least allow unsigned hashes - see owncloud/appstore-issues#105 (comment)

@svengit
Copy link

svengit commented Sep 25, 2017

#23186 would probably be sufficient to solve this.

@michaelstingl
Copy link

Currently I only remove signature.json from my theme app and after a rescan I get no more warnings. I think something has been already done about it? /cc @phisch

@svengit
Copy link

svengit commented Sep 26, 2017

This did not work in Owncloud 10.0.2 a few days ago. The integrity checker complained about a missing signature

@michaelstingl
Copy link

With "currently" I meant 10.0.3. Sorry for being not precise.

@phisch
Copy link
Contributor

phisch commented Sep 26, 2017

The thing is, we do want signature checking for apps (themes) from the marketplace, but not for custom themes.

A few months ago we talked about a blacklist in the config.php, I can't find that discussion right now but i think we talked about possible security issues here, which where debunked by the fact that a hacker could un-patch the integrity check eitherway. So adding the possibility to add apps to an integrity blacklist in config.php would be a good option here. That way apps/themes are still checked, but you could disable the check for a custom theme/app you have for your owncloud only.

@DeepDiver1975 @PVince81 any thoughts on this? I would probably go that way if you guys have no concerns about this!

@svengit
Copy link

svengit commented Sep 27, 2017

Yes, signature checking for the marketplace is literally the only thing that makes sense. You could revert the switch from themes/ to the apps/ directory for themes and just limit signature checking to the apps/ dir or even to the moment an app gets installed.
If you want further integrity checking for the whole webdir, unsigned checksums will be sufficient.

@PVince81
Copy link
Contributor

PVince81 commented Oct 9, 2017

@Peter-Prochaska thoughts ?

@peterprochaska
Copy link
Contributor

I like the idea with the blacklist for own apps/themes in config.php. But not for complete folders.

@ownclouders
Copy link
Contributor

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

@BrainStone
Copy link

Any updates on this issue?

@thommierother
Copy link

Yes, I would also like to have the current status. I still see this issue in 10.0.4, when you activate you own theme without a signature.json, as recommended in https://doc.owncloud.org/server/10.0/developer_manual/core/theming.html, you get the complaint about missing signature

@PVince81
Copy link
Contributor

PVince81 commented Jan 8, 2018

Right, we should also add a setting to disable integrity check for specific apps. Currently, only specific files can be excluded in 10.0.4.

@PVince81
Copy link
Contributor

PVince81 commented Jan 8, 2018

@VicDeo I've assigned this to you as you've been working with themes and also know about the integrity check code.

@thommierother
Copy link

as a workaround, how can I create my own signature.json? Same process as for standard apps?

@svengit
Copy link

svengit commented Jan 8, 2018

In general yes, but as far as I know you will not get your key signed if you are not planning on distributing your theme to the market.

@thommierother
Copy link

The exclusion of the signature requirement from themes is the only useful option - that makes sense.

@VicDeo
Copy link
Member

VicDeo commented Jan 9, 2018

@PVince81 @DeepDiver1975 @Peter-Prochaska I'm not feeling like adding yet_another_config_option_to_bypass_something_that should_never_be_bypassed_by_initial_design
What if we just ignore missing signature for the app if it is not shipped and has a type theme.

@thommierother
Copy link

Good point. wouldn't it be easier to just bypass signature check in those cases where info.xml has or Theme ?

@thommierother
Copy link

<namespace>Theme</namespace>

@BrainStone
Copy link

I really liked the idea to add an option to let unsigned plugins pass on a per plugin level. So admins can allow unsigned plugins they trust (like custom plugins). So like adding a checkbox in the app center for unsigned plugins to approve of them would be apprechiated.

@PVince81
Copy link
Contributor

PVince81 commented Jan 9, 2018

... but then a developer could also self-sign the "files" app after hacking it. We might need another mechanism to detect whether the signature of core files (through integrity report) are the ones from the marketplace key or the one from a self-signed cert/key. So more info to add to the report.

@VicDeo
Copy link
Member

VicDeo commented Jan 9, 2018

another option is to be less naggy towards the admin users and allow them to confirm a specific changed signature. If signature was already confirmed the file is still included into the report but no notifications are popped.

@PVince81
Copy link
Contributor

@settermjd for more input

@thommierother
Copy link

Question: is there a temporary solution to disable the warning message in the UI on a productive server? Just as a workaround until the final solution is up ...

@michaelstingl
Copy link

add 'integrity.check.disabled' => true, to your config.php

@thommierother
Copy link

thanks ;-)

@PVince81
Copy link
Contributor

or have a config.php option "ignore-theme-signature => true" and then ignore signature of any theme app. That's probably the best because then the admin can decide whether they setup a local theme or whether they download the theme from the marketplace, in the latter case the signature is important.

@thommierother
Copy link

Either "ignore-theme-signature => true" or ignore-app-signature.[appname] => true" would be fine. The second solution would allow to exclude particular apps from the check, e.g. during development when some code is "official" and other not (unsigned)

@PVince81
Copy link
Contributor

ok fair enough, let's go with something like "ignore-app-signature => ['app1', 'app2']", etc

we will refuse any bug report or support request that excludes official apps this way as it could hide custom modifications that could break ownCloud

@PVince81
Copy link
Contributor

@VicDeo please find a format that fits well with the latest trend for existing key format in config.php ^

@VicDeo
Copy link
Member

VicDeo commented Mar 16, 2018

@PVince81 ignore-app-signature sounds bad ;)
I prefer ignore-missing-app-signature

@PVince81
Copy link
Contributor

@VicDeo so if the app is listed in this config key but has a signature, then the signature will still be checked. So if a signature file is there but there is a violation, the violation will still be reported.
That should work!

@PVince81
Copy link
Contributor

please test with #30891

@thommierother
Copy link

Hm, I tried a test with a zip download of the stable10 branch, it required a make/compose before occ update was possible and now I see many reference errors when trying to login. Sorry, I am doing my first test from unstable code and I am not a fluent php developer... Can you give me a hint what is missing? Any docs to read?

@VicDeo
Copy link
Member

VicDeo commented Mar 27, 2018

@thommierother zipped branch is not proper way to go.
You need just these two files
https://github.com/owncloud/core/raw/632cbba864e2cf4a32366a85fe631dcc8005a084/lib/private/IntegrityCheck/Checker.php
https://github.com/owncloud/core/raw/632cbba864e2cf4a32366a85fe631dcc8005a084/lib/private/IntegrityCheck/Exceptions/MissingSignatureException.php

and the following line in config.php
'integrity.ignore.missing.app.signature' => ['apptheme-name'],

@VicDeo
Copy link
Member

VicDeo commented Mar 27, 2018

alternatively you can try a daily build https://download.owncloud.org/community/owncloud-daily-stable10.zip

@thommierother
Copy link

@VicDeo: you're right, I am stupid ... Test with daily build worked, all fine ;-).

@lock
Copy link

lock bot commented Jul 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blue-ticket enhancement p2-high Escalation, on top of current planning, release blocker
Projects
None yet
Development

No branches or pull requests

10 participants