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

Fix security header setting in .htaccess by adding 'onsuccess unset' #19002

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

zertrin
Copy link
Member

@zertrin zertrin commented Jan 19, 2020

The headers might already be set by the system administrator at the http server
level (apache or nginx) for some or all virtualhosts.

Using always set in the .htaccess of Nextcloud leads to the situation where
the headers might be set twice (once in the default onsuccess table and once
in the always table)! Which leads to warnings in the admin area.

Adding onsuccess unset solves the problem, and forces the header in
the onsucess table to be unset, and the header in the always table to be set.

NOTE: with this change, Nextcloud overrides whatever the system administrator
might have already set

See github issues #16893 #16476 #16938 #18017

@MichaIng
Copy link
Member

@zertrin
That is great, in two ways:

  • I don't like the "always" option, which leads to headers being send on failing requests and redirects as well, which is IMO nothing but an unnecessary overhead outside of debug/testing scenarios. The only answer/"reason" I got when addressing this in the other issue (where "always" was added) was "Why should they not being send on every request?", why I would always ask this the other way round: "Why should they?"
    • But note that for consistency this should be adjusted in docs Nginx config then as well. Not sure if Nginx has some "ifempty" option, but "always" should be removed then.
  • With "ifempty" admins are able to change the headers the need to edit .htaccess, which would be reverted on updates and leads to integrity check errors. Also for debug/testing the "always" flag can then be set manually in Apache config without having doubled headers.
    • The admin panel still prints the warning about recommended headers, if those were set weaker.

@J0WI
Copy link
Contributor

J0WI commented Jan 20, 2020

The goal in the previous change was to make manipulations harder. I think this is okay for production.
The always ensures that the security header works even if you're able to to crash something in the PHP app. This might be an edge case, but 3xx and 4xx responses are common. always setifempty can be used though.

Not sure if Nginx has some "ifempty" option

There is no comparable option in nginx.

With "ifempty" admins are able to change the headers

Where/How would you like to change them?

The admin panel still prints the warning about recommended headers, if those were set weaker.

It just checks a 2xx response form PHP.

@MichaIng
Copy link
Member

@J0WI
Is there any data transferred besides connection meta info together with non-2XX responses, that would render the headers useful?

Some admins might want to frame pages from their instance on remote sites e.g. or similar. Of course edge cases as well.

Okay I thought the admin panel checks headers via regular request to CLI URL, hence check headers finally sent from webserver.

@zertrin
Copy link
Member Author

zertrin commented Jan 20, 2020

The goal in the previous change was to make manipulations harder. I think this is okay for production.
The always ensures that the security header works even if you're able to to crash something in the PHP app. This might be an edge case, but 3xx and 4xx responses are common. always setifempty can be used though.

The .htaccess doesn't depend on PHP. I don't see what is the argument here.

With "ifempty" admins are able to change the headers

Where/How would you like to change them?

The Nextcloud admin might not have control on the HTTP server settings (managed by another sysadmin with root access to the server). It is not so uncommon to set security headers in a global config file which applies to all VHosts. (http server hardening config)

In any case, if a server admin already has set security headers, there is no point in Nextcloud duplicating them in the response.

So always setifempty is ok. Do you want me to update the PR?

@zertrin
Copy link
Member Author

zertrin commented Jan 20, 2020

The .htaccess doesn't depend on PHP. I don't see what is the argument here.

Ah sorry, I just re-read the apache documentation.

The optional condition argument determines which internal table of responses headers this directive will operate against: onsuccess (default, can be omitted) or always. The difference between the two lists is that the headers contained in the latter are added to the response even on error, and persisted across internal redirects (for example, ErrorDocument handlers).

I see the difference. Without always, it is only sent on 2xx HTTP responses.

@zertrin
Copy link
Member Author

zertrin commented Jan 20, 2020

I have updated the PR accordingly 😉

@zertrin zertrin changed the title Use "setifempty" instead of "always set" in .htaccess Use "setifempty" instead of "set" in .htaccess Jan 20, 2020
Copy link
Contributor

@J0WI J0WI left a comment

Choose a reason for hiding this comment

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

Do we require compatibility with Apache 2.2?

@zertrin
Copy link
Member Author

zertrin commented Jan 20, 2020

Do we require compatibility with Apache 2.2?

For information, according to https://httpd.apache.org/ Apache 2.2 is EOL since 2 years.

Apache httpd 2.2 End-of-Life 2018-01-01¶

As previously announced, the Apache HTTP Server Project has discontinued all development and patch review of the 2.2.x series of releases.

The Apache HTTP Server Project had long committed to provide maintenance releases of the 2.2.x flavor through June of 2017. The final release 2.2.34 was published in July 2017, and no further evaluation of bug reports or security risks will be considered or published for 2.2.x releases.

@MichaIng
Copy link
Member

MichaIng commented Jan 20, 2020

Ah actually that on shared hostings admins have no control of server-wide headers actually IS and argument to allow overriding them (e.g. if set weaker) with .htaccess, I didn't think about this. Headers on the same table (always and non-always) override each others and are not set doubled. Doubled headers are AFAIK only present if the same header is once set without "always" and once with "always" on Apache, which then leads to warning in admin panel and headers shown twice on e.g. curl -D responses.

About Apache 2.2, currently the .htaccess files that prevent access to config and data dir contain 2.2-specific parts.
It could make sense to drop support for this EOL version, which would simplify things, but this must be communicated prominently first.

@zertrin
Copy link
Member Author

zertrin commented Jan 21, 2020

Well then if you really want nextcloud to have the last word about the headers, you need to do:

Header onsuccess unset Referrer-Policy
Header always set Referrer-Policy "no-referrer"

for each header, both in the .htaccess and in PHP. This guarantees that the one set in the onsuccess table is removed, and the one set in the always table is added (or replaced if already set).

If that's what Nextcloud prefer, then the server admin will not be able to override the value anymore. We have to choose one way or another anyway: either we only set things if not already set XOR we override everything always.

I can update the PR accordingly if that's what wanted by nextcloud core devs.

(PS: this two tables shenanigan is really confusing if one doesn't read the apache doc in details...)

@MichaIng
Copy link
Member

MichaIng commented Jan 21, 2020

@zertrin
Jep you're right, unsetting the onsuccess headers would be consequent then, btw in every case. Regardless if "always" table is used or not, if doubled headers are to be avoided at any cost, the other type must be always unset. Same with "setifempty", which does not prevent doubled headers, when the same header is set on the other table already.

And yes, the Apache two-table solution for headers is not only confusing but simply ugly. It does not make any sense to allow sending the same header from two different tables. This raises effort and even creates an impossibility: How can you assure that a specific header is sent exactly once, while preserving the existing one from any table. I assume that setifempty only checks the table it would assign to, but not both. 100% clean would be to check both tables for the specific header and only set a header if both tables are empty.
However, I assume this is deeply buried in Apache2 code and hard or impossible to change without a massive rework 😉.

zertrin added a commit to zertrin/nextcloud-server that referenced this pull request Jan 28, 2020
The headers might already be set by the system administrator at the http server
level (apache or nginx) for some or all virtualhosts.

Using "always set" in the .htaccess of Nextcloud leads to the situation where
the headers might be set twice (once in the default 'onsuccess' table and once
in the 'always' table)! Which leads to warnings in the admin area.

Adding "onsuccess unset" solves the problem, and forces the header in
the 'onsucess' table to be unset, and the header in the 'always' table to be set.

NOTE: with this change, Nextcloud overrides whatever the system administrator
might have already set

See github issues nextcloud#16893 nextcloud#16476 nextcloud#16938 nextcloud#18017 and discussion in PR nextcloud#19002

Signed-off-by: zertrin <zertrin@gmail.com>
@zertrin zertrin force-pushed the patch-1 branch 2 times, most recently from 9167f02 to 17e6485 Compare January 28, 2020 09:28
zertrin added a commit to zertrin/nextcloud-server that referenced this pull request Jan 28, 2020
The headers might already be set by the system administrator at the http server
level (apache or nginx) for some or all virtualhosts.

Using "always set" in the .htaccess of Nextcloud leads to the situation where
the headers might be set twice (once in the default 'onsuccess' table and once
in the 'always' table)! Which leads to warnings in the admin area.

Adding "onsuccess unset" solves the problem, and forces the header in
the 'onsucess' table to be unset, and the header in the 'always' table to be set.

NOTE: with this change, Nextcloud overrides whatever the system administrator
might have already set

See github issues nextcloud#16893 nextcloud#16476 nextcloud#16938 nextcloud#18017 and discussion in PR nextcloud#19002

Signed-off-by: zertrin <zertrin@gmail.com>
@zertrin zertrin changed the title Use "setifempty" instead of "set" in .htaccess Fix security header setting in .htaccess by adding 'onsuccess unset' Jan 28, 2020
@zertrin
Copy link
Member Author

zertrin commented Jan 28, 2020

Sorry for the delay. I have updated the PR title, description and patch according to the latest discussion.

Please review!

@MichaIng
Copy link
Member

It looks like too much/overhead, but AFAIK the only way to guarantee that all wanted headers are set exactly once on every kind response.

@zertrin
Copy link
Member Author

zertrin commented Feb 10, 2020

How to get this moving further? (get tags and a milestone maybe?)

@MichaIng MichaIng added the 3. to review Waiting for reviews label Feb 10, 2020
@MichaIng
Copy link
Member

MichaIng commented Mar 4, 2020

@zertrin
Uh, my review seems to count now for approval. Please do not yet merge, if possible, as I think another review of someone with some Apache + performance knowledge would be great. However DCO is unhappy anyway since you didn't sign off the last commit 😅.

Generally the changes do what we want or need to prevent doubled headers. However since .htaccess is parsed on every request I am not sure how much this amount of additional directives affect request speed. Most likely it is not measurable compared to all the PHP scripts that are called...

Another wish might be to reduce the amount of additional lines, e.g. strip the empty lines and reduce the comment to a bare minimum, e.g.:

# Avoid doubled headers by unsetting headers in "onsuccess" table,
# then add headers to "always" table: https://github.com/nextcloud/server/pull/19002

When someone takes care request performance on Apache, then it would be better anyway to disable .htaccess all together and instead add those directives to the Apache server/vhost config, as it's done with Nginx. I did this for my home server and it works pretty well with all redirects and permissions, even allows to drop some non-required ones when having Nextcloud in a subdir of webroot and/or data dir outside of webroot 😉.

zertrin added a commit to zertrin/nextcloud-server that referenced this pull request Mar 5, 2020
The headers might already be set by the system administrator at the http server
level (apache or nginx) for some or all virtualhosts.

Using "always set" in the .htaccess of Nextcloud leads to the situation where
the headers might be set twice (once in the default 'onsuccess' table and once
in the 'always' table)! Which leads to warnings in the admin area.

Adding "onsuccess unset" solves the problem, and forces the header in
the 'onsucess' table to be unset, and the header in the 'always' table to be set.

NOTE: with this change, Nextcloud overrides whatever the system administrator
might have already set

See github issues nextcloud#16893 nextcloud#16476 nextcloud#16938 nextcloud#18017 and discussion in PR nextcloud#19002

Signed-off-by: zertrin <zertrin@gmail.com>
@zertrin
Copy link
Member Author

zertrin commented Mar 5, 2020

@zertrin
Uh, my review seems to count now for approval. Please do not yet merge, if possible, as I think another review of someone with some Apache + performance knowledge would be great. However DCO is unhappy anyway since you didn't sign off the last commit 😅.

Ah sorry forgot about the DCO.

I took the opportunity to squash and rebase on top of current master.

EDIT: and also included your suggestion about the reducing the size of the comment

The headers might already be set by the system administrator at the http server
level (apache or nginx) for some or all virtualhosts.

Using "always set" in the .htaccess of Nextcloud leads to the situation where
the headers might be set twice (once in the default 'onsuccess' table and once
in the 'always' table)! Which leads to warnings in the admin area.

Adding "onsuccess unset" solves the problem, and forces the header in
the 'onsucess' table to be unset, and the header in the 'always' table to be set.

NOTE: with this change, Nextcloud overrides whatever the system administrator
might have already set

See github issues nextcloud#16893 nextcloud#16476 nextcloud#16938 nextcloud#18017 and discussion in PR nextcloud#19002

Signed-off-by: zertrin <zertrin@gmail.com>
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

reasonable 👍

@blizzz blizzz merged commit 797fa18 into nextcloud:master Apr 24, 2020
@welcome
Copy link

welcome bot commented Apr 24, 2020

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

@zertrin
Copy link
Member Author

zertrin commented Apr 25, 2020

I've been wondering, would this kind of fix be considered for a stable backport?

And if yes, what is the proper way to propose and do this?

@kesselb
Copy link
Contributor

kesselb commented Apr 25, 2020

/backport to stable18

backportbot-nextcloud bot pushed a commit that referenced this pull request Apr 25, 2020
The headers might already be set by the system administrator at the http server
level (apache or nginx) for some or all virtualhosts.

Using "always set" in the .htaccess of Nextcloud leads to the situation where
the headers might be set twice (once in the default 'onsuccess' table and once
in the 'always' table)! Which leads to warnings in the admin area.

Adding "onsuccess unset" solves the problem, and forces the header in
the 'onsucess' table to be unset, and the header in the 'always' table to be set.

NOTE: with this change, Nextcloud overrides whatever the system administrator
might have already set

See github issues #16893 #16476 #16938 #18017 and discussion in PR #19002

Signed-off-by: zertrin <zertrin@gmail.com>
@backportbot-nextcloud
Copy link

backport to stable18 in #20647

@zertrin zertrin deleted the patch-1 branch June 26, 2020 03:54
f4z4on added a commit to budoucnost/web that referenced this pull request Jul 28, 2020
This is inspired by Nextcloud’s approach.† We want to prevent duplicate
headers because Apache HTTP Server basically keeps two independent
tables with headers: onsuccess and always. We have to use “always” for
HSTS and no-sniff headers because without it, mod_rewrite ignores the
header. But there is a possibility that something else sets the same
header in “onsuccess” table.

We do not have to care about other headers because we do not use
“always” table for them. Firstly, it’s probably OK to set them only for
successful requests. Secondly, “always” table is also used to override
headers set by scripts which we do not have which is why we are not
using it.

† nextcloud/server#19002
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 enhancement
Projects
None yet
6 participants