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

Implementation of HTTP security headers #248

Closed
JaneX8 opened this issue Apr 28, 2020 · 29 comments · Fixed by #545
Closed

Implementation of HTTP security headers #248

JaneX8 opened this issue Apr 28, 2020 · 29 comments · Fixed by #545
Labels
security Pull requests that address a security vulnerability

Comments

@JaneX8
Copy link
Contributor

JaneX8 commented Apr 28, 2020

Please use https://securityheaders.com/ if you want to do some testing yourself and get some feedback. But let's drop in a few thoughts on this:

Content-Security-Policy and Feature-Policy

We should definitely implement this headers as strict as possible to only use the browser features Navidrome needs. Meaning disabling features like webcam, geo, microphone et cetera. This would limit the possibilities extremely when an XSS injection is possible.

X-Frame-Options

This Header we should also include with the "same-origin" option by default. Meaning simply that it isn't possible to frame Navidrome (iframes) and do some clickjacking attacks. Although I can imaging in some cases some advanced users might want to configure and frame it anyway, so I suggest this header should be default on "same-origin" but configurable in Settings.

X-Content-Type-Options

Mainly for older browsers but lets add it with the "nosniff" option. Doesn't hurt anyone. Possibly prevents content type sniffing.

X-XSS-Protection

Yes, should use with "1; mode=block" value. Does not work in all browsers but won't affect browsers that don't support it. Should be enabled in the browser by default in most cases. Just a case of better be safe then sorry when XSS is possible and the browser config sucks.

Referrer-Policy

Only relevant when we start to refer to external URL's, which I believe is not the case currently. Nevertheless I would already set it up with the "no-referrer" or "same-origin".

Other HTTP security headers such as Expect-CT, Expect-Staple, HSTS, HPKP should be left to advanced users to configure themselves, only when SSL is enabled. Maybe we could facilitate adding custom HTTP headers from a config file?

@jvoisin
Copy link
Collaborator

jvoisin commented Apr 28, 2020

The XSS thingy is a bad idea. X-Content-Type-Options is a thing from the past, I doubt that navidrome is usable on antediluvian browsers anyway.

As for the CSP directive, it's a good idea, especially coupled with Trusted Types.

@deluan deluan added the security Pull requests that address a security vulnerability label Apr 28, 2020
@arrrgi
Copy link

arrrgi commented May 3, 2020

Probably one for the documentation or for future reference to anybody who finds this useful.

I did some testing using Traefik as my reverse proxy and found the following security options were suitable for Navidrome:

stsSeconds: 2592000
stsIncludeSubdomains: true
browserXssFilter: true
contentTypeNosniff: true
forceSTSHeader: true
frameDeny: true
referrerPolicy: same-origin
contentSecurityPolicy: |
  default-src https://<redacted domain/subdomain>;
  script-src https://<redacted domain/subdomain> 'unsafe-inline'
featurePolicy: |
  autoplay 'none';
  camera: 'none';
  display-capture 'none';
  microphone: 'none';
  usb: 'none'

This is obviously not my entire Traefik config but the relevant portion I used for my middlewares declarations. These were enough to get an 'A' score from a security headers scan.

This mitigates the need to add them into Navidrome's response headers, but not everyone is going to have a reverse proxy and HTTPS setup.

@deluan
Copy link
Member

deluan commented May 3, 2020

Thanks all! I'm gonna add a security middleware: https://github.com/unrolled/secure
Any suggestions on the configuration we should use?
(of course none of the SSL/STS stuff should be implemented, as we don't support SSL termination on Navidrome itself)

@arrrgi
Copy link

arrrgi commented May 3, 2020

Neat, that's really useful. I saw that Traefik referred to this same handler in their docs

I guess with using that, be mindful that people may use bare domain's or subdomains so the Content-Security-Policy and Feature-Policy will probably need to take that into consideration.

👍

@deluan deluan pinned this issue Aug 24, 2020
@deluan deluan unpinned this issue Aug 24, 2020
@deluan
Copy link
Member

deluan commented Oct 6, 2020

I'm gonna leave this open, to track the work on the CSP implementation

@deluan deluan reopened this Oct 6, 2020
@deluan
Copy link
Member

deluan commented Oct 6, 2020

With the new Feature-Policy in place, I'm seeing these messages in Chome's console:
Screen Shot 2020-10-06 at 11 48 23 AM

Not sure if this is caused by Feature Policy been renamed to Permissions Policy, will have to investigate. If anyone knows anything about it, please chime in.

@Dnouv
Copy link
Contributor

Dnouv commented May 10, 2021

@deluan, I was trying to add the PermissionsPolicy as mentioned on the Secure, but I got the following error,
unknown field PermissionsPolicy in struct literal compiler(MissingLitField)
here is the screenshot of the error,
image

I understand you may be busy, but I would truly appreciate it if you could help me with this issue.
Thank you!

@deluan
Copy link
Member

deluan commented May 10, 2021

@Dnouv Latest release of the Secure package is 1.0.8, which does not implement the PermissionsPolicy header. See unrolled/secure#79

@Dnouv
Copy link
Contributor

Dnouv commented May 10, 2021

Ah, thanks a lot, looks like we have to wait for new release.

@Dnouv
Copy link
Contributor

Dnouv commented May 11, 2021

@deluan, sorry for interrupting again, but how do we test the CSP in the development environment? Could you please help me out. Thank you!

@deluan
Copy link
Member

deluan commented May 11, 2021

Sorry, but it's been a while since I tried adding CSP, and had some issues with it, so I don't actually remember. I think we would need to inject something in the index.html template.

Maybe it is worth to find another project using the secure middleware and see how it is doing it.

@Dnouv
Copy link
Contributor

Dnouv commented May 12, 2021

Thanks a lot, @deluan; I will try to find another project using secure middleware.

@Dnouv
Copy link
Contributor

Dnouv commented May 12, 2021

@deluan, I tried implementing the CSP, but there are inline scripts that will probably require a hash or nonce for proper integration with CSP, which require other packages to be included, as you mentioned here. Here is the screenshot of inline scripts currently in use, thoughts?
image

Feedbacks are most welcomed. Thank you!

@deluan
Copy link
Member

deluan commented May 12, 2021

I think we should not implement CSP in Navidrome. IMHO, it is already safe enough, and a good practice is to put it behind a reverse proxy, which can handle this and other security improvements (ex: SSL).

@jvoisin Thoughts?

@jvoisin
Copy link
Collaborator

jvoisin commented May 12, 2021

I think that the way to go is to wait on react to implement Trusted-type support, and let them deal with the CSP-compatibility, because I'm sure that we really don't want to start monkey-patching things there.

@deluan
Copy link
Member

deluan commented May 12, 2021

Are they (React) already working on it? Do they have an open issue to track it? Should we keep this open to track the progress, or should we close this one and open another when they implement it?

@Dnouv
Copy link
Contributor

Dnouv commented May 12, 2021

There was once an issue regarding this facebook/create-react-app#5288; they mentioned setting INLINE_RUNTIME_CHUNK=false, in .env to opt-out of the inline script. Although I did not quite understand how to implement this and what was .env

@deluan
Copy link
Member

deluan commented May 12, 2021

@Dnouv .env is a way to configure your Node/JS app with environment variables.

What they are saying is that to remove inline scripts you just have to set INLINE_RUNTIME_CHUNK=false before calling npm run build. Then the generated index.html will transform the <script> tag into a <script src=""> one, making it easier to implement CSP.

Either way, as I said above, I don't think this is relevant at the moment. Let's wait for Trusted-types in React.

@Dnouv
Copy link
Contributor

Dnouv commented May 13, 2021

Thanks a lot, @deluan; I really appreciate it. For CSP implementation, it looks like we need to wait.

@Hukuma1
Copy link

Hukuma1 commented Sep 2, 2021

Am I in the right place?

I am trying to iframe Navidrome into Home Assistant (since there is no native Home Assistant integration). Using Home Assistant's normal iframe_panel method: https://www.home-assistant.io/integrations/panel_iframe/

I don't even make it to the username/password screen of Navidrome. :/ Blocked by header immediately?

Refused to display 'http://192.168.1.4:4533/' in a frame because it set 'X-Frame-Options' to 'deny'.

Any way to edit this to allow? This is just one local IP trying to connect to another local IP in this instance. HTTP to HTTP.

Edit: Adding proxy_hide_header X-Frame-Options; into my Nginx-Proxy-Manager for Navidrome right now as a workaround.

@deluan
Copy link
Member

deluan commented Sep 3, 2021

@Hukuma1 , this is disabled as a security measure. The way to override it is what you already did: in your Nginx configuration

@bilogic
Copy link

bilogic commented Oct 28, 2023

@deluan

able to make this configurable so that we can use X-Frame-Options: SAMEORIGIN?

@bilogic
Copy link

bilogic commented Nov 10, 2023

Possible? I'm also having the same issue as #1883 when using it with Home Assistant thanks

@eric-saintetienne
Copy link

eric-saintetienne commented Apr 26, 2024

@deluan

able to make this configurable so that we can use X-Frame-Options: SAMEORIGIN?

To your point, I wish there was an environment variable that would allow to set the level of X-Frame-Options directly from a Dockerfile.

@deluan
Copy link
Member

deluan commented May 2, 2024

@eric-saintetienne / @bilogic: Can't you just map this in your reverse proxy configuration?

@bilogic
Copy link

bilogic commented May 2, 2024

@deluan it has been sometime and I can't recall my exact findings from the debugging, but I think there is no reverse proxy in HomeAssistant.

@eric-saintetienne
Copy link

@eric-saintetienne / @bilogic: Can't you just map this in your reverse proxy configuration?

I'm not using a reverse proxy, I'm using docker to expose the port and use it from other hosts.

@deluan
Copy link
Member

deluan commented May 2, 2024

Ok, I made it configurable.

In navidrome.toml:

HTTPSecurityHeaders.CustomFrameOptionsValue = "SAMEORIGIN"

As env var:

ND_HTTPSECURITYHEADERS_CUSTOMFRAMEOPTIONSVALUE=SAMEORIGIN

That's the only option introduced for now. More may be added in the future.

@deluan
Copy link
Member

deluan commented May 3, 2024

Closing this now, as I think all items in the original issue were addressed.

We may revisit this to leverage Trusted Types in React, but it is a different issue.

If you have any concerns, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants