OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.37k stars 2.38k forks source link

Security Headers #854

Closed jrestall closed 2 years ago

jrestall commented 7 years ago

Hi guys,

What are your thoughts on implementing the best practice security headers out of the box in Orchard? We don't necessarily need to have them enabled by default but I was thinking a config switch might be helpful to users of the CMS and developers using the framework.

https://securityheaders.io/ would give us an F rating currently which I'd like to move up.

These are the ones we are currently missing support for:

In addition I would recommend removing the Kestrel Server header and IIS X-Powered-By. I could submit a pull request with the web.config changes for IIS and the AddServerHeader = false for Kestrel.

For the other headers I could create a middleware in the Security module and have a new SecurityHeaderOptions class so that they could be toggled and configured from appsettings.json?

 "Modules": {
    "Orchard.Security":{
      "SecurityHeaders":{
        "RemoveServer": true,
        "XFrameOptions":"deny",
        "XssProtection":"1;mode=block",
        "XContentTypeOptions":"nosniff",
        "StrictTransportSecurity":"max-age=31536000; preload",
        "ReferrerPolicy":"none",
        "PublicKeyPins":"pin-sha256="<pin-value>"; 
                 max-age=<expire-time>; 
                 includeSubDomains; 
                 report-uri="<uri>"",
        "ContentSecurityPolicy":"..."
      }
    }
  }

Feedback would be needed on sensible default values and which to toggle on by default.

Please let me know if there is interest in this and thoughts on the technical design and source location of such functionality.

Jetski5822 commented 7 years ago

I like the idea, and hate the idea of an F!!! Hey @PinpointTownes and @jersiovic - as the chaps that did the security side of things so far, what do you think? I know you both would hate having an F!

jersiovic commented 7 years ago

+1 to move up. So, please go for that PR

kevinchalet commented 7 years ago

Sounds like a very good idea, indeed :+1:

That said, please consider leveraging an existing library instead of reinventing the wheel.

I personally use NWebsec for all my projects (https://github.com/NWebsec/NWebsec/tree/master/src/NWebsec.AspNetCore.Middleware), but there are other projects like https://github.com/TerribleDev/HardHat or https://github.com/andrewlock/NetEscapades.AspNetCore.SecurityHeaders that can help you with that.

The hard part will be making all that stuff not too confusing/too hard to use for the developers. Specially since an invalid configuration can lead to terrible consequences :sweat_smile:

Jetski5822 commented 7 years ago

hey @jrestall Did you manage to make any head way with this?

rserj commented 6 years ago

I guess we should decide where to store such configuration in Config file or Db. I think it would be better to store configuration in Db, so Admin can easily view/change configuration at runtime, +we can add some helpful hints on UI to Admin about each option. (Changing configuration may require restarting Tenant)

I'm thinking about multiple proposals

  1. What if we create OrchardCore.HttpSecurity module And create a class HttpSecurityHeaderSettings : Entity like it was done with SiteSettings and User, so we can create drivers responsible for each of the configuration, like Cors, XSS, Force ContentType, X-Frame-Options, ... Adv. We can easily extend configuration by adding New Drivers Disadv. Since all of these Config sections rendered in one page it will be messy. Until, we migrate that feature from Orchard 1.10.x which allows to organize Part configuration by Tabs http://schouten-online.nl/projecten/created-content-editor-tabs-feature-in-the-core-of-orchard-cms

2) We may create multiple Orchard Features in one module. Each Feature per "Security Header" Configuration (Cors, XSS, Force ContentType, X-Frame-Options, ...). Each of the features will have Controllers/View as usual. I'm not sure about Admin menu, how we should organize it per "Security Header" Configuration, Does Orchard Core renders third level menus as Tabs like Orchard 1?

I think in this case 2nd approach will be better. What do you think about using one of the above @PinpointTownes suggested libraries? Each Feature can read it's own configuration and use one of the NWebsec middlewares for example.

cc: @sebastienros

sebastienros commented 6 years ago

I think we can handle it with a single, specialized module, don't need for extensibility here. Just settings to enable specific properties.

sebastienros commented 6 years ago

As mentioned in https://github.com/OrchardCMS/OrchardCore/pull/1277, we should have this as features in the same Security module.

Another solution is to have a single feature with a single configuration page, and suggest to users that they should check everything. Bonus points if the settings point to articles about why the setting is important.

I am setting the milestone to beta2.

sebastienros commented 6 years ago

/cc @petedavis

petedavis commented 6 years ago

Using cloudflare as an example, clicking enable HSTS should come with some level of acknowledgement before enabling it on a site: image

And we should try and configure the middle ware with from settings: image

sebastienros commented 6 years ago

But we set HSTS when the "force https" option is set, isn't that good enough?

petedavis commented 6 years ago

Having HSTS enabled by default and a zero max-age is missing some of the main benefits of HSTS. Knowing that you cannot disable HTTPS support until HSTS is disabled, and the HSTS max age expires should be known. And then max age can be used effectively as people should understand this is on and cant be turned of like a switch like just HTTPS can.

jptissot commented 5 years ago

I am currently configuring a deployment of Orchard and attempted to implement a CSP for it. I found some issues that need to be addressed. I wanted to force these headers to all tenants, so a module would not be useful for my use case.

Using the https://github.com/andrewlock/NetEscapades.AspNetCore.SecurityHeaders package. This is what my Startup.cs looks like:

using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.DependencyInjection;

namespace web
{
    public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddOrchardCms();
        }

        public void Configure(IApplicationBuilder app, IHostingEnvironment env)
        {
            var policyCollection = new HeaderPolicyCollection()
              .AddFrameOptionsSameOrigin()
              .AddXssProtectionBlock()
              .AddContentTypeOptionsNoSniff()
              .AddReferrerPolicyStrictOriginWhenCrossOrigin()
              .RemoveServerHeader()
              .AddContentSecurityPolicy(builder =>
              {
                  builder.AddObjectSrc().Self();
                  builder.AddFormAction().Self();
                  builder.AddFrameAncestors().Self();
                  builder.AddDefaultSrc().Self();
                  builder.AddFontSrc().Self().From("cdn.jsdelivr.net").From("fonts.googleapis.com").From("fonts.gstatic.com");

                  builder.AddStyleSrc().UnsafeInline().Self().From("cdn.jsdelivr.net").From("fonts.googleapis.com").From("cdn.datatables.net")
                  .From("unpkg.com")
                  .From("cdnjs.cloudflare.com")
                  ;
                  // unsafe-evail needed for vue.js runtime templates
                  builder.AddScriptSrc().UnsafeEval().UnsafeInline().Self()
                   .From("cdn.jsdelivr.net").From("cdn.datatables.net").From("cdnjs.cloudflare.com").From("vuejs.org")
                   .From("unpkg.com")
                   .From("cdnjs.cloudflare.com")
                   ;
              });

            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            } else
            {
                policyCollection.AddStrictTransportSecurityMaxAgeIncludeSubDomains(maxAgeInSeconds: 60 * 60 * 24 * 365); // maxage = one year in seconds
            }

            app.UseSecurityHeaders(policyCollection)
                .UsePoweredByOrchardCore(false)
                .UseStaticFiles()
                .UseOrchardCore(builder=>builder
                    .UsePoweredByOrchardCore(false)
                    .UseCookiePolicy(new CookiePolicyOptions()
                    {
                        HttpOnly = Microsoft.AspNetCore.CookiePolicy.HttpOnlyPolicy.Always,
                        Secure = Microsoft.AspNetCore.Http.CookieSecurePolicy.SameAsRequest
                    })
                );
        }
    }
}

Edit: Some of the issues I mentionned might be fixed when this lands: https://github.com/aspnet/AspNetCore/issues/6001

lahma commented 5 years ago

Mozilla's Observatory gives a pretty readable report

SteGriff commented 4 years ago

Hey folks, I have a business requirement to get these headers on our Orchard instance. Where did this feature get to? If implemented, what version did it go in? Many thanks!

hishamco commented 4 years ago

@sebastienros can we handle this before 1.0.0?

mono-wallace commented 3 years ago

I got a business requirement to get these headers on our Orchard instance. Currently, I have implemented these headers with NWebsec (https://github.com/NWebsec/NWebsec) which is working well for me. The only problem that I am experiencing is that I need to enable the script-src: 'unsafe-eval' because of (dynamic) inline scripts in some Orchard modules. I don't want to allow this. This is the same issue as @jptissot mentions in his comment: https://github.com/OrchardCMS/OrchardCore/issues/854#issuecomment-491041921

Do you have any plans of solving this on short term (this year)

hishamco commented 2 years ago

@sebastienros shall we add this as a middleware, something similar to PoweredByMiddleware or shall we add as a module?

hishamco commented 2 years ago

I think we can handle it with a single, specialized module, don't need for extensibility here. Just settings to enable specific properties.

@sebastienros shall we add new module for that OrchardCore.Security, or can we add a simple middleware as I suggested above?

sebastienros commented 2 years ago

If we have a security module use it, I think we have one. Dedicated feature might be nice.

hishamco commented 2 years ago

I think we have one.

Where? Do you mean Https?

sebastienros commented 2 years ago

If we just have an https module, we could create a security one and move the https feature there

hishamco commented 2 years ago

Make sense, but for now we can introduce a Security module, then add HTTPs as feature before release 2.0.0 to avoid breaking changes

hishamco commented 2 years ago

The OrchardCore.Security module should have at least the following features:

  1. Content Security Policy
  2. XSS Protection
  3. Strict Transport Security
  4. Referrer Policy
  5. Frame Options

Perhaps there are few features related to Https & Cors module, so we could add them here by enabling related modules or move some of them to the OrchardCore.Security as suggested above

@Piedone your feedback please I'm planning to start a PR soon ...

Piedone commented 2 years ago

What does https://securityheaders.com/ say?

hishamco commented 2 years ago

https://securityheaders.com/?q=https%3A%2F%2Forchardcore.net%2F states the following:

Piedone commented 2 years ago

So, I think the goal should be that a vanilla Orchard site gives a flawless score with the extension, with corresponding docs on the necessary setup.

hishamco commented 2 years ago

Few of them are optional, so can we add the above list as features or we they should be added once the module is enabled with no option to toggle them? I think make them as features is a very good choice

So, I think the goal should be that a vanilla Orchard site gives a flawless score with the extension, with corresponding docs on the necessary setup.

We could enable this module on setup

jptissot commented 2 years ago

Have a look at this:

https://github.com/StatCan/StatCan.OrchardCore/blob/master/src/Lib/StatCan.OrchardCore.Security/SecurityExtensions.cs

Maybe a good starting point.

sebastienros commented 2 years ago

@hishamco good, watching

hishamco commented 2 years ago

Adding those header as features are good and bad because the intend to make the default OC site supports those header. So, I will leave this for now

hishamco commented 2 years ago

Adding those header as features are good and bad because the intend to make the default OC site supports those header. So, I will leave this for now

Piedone commented 2 years ago

I don't really understand what you mean by "So, I will leave this for now"

hishamco commented 2 years ago

My intend at the beginning to add these header as separated feature, so they can be enabled or disabled, but this conflict with the original issue where the header should be present

What I did in the related PR is add the headers when the module is enabled (which should be always enabled)

hishamco commented 2 years ago

BTW @Piedone seems the extension that you refer to is working only for a live site, for that my testing is to check the response header

Piedone commented 2 years ago

@sebastienros once we're done here, it would be great if some security expert from your team could also test it while it's running (no need for a detailed code review but that would be even better).

hishamco commented 2 years ago

I already checked the response headers, everything seems works as expected, also I may look for an extension for test this too

hishamco commented 2 years ago

@jrestall finally ;)

Screenshot 2022-05-12 054514

jrestall commented 2 years ago

@hishamco Haha that's epic to see this complete after 5 years, really nice job! 🎉

hishamco commented 2 years ago

It happens ;) you remind me with one of my PR which take two years or more to get merged