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

[GSoC2024] Added additional security headers #7626

Closed
wants to merge 5 commits into from

Conversation

mach-12
Copy link
Contributor

@mach-12 mach-12 commented Mar 17, 2024

Fixes #7398
Added security headers for Referrer-Policy, X-Content-Type-Options, Content-Security-Policy.

Motivation and context

Referring to Issue #7398, Added additional security headers. Added to address the deduction in security score rating third party scanners.

  1. Referrer-Policy "strict-origin-when-cross-origin";: Limit the referrer information sent when a user navigates away from the website

  2. X-Content-Type-Options "nosniff";: Prevent browsers from attempting to MIME-sniff the content type of a response to reduce risk of XSS and Content Injection

  3. Content-Security-Policy "default-src 'self' http: https: data: blob: 'unsafe-inline'" always;: Set rules to limit what resources a browser can load from out site to reduce the risk of XSS and Data Injection.

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

Added headers for:
X-Frame-Options, Referrer-Policy, X-Content-Type-Options, Content-Security-Policy
Added headers for:
X-Frame-Options, Referrer-Policy, X-Content-Type-Options, Content-Security-Policy
@mach-12 mach-12 requested a review from azhavoro as a code owner March 17, 2024 15:24
Copy link
Contributor

@SpecLad SpecLad left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry. Also, add "Fixes #7398" to the PR description so that GitHub links it to the issue.

Comment on lines +24 to +25
add_header Referrer-Policy "strict-origin-when-cross-origin";
add_header X-Content-Type-Options "nosniff";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
add_header Referrer-Policy "strict-origin-when-cross-origin";
add_header X-Content-Type-Options "nosniff";
add_header Referrer-Policy "strict-origin-when-cross-origin" always;
add_header X-Content-Type-Options "nosniff" always;

Same for the other file.

@@ -21,6 +21,9 @@ server {
add_header Cross-Origin-Embedder-Policy "credentialless";
add_header Expires 0;
add_header X-Frame-Options "deny";
add_header Referrer-Policy "strict-origin-when-cross-origin";
add_header X-Content-Type-Options "nosniff";
add_header Content-Security-Policy "default-src 'self' http: https: data: blob: 'unsafe-inline'" always;
Copy link
Contributor

Choose a reason for hiding this comment

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

I already said this in #7398, but you did not address it: this CSP allows basically everything, so it's pretty useless. Can you come up with a more stringent policy?

@mach-12 mach-12 requested a review from nmanovic as a code owner March 18, 2024 20:21
@mach-12
Copy link
Contributor Author

mach-12 commented Mar 18, 2024

@SpecLad I have added a more specific policy. Can you review what needs to be allowed/disallowed? I'll fix it asap.

@mach-12 mach-12 requested a review from SpecLad March 18, 2024 20:24
@@ -21,6 +21,9 @@ server {
add_header Cross-Origin-Embedder-Policy "credentialless";
add_header Expires 0;
add_header X-Frame-Options "deny";
add_header Referrer-Policy "strict-origin-when-cross-origin";
add_header X-Content-Type-Options "nosniff";
add_header Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; img-src 'self' data: https://app.cvat.ai; font-src 'self' https://fonts.gstatic.com; frame-src 'none'; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'; manifest-src 'self'; worker-src 'self';
Copy link
Contributor

Choose a reason for hiding this comment

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

script-src 'unsafe-inline' nullifies most of the advantages of CSP - are you sure it is needed?

https://app.cvat.ai should not be in there - it's specific to our CVAT instance, and is already covered by 'self'.

Comment on lines +52 to +54
add_header X-Frame-Options "deny";
add_header Referrer-Policy "strict-origin-when-cross-origin";
add_header X-Content-Type-Options "nosniff";
Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to have added duplicate headers.

add_header X-Frame-Options "deny";
add_header Referrer-Policy "strict-origin-when-cross-origin";
add_header X-Content-Type-Options "nosniff";
add_header Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; img-src 'self' data: https://app.cvat.ai; font-src 'self' https://fonts.gstatic.com; frame-src 'none'; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'; manifest-src 'self'; worker-src 'self';
Copy link
Contributor

Choose a reason for hiding this comment

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

This config will likely require a different CSP, because this instance of NGINX hosts the backend, so it should allow resources referenced by pages generated by backend. In particular, you should examine the external resources loaded by pages like:

  • https://<domain>/api/docs/
  • https://<domain>/api/swagger/
  • https://<domain>/api/about/
  • https://<domain>/django-rq/
  • https://<domain>/admin/

@@ -0,0 +1,4 @@
### Security
Copy link
Contributor

Choose a reason for hiding this comment

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

The Security category is for security fixes. This PR is not a fix, it's more of a hardening; so I think this entry belongs in the Added category.

@@ -0,0 +1,4 @@
### Security

- Added security headers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please list which headers were added.

@nmanovic
Copy link
Contributor

@mach-12 , will have you time to finish the PR?

@mach-12
Copy link
Contributor Author

mach-12 commented Mar 25, 2024

Yes @nmanovic it's on the way. I did a security audit and I'm adding the CSP to address the issues. I also found an XSS in the Django admin dashboard.

@SpecLad
Copy link
Contributor

SpecLad commented Apr 1, 2024

Hi,

To avoid delaying this PR, let's limit the scope. Content-Security-Policy is complicated, and fairly risky to introduce, and we shouldn't hold off on adding the other two headers in this PR until we get CSP right. Let's just exclude CSP for now.

@mach-12
Copy link
Contributor Author

mach-12 commented Apr 1, 2024

Hello @SpecLad
I have narrowed these down, please review

Also, there is a potential XSS risk at the admin login panel.

  1. CVAT Assets:

  2. CVAT API:

  3. CVAT Administration:

  4. CVAT Static Resources:

@SpecLad
Copy link
Contributor

SpecLad commented Apr 2, 2024

I have narrowed these down, please review

I don't understand what you want me to review. It's just a list of URLs.

Also, there is a potential XSS risk at the admin login panel.

Can you elaborate?

@SpecLad
Copy link
Contributor

SpecLad commented Apr 11, 2024

Seeing as you haven't replied for over a week, I'm going to close this. If you'd like to go back and finish this, I suggest that you reopen it, and remove the CSP header as I've proposed previously. The other two headers are fairly trivial, so we should be able to get the PR in working order quickly.

@SpecLad SpecLad closed this Apr 11, 2024
@mach-12
Copy link
Contributor Author

mach-12 commented Apr 11, 2024

I have done the proposed changes @SpecLad here #7752

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

Successfully merging this pull request may close these issues.

Security headers not present in responses
3 participants