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

Harden data and config protection .htaccess #16792

Merged
merged 10 commits into from
Dec 19, 2019
Merged

Harden data and config protection .htaccess #16792

merged 10 commits into from
Dec 19, 2019

Conversation

MichaIng
Copy link
Member

@MichaIng MichaIng commented Aug 19, 2019

  • Set "Satisfy All" whenever available, as well on Apache 2.4+. This is required to override possible "Satisfy Any" on parent dir, which otherwise would allow public access, regardless of "Require" directive.
  • Set "Deny from all" as well whenever available, to block access regardless of which access control directive takes priority.
  • Assume Apache 2.2 only, if mod_authz_core and mod_access_compat are both not available, to avoid doubled directives. In this case set "Deny from all" directive only if the providing mod_authz_host module is available. "Satisfy" is a core directive on Apache 2.2.
  • Update Apache version strings. Regarding the used directives/modules, Apache 2.4 and 2.5 (will be released as 2.6) behave the same.
  • Add ordering spaces to better reflect the nested directives and to match style of other .htaccess files.
  • Remove one doubled whitespace + some minor syntax "fixes" to match standards:
    • ifModule => IfModule
    • $content.= "string"; => $content .= "string"; EDIT: Meanwhile done by: cdf8c16

Fixes: #6449

Signed-off-by: MichaIng micha@dietpi.com

+ Set "Satisfy All" whenever available, as well on Apache 2.4+. This is required to override possible "Satisfy Any" on parent dir, which otherwise would allow direct access to data, regardless of "Require" directive.
+ Set "Deny from all" as well whenever available, to block access regardless of which access control directive takes priority.
+ Assume Apache 2.2 only, if mod_authz_core and mod_access_compat are both not available, to avoid doubled directives. In this case set "Deny from all" directive only if the providing mod_authz_host module is available. "Satisfy" is a core directive on Apache 2.2.
+ Update Apache version strings. Regarding the used directives/modules, Apache 2.4 and 2.5 behave the same.
+ Add ordering spaces to better reflect the nested directives and to match style of other .htaccess files.

Fixes: #6449

Signed-off-by: Micha Felle <micha@dietpi.com>
+ Set "Satisfy All" whenever available, as well on Apache 2.4+. This is required to override possible "Satisfy Any" on parent dir, which otherwise would allow direct access to data, regardless of "Require" directive.
+ Set "Deny from all" as well whenever available, to block access regardless of which access control directive takes priority.
+ Assume Apache 2.2 only, if mod_authz_core and mod_access_compat are both not available, to avoid doubled directives. In this case set "Deny from all" directive only if the providing mod_authz_host module is available. "Satisfy" is a core directive on Apache 2.2.
+ Update Apache version strings. Regarding the used directives/modules, Apache 2.4 and 2.5 behave the same.
+ Add ordering spaces to better reflect the nested directives and to match style of other .htaccess files.

Fixes: #6449 (for the config directory)

Signed-off-by: Micha Felle <micha@dietpi.com>
@MichaIng MichaIng changed the title Harden data protection .htaccess Harden data and config protection .htaccess Aug 19, 2019
lib/private/Setup.php Outdated Show resolved Hide resolved
lib/private/Setup.php Outdated Show resolved Hide resolved
lib/private/Setup.php Outdated Show resolved Hide resolved
lib/private/Setup.php Outdated Show resolved Hide resolved
MichaIng and others added 4 commits August 19, 2019 15:29
+ Remove unnecessary spaces from code

Co-Authored-By: Daniel Kesselberg <mail@danielkesselberg.de>
+ Use Apache syntax with cases according to official docs: https://github.com/nextcloud/server/pull/16792/files#r315207691
+ Add missing whitespace for concatenating strings to variable: https://github.com/nextcloud/server/pull/16792/files#r315207520
+ Apache 2.5 will be released as 2.6: https://github.com/nextcloud/server/pull/16792/files#r315206147

Signed-off-by: Micha Felle <micha@dietpi.com>
@MichaIng MichaIng added 3. to review Waiting for reviews security labels Aug 24, 2019
@MichaIng
Copy link
Member Author

MichaIng commented Aug 24, 2019

Actually, to be failsafe, it would make sense to add Order allow,deny in front of Deny from all. This assures that no Allow directive for this dir, declared anywhere else, is evaluated after our Deny rule. Otherwise theoretically some faulty or harmful config snipped can override our rule and open access.

@MichaIng MichaIng requested a review from kesselb August 30, 2019 23:56
@MichaIng
Copy link
Member Author

@nextcloud/security
Hey guys, I hope you find some time to look into this and probably we can still merge it with NC17.

I added the Order Allow,Deny directive as well. This simply makes sense to assure the Deny has always priority. This is as well common across other web applications and their Apache config recommendations and .htaccess files to block certain dir access, e.g. phpBB.

@nickvergessen
Copy link
Member

Well 17 is already packaged. But I guess we can do it for 17.0.1

@nickvergessen nickvergessen added this to the Nextcloud 18 milestone Sep 27, 2019
Signed-off-by: MichaIng <micha@dietpi.com>
@nickvergessen
Copy link
Member

@rullzer @kesselb time to review so we can merge and get this in?

@MichaIng
Copy link
Member Author

MichaIng commented Dec 4, 2019

Resolved conflict caused by format fix, which was also included in this PR: cdf8c16
So changes of this PR are now about the .htaccess only, no surrounding syntax/format.

This was referenced Dec 11, 2019
Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rullzer rullzer mentioned this pull request Dec 19, 2019
18 tasks
@rullzer rullzer merged commit 5d9fd7b into nextcloud:master Dec 19, 2019
@welcome
Copy link

welcome bot commented Dec 19, 2019

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data folder accessible if "Satisfy Any" is set
5 participants