go-gitea / gitea

Git with a cup of tea! Painless self-hosted all-in-one software development service, including Git hosting, code review, team collaboration, package registry and CI/CD
https://gitea.com
MIT License
43.98k stars 5.4k forks source link

Content-Security-Policy #305

Open bamboleeeero-bamboleeeero opened 7 years ago

bamboleeeero-bamboleeeero commented 7 years ago

There seems to be a bunch of inline scripts or style rules such as this that don't play nice with CSP. These should be replaced by a CSS class.

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/39614247-content-security-policy?utm_campaign=plugin&utm_content=tracker%2F47456670&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F47456670&utm_medium=issues&utm_source=github).
bkcsoft commented 7 years ago

Yes, that should be a class, not inline style :)

Eriner commented 7 years ago

As a note: report-uri is a CSP directive that informs the browser where to report violations. Something to consider is providing/setting this directive and ingesting it into gitea to make it available to administrators.

denji commented 7 years ago

https://github.com/unrolled/secure#content-security-policy

Bwko commented 7 years ago

I'm currently implementing a Content-Security-Policy but we need style-src: unsafe-inline & script-src: unsafe-eval for semantic ui & jquery datetimepicker. @bamboleeeero-bamboleeeero any advice on this?

tboerger commented 7 years ago

We should get rid of all inline crap.

Bwko commented 7 years ago

@tboerger I'm almost done with that (except for labels) but for tooltips semantic ui needs unsafe-inline :confused:

tboerger commented 7 years ago

Even tooltips can be perfectly done with data attributes and unobtrusive js.

hasufell commented 6 years ago

any news?

clarfonthey commented 6 years ago

Personally, I would appreciate at least adding a content-security-policy header to Gitea, slowly making it more strict as changes are made. At least things like form-action and frame-ancestors would be nice, which are doable now.

I'd be willing to work on this within the next few days if someone more-acquainted with the codebase isn't up to it!

toth-dev commented 6 years ago

I'm adding this CSP with nginx:

default-src 'none'; base-uri 'none'; manifest-src 'self'; img-src 'self' data:; font-src 'self' data:; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-eval' 'unsafe-inline'; connect-src 'self'; form-action 'self'

Everything seems to work with this.

hasufell commented 6 years ago

@totpet interesting, I tried that too and it seems to work so far, but afais mozilla observatory reports this as -20 score, so it seems to be too lax.

clarfonthey commented 6 years ago

As I said, things like frame-ancestors and form-action are really important parts of CSP that we could add for existing gitea. Adding unsafe-inline and unsafe-eval to JavaScript are really bad though, and that pretty much negates everything else.

toth-dev commented 6 years ago

My policy broke the embedded pdf viewer, because it is loaded as an iframe, and frame-src falls back to default-src, adding this fixes the problem: frame-src 'self'; frame-ancestors 'self'

(frame-ancestors only needs 'default' inside vendor/plugins/pdfjs/web/, for other locations it can be set to 'none')

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

Oreolek commented 5 years ago

Any news on this? Keep in mind the "Gitea behind a reverse proxy" case where Gitea won't be able to change CSP header dynamically.

noplanman commented 5 years ago

I'm running into this issue too, would be great to get this built-in šŸ‘Œ

MetroWind commented 4 years ago

New user here. Iā€™d also love to see this sorted out. Beside inlines, I think there are also some evals, because I had to add unsafe-eval to my CSP header for that activity matrix thingy in the dashboard page to show up.

hasufell commented 4 years ago

@MetroWind to which header?

dpertin commented 4 years ago

I used the Mozilla Laboratory to generate the following CSP:

default-src 'none'; connect-src 'self'; font-src 'self' data:; form-action 'self'; img-src 'self' https://ga-beacon.appspot.com https://raw.githubusercontent.com https://secure.gravatar.com https://sourcethemes.com; script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; worker-src 'self';

Everything seems to work with nginx, but the Google CSP Evaluator notifies a high-priority issue due to the use of 'unsafe-inline' in script-src. I'd love to see all those inline scripts either externalized, or whitelisted by supporting nonces as proposed here.

SuperSandro2000 commented 4 years ago

img-src 'self' https://ga-beacon.appspot.com https://raw.githubusercontent.com https://secure.gravatar.com https://sourcethemes.com;

This should probably be more like: img-src 'self' https://raw.githubusercontent.com https://secure.gravatar.com;

tomposmiko commented 4 years ago

Can we count on fixing this issue ever? I'm wondering if it will ever happen or we should plan with not fixing it ever.

alexanderadam commented 4 years ago

Can we count on fixing this issue ever? I'm wondering if it will ever happen or we should plan with not fixing it ever.

@tomposmiko most free softwares don't have dedicated developers who are able to make a living by working on them. Meaning, that you can not rely that a feature or a bugfix will be implemented on one point. It is also important that you understand, that this is not specific to gitea, however.

On the plus side, you can usually implement those things by yourself or pay someone to do it. If I'm not mistaken, this issue wasn't important enough for anyone here, to pay anything, however. And it wasn't important enough to anyone to invest their valuable time fixing this issue neither.

Now it should be clear, that it would need clairvoyant abilities to answer your question properly. Because this is not a commercial product that can afford ensuring due dates for features or bugfixes reliably.

SuperSandro2000 commented 4 years ago

that a feature or a bugfix will be implemented on one point.

Not that it is different if your paying for software :laughing:

Crocmagnon commented 3 years ago

Could we at least make gitea serve its own CSP? IMO it's the responsibility of the app to know what it needs to load and from where. Even if we're not going to implement a safe CSP in the first iteration, I'd suggest starting by providing one and then refining it when we clean the inlines and evals.

An even easier first step might be to provide a CSP in the documentation that admins can set on their proxies and after some time implementing it in the code.

ix5 commented 2 years ago

Just as a current (2022, gitea 1.16.1) reference point, I have the following policy implemented and haven't had any (gitea-related) pings on my report-uri:

Content-Security-Policy-Report-Only: "default-src 'self'; connect-src 'self'; font-src 'self' data:; form-action 'self'; img-src 'self' https: data:; object-src 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; worker-src 'self';"

Edit: Amended with below comment (which I can confirm): Content-Security-Policy-Report-Only "default-src 'self'; connect-src 'self'; font-src 'self' data:; form-action 'self'; img-src 'self' https: data:; manifest-src 'self' data:; object-src 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; worker-src 'self';"

Admins who would like to tighten that policy up (esp. img-src https) should be free to do so, but I think this is a good baseline.

tepozoa commented 2 years ago

Generally following the above, I ran into Chrome refusing to load a manifest-src base64 encoded element as it wasn't explicitly allowed to use data: and was inheriting default-src attributes. Adding manifest-src 'self' data:; onto the above solved that Chrome error.

silverwind commented 1 year ago

script-src: unsafe-inline will probably required indefinitely. There are techniques that can only work with inline script like synchronously altering DOM during rendering to avoid content pop-in, so I see it as a requirement for a ideal experience. I made an attempt to remove inline script in https://github.com/go-gitea/gitea/pull/21429, but it was not well received as it's quite a hack.

We should put out a CSP recommendation in the docs as a guidline on how to set up the most strict CSP possible for gitea to work, but I don't see anything else to be done here.

clarfonthey commented 1 year ago

I'm not 100% sure of the details, but surely simply hashing the critical JS would be a solution here? CSP supports this, and it means you'd be able to say that these scripts could be run without anything else being allowed. The hash might change between releases, but I don't imagine that being difficult for folks to deal with if they really care that much over using unsafe-inline.

IMHO the most important aspect of this would be to have similar docs recommending a CSP header for gitea, or having gitea output one itself, even if it does include unsafe-inline. Could this issue be kept open until that is finished?

silverwind commented 1 year ago

Guess we can keep open for docs.

hashing the critical JS

What technique is that? Any more info?

clarfonthey commented 1 year ago

MDN has you covered: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src

Essentially, you can add sha256-(hash) to the list of allowed sources in the CSP header.

silverwind commented 1 year ago

Hmm I guess we could set a static nonce on all our inline scripts and CSP could be recommended to use that nonce. It won't be a hash as it needs to work across upgrades.

jotoho commented 1 year ago

If Gitea emitted the CSP-header itself, it could dynamically calculate the hashes of the inline scripts it sends, couldn't it?

I wouldn't be surprised if browsers or the standardization committees implement additional measures (in the future if they haven't already) to prevent usage of nonces that are used more than once.

tepozoa commented 1 year ago

If you're running other resource paths on your frontend and need CSP policies to accommodate their needs, having Gitea generate a full header could be problematic. I found this issue discussing the spec outlining that multiple headers are not to be sent by a server though, but if they are they are evaluated as individuals and not a combined directive.

Given what I read in the above, a toggle in app.ini to allow an admin to disable Gitea from generating it's own CSP header has to be present, as the Gitea header may conflict with what they're running in their reverse proxy layer.

silverwind commented 1 year ago

Yes, own CSP header is problematic when reverse proxies "Add" instead of "Replace" the header, which I think is the most common configuration. So I think we should just put out a recommendation in docs initially.

Also, I do not see inline scripts as the grave security problem that some sites make it out to be. If everything is first-party, a inline script is equivalent to a url-sourced script in terms of security. I would not recommend running third-party scripts on Gitea anyways.

clarfonthey commented 1 year ago

The main benefit of disallowing inline scripts is to proactively prevent XSS and other vulnerabilities from working even if someone manages to get a script tag in the DOM. I agree that the threat model is probably overprotective in some cases, but it's not just about whether there's an external script or not.

silverwind commented 1 year ago

Reading MDN nonce it does seem something that would be easy to support, just generate a random number per HTML request and add it to all resource tags (inline or not).

If we set CSP ourselves in gitea, it will need extensive configurability so users can include 3rd party resources and the like, e.g. a format in config like {"script-src": ["https://example.com"]} to define additional directives.

nicheosala commented 7 months ago

I'm trying to remove inline JS scripts from Gitea, so that we can avoid script-src: 'unsafe-inline' in CSP.

My main problem is the inline script in templates/base/head_script.tmpl: does anyone have a any idea how to move it to a separate JS file? It is obtained mixing Go templates and Javascript.

I'm not an expert on Go templates and can't find a solution to the above problem. The other inline scripts are not many and they do not mix templates and Javascript. I can list them using VS Code (don't consider the ones that start with <script src, they are fine):

inline_scripts

wxiaoguang commented 7 months ago

I'm trying to remove inline JS scripts from Gitea, so that we can avoid script-src: 'unsafe-inline' in CSP.

My main problem is the inline script in templates/base/head_script.tmpl: does anyone have a any idea how to move it to a separate JS file? It is obtained mixing Go templates and Javascript.

As one of the people who do a lot of Gitea frontend refactoring works, I would say that avoiding inline <script> is not possible at the moment -- at least, very difficult. Actually head_script.tmpl is the easiest one, it could be replaced by some data attribute tags. But, the real blockers are some other <script> blocks which heavily depend on the JS code in page.

nicheosala commented 6 months ago

Thank you @wxiaoguang. I think that any reduction of inline scripts is useful for the security of Gitea, although this is an objective often overlooked in favor of others. Maybe I'm particularly sensitive, since the cybersecurity professor just taught us how to carry out XSS attacks šŸ˜‚

I'll try to study the Gitea code better (and the Go language and its templates) and help out.

silverwind commented 5 months ago

Inline scripts can be exempted from CSP by adding a nonce

Also, I think there is nothing preventing us from serving a 1st-party CSP. Load balancers can always override or extend it.