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

Add hint about duplicated headers #674

Closed
wants to merge 2 commits into from
Closed

Conversation

J0WI
Copy link
Contributor

@J0WI J0WI commented Feb 9, 2018

See issue described in nextcloud/server#8207

cc @MorrisJobke

@tflidd
Copy link
Contributor

tflidd commented Feb 9, 2018

If you use the default settings for nginx from the docs, it would make also sense to put this line in the default configuration examples??

@J0WI
Copy link
Contributor Author

J0WI commented Feb 10, 2018

Well... that doesn't make things any clearer.

@J0WI
Copy link
Contributor Author

J0WI commented Mar 30, 2018

@MorrisJobke would be great to get some feedback here.

@@ -71,9 +69,14 @@ webroot of your nginx installation. In this example it is
add_header X-Robots-Tag none;
add_header X-Download-Options noopen;
add_header X-Permitted-Cross-Domain-Policies none;
fastcgi_hide_header X-Content-Type-Options;
fastcgi_hide_header X-XSS-Protection;
Copy link
Member

Choose a reason for hiding this comment

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

We should not remove them by default, because then it could be that those are not set by the web server as well and causing a security problem.


.. code-block:: nginx

fastcgi_hide_header X-XSS-Protection;
Copy link
Member

Choose a reason for hiding this comment

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

I like this separate section, that we maybe also could link from the admin settings, so that admins directly know where it is coming from and how to fix it in their environment.

@MorrisJobke
Copy link
Member

cc @josh4trunks for the general Nginx explanation.

@josh4trunks
Copy link
Contributor

Sorry, I am not 100% familiar with these security headers. Is my understanding below correct?

  • PHP adds all the necessary security headers, except for HSTS
  • We need security headers for files served by PHP and the CSS/JS block served by NGINX
  • In NGINX, any location we add additional headers, like HSTS or Cache-Control, needs all its headers added again
  • Anytime we add headers with NGINX we will also want to unset the headers from PHP to avoid duplicates

If all the above is true, then this PR makes sense to me and looks correct.

Personally, I don't add any headers with NGINX except Cache-Control. But, I do add HSTS with an upstream Varnish server.
So my JS/CSS files don't have any security headers. Anyone have a link to explain what my vulnerability is? Thanks,

@MorrisJobke
Copy link
Member

PHP adds all the necessary security headers, except for HSTS
We need security headers for files served by PHP and the CSS/JS block served by NGINX
In NGINX, any location we add additional headers, like HSTS or Cache-Control, needs all its headers added again
Anytime we add headers with NGINX we will also want to unset the headers from PHP to avoid duplicates

Those are correct, except that the security headers only need to be added to the served HTML files and not the JS/CSS. Those just define what JS is allowed to be loaded and executed within this HTML.

The problem with the config is, that some distros adds some headers automatically and thus the ones from PHP are then duplicating those headers. So it should only be added in distros that add those headers already and not on all installs, because then it would lack those headers on vanilla distros, that don't add anything by default.

@josh4trunks
Copy link
Contributor

@MorrisJobke in this PR, the five headers being removed by fastcgi_hide_header are being added just before it so I do not see how these would not exist if all 10 of these lines are in a location in their config?

    add_header X-Content-Type-Options nosniff;
    add_header X-XSS-Protection "1; mode=block";
    add_header X-Robots-Tag none;
    add_header X-Download-Options noopen;
    add_header X-Permitted-Cross-Domain-Policies none;
    fastcgi_hide_header X-Content-Type-Options;
    fastcgi_hide_header X-XSS-Protection;
    fastcgi_hide_header X-Robots-Tag;
    fastcgi_hide_header X-Download-Options;
    fastcgi_hide_header X-Permitted-Cross-Domain-Policies;

I do not understand your point how a distro can have any affect on what headers NGINX sends in this case. Once add_header has been used, any headers the distro tried adding in a previous location or with an include is no longer going to be used. The headers that will be sent have been reset for this and any descending locations.

@josh4trunks
Copy link
Contributor

I wonder if it is possible to do this to make sure anything stripped from PHP response is already added.

    add_header X-Content-Type-Options nosniff; fastcgi_hide_header X-Content-Type-Options;
    add_header X-XSS-Protection "1; mode=block"; fastcgi_hide_header X-XSS-Protection;
    add_header X-Robots-Tag none; fastcgi_hide_header X-Robots-Tag;
    add_header X-Download-Options noopen; fastcgi_hide_header X-Download-Options;
    add_header X-Permitted-Cross-Domain-Policies none; fastcgi_hide_header X-Permitted-Cross-Domain-Policies;

@MorrisJobke
Copy link
Member

I do not understand your point how a distro can have any affect on what headers NGINX sends in this case. Once add_header has been used, any headers the distro tried adding in a previous location or with an include is no longer going to be used. The headers that will be sent have been reset for this and any descending locations.

Didn't knew that, but we had a lot of people complaining, that our checks for the headers fail and then they looked it up in the response and the header was there twice. :/

@josh4trunks
Copy link
Contributor

josh4trunks commented Apr 4, 2018

This PR should fix that (thought I have not had a chance to test it myself).

Or they can be like me and just not add any header in NGINX (except Cache-Control), just let PHP handle it, and i have none of those errors

@MorrisJobke
Copy link
Member

Or they can be like me and just not add any header in NGINX (except Cache-Control), just let PHP handle it, and i have none of those errors

We told this the people a lot of times and also linked directly to that ticket. Nevertheless it pops up every second week :/

@J0WI
Copy link
Contributor Author

J0WI commented Apr 4, 2018

Setting the header on nginx makes sense because of nextcloud/server#8207 (comment) and you may want to customize (being more restrictive) the headers (nextcloud/server#8207 (comment)).

@francoism90
Copy link

francoism90 commented Apr 28, 2018

What's the status of this? Because I'm facing the same issue.

I don't understand why the same headers are being set on location ~ \.(?:css|js|woff|svg|gif)$ { ... This will duplicate the headers and should not be needed, because the headers are already set in the server block.

@josh4trunks
Copy link
Contributor

@francoism90 This does not acutally change NExtcloud's code so you are free to read the resultant docs and use that to configure NGINX.

To understand why headers are set in multiple location is covered in my 3rd point here.
#674 (comment)

@skjnldsv
Copy link
Member

skjnldsv commented Dec 7, 2018

Any news here?

@J0WI
Copy link
Contributor Author

J0WI commented Jul 1, 2019

Closing in favor of #1520 #1521 #1522

@J0WI J0WI closed this Jul 1, 2019
@J0WI J0WI deleted the patch-1 branch July 1, 2019 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants