Initial draft of headers #483

M4LuZ · 2022-05-06T22:31:35Z

Includes settings for cookies

fixes #179

Includes settings for cookies fixes #179

M4LuZ · 2022-12-29T13:30:37Z

@lansuite/lansuite-developers : It would be good you give the proposal a try.
Should be fine, but would appreciate your feedback

sonarcloud · 2023-07-05T10:58:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

sonarcloud · 2023-08-08T07:04:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

sonarcloud · 2023-10-14T18:56:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

sonarcloud · 2023-12-01T13:01:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

M4LuZ · 2024-01-09T21:56:18Z

(removing from milestone, as not critical and no feedback received so far)

andygrunwald · 2024-02-18T11:53:52Z

I am planning to review, check and test this one soon.

M4LuZ · 2024-02-22T09:40:14Z

I think some additional work needs to be done on this, as current values prevent external inclusions that would be needed for gmaps, analytics and I think also discord.
But I guess that should be doable by providing the option to modify settings from standard values when needed (potentially by changing related code to use HttpFoundation?)

andygrunwald · 2024-02-22T19:54:11Z

I think some additional work needs to be done on this, as current values prevent external inclusions that would be needed for gmaps, analytics and I think also discord.

I am sure there is a right set of attributes to allow / whitelist external includes. Let me check a bit about this.

But I guess that should be doable by providing the option to modify settings from standard values when needed (potentially by changing related code to use HttpFoundation?)

I may not understand how HttpFoundation helps here.

EDIT: I got the reference with HttpFoundation now.

andygrunwald

I had a deeper look and this might need a bit of more work. See my code comments.

andygrunwald · 2024-02-22T20:00:53Z

ext_scripts/captcha.php

@@ -9,7 +9,7 @@ function imageAuthCode($width, $height) {
  $auth_code = md5($auth_code);
  $auth_code = substr($auth_code, 0, 5);
  $auth_code = strtoupper($auth_code);
-  setcookie('image_auth_code', md5($auth_code), time()+60*60 , '/');
+  setcookie('image_auth_code', md5($auth_code), time()+60*60 , '/',"",$_SERVER['HTTPS'] == 'on' ? true : false, true);


For the HTTPS detection, we should use isSecure() by HttpFoundation, see https://github.com/symfony/http-foundation/blob/439fdfdd344943254b1ef6278613e79040548045/Request.php#L1043

It respects also proxies and loadbalancers.
I know, this might be tricky in the captcha here, but maybe we should switch to another captcha lib in this phase.

andygrunwald · 2024-02-22T20:01:11Z

inc/Classes/Auth.php

@@ -715,7 +715,7 @@ private function cookie_set()
        setcookie(
            $this->cookie_name,
            $this->cookiedata_pack(),
-            ['expires' => time()+3600*24*$this->cookie_time, 'path' => $this->cookie_path, 'domain' => $this->cookie_domain]
+            ['expires' => time()+3600*24*$this->cookie_time, 'path' => $this->cookie_path, 'domain' => $this->cookie_domain, 'secure' => $_SERVER['HTTPS'] == 'on' ? true : false, 'httponly' => true]


See HTTPS detection comment above - https://github.com/symfony/http-foundation/blob/439fdfdd344943254b1ef6278613e79040548045/Request.php#L1043

andygrunwald · 2024-02-22T20:01:44Z

index.php

+header('Referrer-Policy: no-referrer');
+header("'Content-Security-Policy: default-src 'self'; script-src 'self'; img-src 'self'; style-src 'self'; font-src 'self'; object-src 'none'; frame-src 'self'; worker-src 'self'; connect-src 'self';");
+// Enforce HSTS if browsing via HTTPS
+if ($_SERVER['HTTPS']=='on') {


Same comment on HTTPS detection - https://github.com/symfony/http-foundation/blob/439fdfdd344943254b1ef6278613e79040548045/Request.php#L1043

index.php

andygrunwald · 2024-02-22T20:07:43Z

index.php

@@ -114,6 +114,15 @@

 // Set HTTP-Headers
 header('Content-Type: text/html; charset=utf-8');
+header('X-Frame-Options: sameorigin');
+header('X-XSS-Protection: 1; mode=block');
+header('X-Content-Type-Options: nosniff');


The idea of this header is great and I like the idea. However, according to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Content-Type-Options it says

The X-Content-Type-Options response HTTP header is a marker used by the server to indicate that the MIME types advertised in the Content-Type headers should be followed and not be changed.

I am not aware that we are sending proper mime types in every request. However, I see this as a pre-requisite to make use of this header (and keep this system working).

My suggestion: Commenting this header out for a later use once we got more confidence sending the right mime types

I have more confidence that the right mime types are being sent, as a mapping is done by the webserver anyways based on file extension unless specifically told not do do so by configuration or overridden.
LS does set MIME-type specifically for file downloads e.g. in the gallery (see modules/picgallery/download.php)

So I see no issue with setting that (and also was not able to find one in superficial tests), but fine to leave it out for now

Suggested change

header('X-Content-Type-Options: nosniff');

// TODO: This header is still useful - Once we verified to send the correct MIME types, enable this header

// header('X-Content-Type-Options: nosniff');

index.php

andygrunwald · 2024-02-22T20:18:41Z

index.php

+header('X-XSS-Protection: 1; mode=block');
+header('X-Content-Type-Options: nosniff');
+header('Referrer-Policy: no-referrer');
+header("'Content-Security-Policy: default-src 'self'; script-src 'self'; img-src 'self'; style-src 'self'; font-src 'self'; object-src 'none'; frame-src 'self'; worker-src 'self'; connect-src 'self';");


This might be tricky with the external libraries we include. We may need to specify domains on this one, see https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#example_3

I am not sure if this works.

What do you think about starting learn and only going with default-src 'self' at first?

M4LuZ · 2024-02-22T22:17:33Z

EDIT: I got the reference with HttpFoundation now.

That is a yes if you meant the response object and the related options / functions to manipulate headers in the response
https://symfony.com/doc/current/components/http_foundation.html#response
If you mean the reference to $request->isSecure() then that is a good find that I was not aware of, but not what I meant ;)

I'm in doubt if that is an easy implementation given the output buffer management done via LS and Smarty, so we may be better to use native functions or just a adequate wrapper for our management for now.

andygrunwald · 2024-02-23T07:10:13Z

If you mean the reference to $request->isSecure() then that is a good find that I was not aware of, but not what I meant ;)

Yes, I mean using the isSecure method to check if the request is HTTPS or not.

That is a yes if you meant the response object and the related options / functions to manipulate headers in the response
https://symfony.com/doc/current/components/http_foundation.html#response

Oh, we should use the lib to send the respective response headers. The challenge here: We don't use the response object at all right now.

I'm in doubt if that is an easy implementation given the output buffer management done via LS and Smarty, so we may be better to use native functions or just a adequate wrapper for our management for now.

I am in line with you. My suggestion: Merging these headers once we clarified the details. Migrating to the response object at some later point in time. This might be a bigger change.

Co-authored-by: Andy Grunwald <andygrunwald@gmail.com>

M4LuZ · 2024-02-23T21:48:32Z

I am in line with you. My suggestion: Merging these headers once we clarified the details. Migrating to the response object at some later point in time. This might be a bigger change.

Actually...
It appears that we can benefit from httpFoundation nevertheless and now by:

creating (yet another) global object. This time of ResponseHeaderBag (this is an object that is used within the httpFoundation response to keep track of headers and cookies).
Instantiate all headers that we want to set with standard values in index.php
have modules modify these as/if needed (we may need to handle Content-Security-Policy differently, as that may need to have multiple addittions over multiple calls)
before we start output we have just to call ->all() to get all headers defined and then iterate and set them all (or potentially use a derived class where we put that in a function?)

sonarcloud · 2024-02-24T09:03:51Z

Quality Gate passed

Issues
2 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

andygrunwald · 2024-02-24T09:32:35Z

@M4LuZ Your suggestion sounds reasonable. However, I am more in favor of

Getting this first implementation done
Opening a second PR and working on the HttpFoundation/ResponseHeaderBag integration

Why?

Work was invested here already and is close to "mergable"
Merging this provides already value for LanSuite
Making this change later has no impact on the end user and is an internal change (ho additional effort needed by people who run LanSuite)
Moving in small incremental steps is better and faster than "big bangs"
Perfect is the enemy of good

M4LuZ · 2024-02-24T12:33:27Z

I'm all in favor of getting a working solution out of the door, but current solution does not cover this yet:

[...] as current values prevent external inclusions that would be needed for gmaps, analytics and I think also discord.

So we either need to use default values for CSP that allow basically everything (which is not a good idea) oder at least add header overrides to related functions. Overwriting the referrer-option may also be needed for gmaps, as I think one of the recommendations is to restrict use of API keys to a site (which then needs to be checked via client referrer)

Also I see the level of code a bit differently:

this as-is: draft
having some type of header-management: usable
Using some standard framework: good
not having to deal with this at all: perfect ;)

andygrunwald · 2024-02-25T09:23:29Z

I'm all in favor of getting a working solution out of the door, but current solution does not cover this yet:

I think you missed my point.
My point is: Lets add the header that are clear and solved already rather discussing the perfect value of other headers.
With introducing the working headers already, we get at least a bit of improvement.
This is open source work. We don't get paid for this. We are clear about the values for a few headers at least. Lets use those, introduce them and provide value already.

To make a start, I have created:

Those Pull Requests provide a bit of value and we still can continue to work on the CSP header.

M4LuZ · 2024-02-27T11:57:28Z

Yes, I misunderstood, sorry.
Jut merging proven-to-be-ok headers is surely fine!
I continue to fail to see how getting the others in a manageable state is anything even close to "perfect", but happy to work on that in the separate tickets.

Will then include suggestions and remove headers covered by the spin-off tickets.

andygrunwald · 2024-02-27T13:27:42Z

I continue to fail to see how getting the others in a manageable state is anything even close to "perfect", but happy to work on that in the separate tickets.

With manageable you mean that modules can modify these headers?

M4LuZ · 2024-02-28T08:35:26Z

We need at absolute minimum to have a modified CSP in place for:

google maps
google analytics
google translate
~~discord~~ (actually does not include external ressources)
So either these need to be added to the CSP in general or a dedicated CSP header call has to be defined and placed for these, otherwise inclusion of CSP will break functionality.

But that is no biggie, will put that on the to-do list with the other pending changes

M4LuZ · 2024-02-28T09:27:15Z

Ahh, I see you did already most of the changes needed in the spin-off tickets.
Thus this can be closed and work continues in the separate tickets?

andygrunwald · 2024-02-28T19:41:29Z

Thus this can be closed and work continues in the separate tickets?

I suggest to first, merging the spin-off PRs and keep this open as a reminder to work on the CSP header.

Initial draft of headers

443a7c5

Includes settings for cookies fixes #179

M4LuZ added the security issue Issues with relation to or with impact on the system security label May 6, 2022

M4LuZ added this to the LanSuite 5.0 RC milestone May 6, 2022

M4LuZ self-assigned this May 6, 2022

fixed comparison and one comma too much

c7cca14

M4LuZ added the pending-maintainer-response label Dec 28, 2022

M4LuZ marked this pull request as ready for review December 29, 2022 13:30

M4LuZ requested a review from a team December 29, 2022 13:30

M4LuZ mentioned this pull request May 25, 2023

Installation: Make installation work on PHP 8.0 without any error #576

Merged

M4LuZ and others added 5 commits June 1, 2023 10:02

Merge branch 'master' into 179-http-security-headers

fcd7ba9

Merge branch 'master' into 179-http-security-headers

a359864

Merge branch 'master' into 179-http-security-headers

182fc30

Merge branch 'master' into 179-http-security-headers

f4b2aec

Merge branch 'master' into 179-http-security-headers

0b70bf6

M4LuZ and others added 2 commits August 8, 2023 06:38

Merge branch 'master' into 179-http-security-headers

d572c2f

Merge branch 'master' into 179-http-security-headers

3479134

andygrunwald and others added 2 commits September 15, 2023 08:01

Merge branch 'master' into 179-http-security-headers

1a32cad

Merge branch 'master' into 179-http-security-headers

9d28f05

Merge branch 'master' into 179-http-security-headers

5d8ced1

M4LuZ removed this from the LanSuite 5.0 RC milestone Jan 9, 2024

Merge branch 'master' into 179-http-security-headers

976b362

Merge branch 'master' into 179-http-security-headers

4a7aba8

andygrunwald requested changes Feb 22, 2024

View reviewed changes

M4LuZ and others added 2 commits February 23, 2024 22:03

Update index.php

4c54be9

Co-authored-by: Andy Grunwald <andygrunwald@gmail.com>

Update index.php

5f354c8

Co-authored-by: Andy Grunwald <andygrunwald@gmail.com>

Merge branch 'master' into 179-http-security-headers

15819d6

This was referenced Feb 24, 2024

Delete ext_scripts/captcha.php as it is not used #908

Merged

[Security] Set Cookies HTTP only and protocol aware (https) #909

Merged

[Security] Add header X-Frame-Options, Referrer-Policy and Strict-Transport-Security #910

Merged

github-actions bot added the conflicts label Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial draft of headers #483

Initial draft of headers #483

M4LuZ commented May 6, 2022

M4LuZ commented Dec 29, 2022

sonarcloud bot commented Jul 5, 2023

sonarcloud bot commented Aug 8, 2023

sonarcloud bot commented Oct 14, 2023

sonarcloud bot commented Dec 1, 2023

M4LuZ commented Jan 9, 2024

andygrunwald commented Feb 18, 2024

M4LuZ commented Feb 22, 2024 •

edited

andygrunwald commented Feb 22, 2024 •

edited

andygrunwald left a comment

andygrunwald Feb 22, 2024

andygrunwald Feb 22, 2024

andygrunwald Feb 22, 2024

andygrunwald Feb 22, 2024

M4LuZ Feb 22, 2024

andygrunwald Feb 23, 2024

andygrunwald Feb 22, 2024

M4LuZ commented Feb 22, 2024

andygrunwald commented Feb 23, 2024

M4LuZ commented Feb 23, 2024

sonarcloud bot commented Feb 24, 2024

andygrunwald commented Feb 24, 2024

M4LuZ commented Feb 24, 2024 •

edited

andygrunwald commented Feb 25, 2024

M4LuZ commented Feb 27, 2024

andygrunwald commented Feb 27, 2024

M4LuZ commented Feb 28, 2024 •

edited

M4LuZ commented Feb 28, 2024

andygrunwald commented Feb 28, 2024

	header('X-Content-Type-Options: nosniff');
	// TODO: This header is still useful - Once we verified to send the correct MIME types, enable this header
	// header('X-Content-Type-Options: nosniff');

Initial draft of headers #483

Are you sure you want to change the base?

Initial draft of headers #483

Conversation

M4LuZ commented May 6, 2022

M4LuZ commented Dec 29, 2022

sonarcloud bot commented Jul 5, 2023

sonarcloud bot commented Aug 8, 2023

sonarcloud bot commented Oct 14, 2023

sonarcloud bot commented Dec 1, 2023

M4LuZ commented Jan 9, 2024

andygrunwald commented Feb 18, 2024

M4LuZ commented Feb 22, 2024 • edited

andygrunwald commented Feb 22, 2024 • edited

andygrunwald left a comment

Choose a reason for hiding this comment

andygrunwald Feb 22, 2024

Choose a reason for hiding this comment

andygrunwald Feb 22, 2024

Choose a reason for hiding this comment

andygrunwald Feb 22, 2024

Choose a reason for hiding this comment

andygrunwald Feb 22, 2024

Choose a reason for hiding this comment

M4LuZ Feb 22, 2024

Choose a reason for hiding this comment

andygrunwald Feb 23, 2024

Choose a reason for hiding this comment

andygrunwald Feb 22, 2024

Choose a reason for hiding this comment

M4LuZ commented Feb 22, 2024

andygrunwald commented Feb 23, 2024

M4LuZ commented Feb 23, 2024

sonarcloud bot commented Feb 24, 2024

Quality Gate passed

andygrunwald commented Feb 24, 2024

M4LuZ commented Feb 24, 2024 • edited

andygrunwald commented Feb 25, 2024

M4LuZ commented Feb 27, 2024

andygrunwald commented Feb 27, 2024

M4LuZ commented Feb 28, 2024 • edited

M4LuZ commented Feb 28, 2024

andygrunwald commented Feb 28, 2024

M4LuZ commented Feb 22, 2024 •

edited

andygrunwald commented Feb 22, 2024 •

edited

M4LuZ commented Feb 24, 2024 •

edited

M4LuZ commented Feb 28, 2024 •

edited