luci-base: http.uc: add CSP headers to http response

[josteink]

• This PR is not from my main or master branch 💩, but a separate branch ✅
• Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
• Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
• Incremented 🆙 any PKG_VERSION in the Makefile
• Tested on: (Atheros AR9132 rev 2, 25.12.0-RC1, Chromium) ✅
• ( Preferred ) Mention: @josteink
• ( Preferred ) Screenshot or mp4 of changes:

• ( Optional ) Closes: e.g. Feature request: Content Security Policy #8160
• Description: (describe the changes proposed in this PR)

This PR adds support for Content Security Policy to Luci Web.

Content Security Policy is a very effective security hardening for browser-based applications:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CSP

CSP for OpenWRT/Luci is implemented in 4 different modes:

• None (Disabled - least secure) - default
• Permissive (More secure. Should permit extensive customization)
• Strict (Most secure. Only allow what is required by OpenWRT/Luci by default) - block everything else
• Custom - allow user to define own CSP policy

This option is stored in /etc/config/uhttpd and also manageable via the Luci Web UI under "System" -> "Administration" -> "uHTTPd" -> "HTTP(s) Acces" -> "General settings".

[efahl]

fetch/etc from own origin/IP, and nowhere else.

I didn't try it, but does this prevent "System -> Backup/Flash" from downloading .img files directly? How about Attended Sysupgrade, which does a bunch of POST and GET fetches to do its upgrades?

You can see some of this at

luci/applications/luci-app-attendedsysupgrade/htdocs/luci-static/resources/view/attendedsysupgrade/overview.js

Line 116 in a0531c7

request.get(overview_url)

[josteink]

Good catch. Attended sysupgrade fails, and will need adjustment to the CSP headers. I'll add rules as required to ensure it passes.

Connecting to 'https://sysupgrade.openwrt.org/api/v1/revision/25.12-SNAPSHOT/ath79/generic?1765915536852' violates the following Content Security Policy directive: "default-src 'self'".
Note that 'connect-src' was not explicitly set, so 'default-src' is used as a fallback. The action has been blocked.

I'm not sure what to test in System -> Backup/Flash. I don't see anything there causing (or which should cause) HTTP requests anywhere?

[efahl]

I'm not sure what to test in System -> Backup/Flash. I don't see anything there causing (or which should cause) HTTP requests anywhere?

Oh, ok, I didn't dig into it, I thought it did a .get when you uploaded the Flash image...

[josteink]

Adding https://sysupgrade.openwrt.org fixes the default//OOB config, but I see that it will break with custom configurations:

I guess probably the simplest, most pragmatic option is just to set connect-src to allow anything. The CSP will still be a major improvement over what is already there (which is no hardening at all).

[josteink]

With the testing I've been able to to, it seems like attended sysupgrade now works as well!

[efahl]

Yeah, changing the upstream url, or adding one to "Rebuilders" complicates it. Could the ASU code add exceptions somehow for the duration of its use???

You can use https://sysupgrade.guerra24.net as primary server, or added as a rebuilder, if you felt like testing that...

[josteink]

I went for the pragmatic and simple solution: allow fetch from any HTTPS-host.

It’s not perfect, but it’s still clearly a major improvement over what is there today due to the restricted script and default permissions.

In absence of any other areas to validate/test, I don’t plan any other updates to this PR.

[efahl]

Hmm, maybe another one. I have luci-app-statistics installed on one of my APs, and am getting this when I try to view the generated images:

Content-Security-Policy: The page’s settings blocked the loading of a resource (img-src) at blob:https://wap01/0bd0c7d6-fc6c-4917-a773-8847d822c46e because it violates the following directive: “img-src 'self' data:”

Hacked it up to contain blob: and it works:

507                 if (!this.headers?.['Content-Security-Policy'])
508                         this.header('Content-Security-Policy',
509                         "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval' 'trusted-types-eval';"
510                         + "img-src 'self' data: blob:;"
511                         + "style-src 'self' 'unsafe-inline';"
512                         + "connect-src 'self' https://*;");

[efahl]

And firefox says

Content-Security-Policy: Couldn’t parse invalid host 'trusted-types-eval'

But how do browsers deal with these "bad" ones? Just ignore them? In which case, who cares if it "errors"?

That one specifically seems to have very narrow support, but if it is supported, seems like it should be there.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Security-Policy/script-src#browser_compatibility

[systemcrash]

For reasons you're now discovering, this is why we've not bothered with extended security measures like this, since most of the customisation people do or use depends on the freedom to do so. There are perhaps only a few apps which call to external resources like this, and the sources are entirely verifiable. Introducing this change will likely break some functionality somewhere... :/

[josteink]

Hacked it up to contain blob: and it works:

Nice. That sounds like a reasonable addition as well.

But how do browsers deal with these "bad" ones? Just ignore them? In which case, who cares if it "errors"?

They ignore them. They can’t apply rules/restrictions they don’t know about.

This one in particular is required by Safari and does no harm elsewhere as far as I’ve tested.

[josteink]

For reasons you're now discovering, this is why we've not bothered with extended security measures like this, since most of the customisation people do or use depends on the freedom to do so.

These are IMO simple issues to work around or loosen up.

If people are qualified to customize these things, they’re probably qualified to update or remove this header too?

There are perhaps only a few apps which call to external resources like this

People might get malicious browser extensions which wants to inject scripts into your router interface.

This would help protect against that to a certain extent.

and the sources are entirely verifiable. Introducing this change will likely break some functionality somewhere... :/

IMO script-src is the most important one to get in and the rest can be loosened where there’s any plausible reason for doing so.

[josteink]

From a brainstorming POV, would it be possible to implement this as a luci-app? luci-app-csp?

Then those who want it can install it and it could contain a config-pane (with these defaults) which injects the headers into http.uc?

[Ramon00]

It could be made configurable indeed. Add a option somewhere maybe in the system menu or something, different degrees of allowing things. Add a help text to explain.

[mdevolde]

could contain a config-pane

Or just a text area to write your CSP from start to finish, which is injected into http.uc (this way, anyone who wants to apply a CSP can completely customise their CSP according to their config).

So, an interface to do it properly is available, without having the end user tinkering with ucode files, but the CSP must be written by the end user.

So, nothing broken by installing the app.

[josteink]

Or just a text area to write your CSP from start to finish, which is injected into http.uc (this way, anyone who wants to apply a CSP can completely customise their CSP according to their config).

This sounds more viable from a practical point of view, and for my first Luci submission 😄

Provide a reasonable default, and allow whatever the user wants. If non-empty, use as a CSP headers. If empty, don't provide a CSP header.

I think that's OK for a version 1 implementation, isn't it?

[Ramon00]

I think that's OK for a version 1 implementation, isn't it?

Depends on the default, if the default is empty then this is NOK (people would not know what to fill in).

I do like the idea of user defined.

I would still go for drop down, 1) none 2) loose 3) tight 4) user defined. {previewing the new header) This is most user friendly and will serve everybody. I'm guessing 99% of the people will use 1), 2) or 3).

Do not choose for the easy solution just because you do not have the experience to implement, there are plenty of people willing to help out.

I do wonder what the default should be, 1) or 2) or 3)...

[josteink]

I've added and pushed a first iteration of using UCI for settings and a minimal UI for it.

1. checkbox to enable/disable
2. default policy as derived from the discussion in this PR.

Do we have any good examples in the luci interface of a dropdown which automatically populates the value of an accompagnying text-field?

[Ramon00]

Do we have any good examples in the luci interface of a dropdown which automatically populates the value of an accompagnying text-field?

Yes there should be plenty around, for example look at luci-app-usteer which I made.

[systemcrash]

Use a textbox.

[josteink]

This is the first time I'm trying to build something in the Luci UI. From what I can tell, this is how most textboxes in the uhttpd.js file is defined.

Could you be more concrete wrt to what you think should be "textboxed" here?

[Ramon00]

CSP is a browser-only technology. Forbidding firmware-uploads from the main luci web interface might still permit it from other origins unless it is explicitly forbidden on the server-side.

nevermind, you do not get what i mean

[josteink]

Latest changes used RichListValue and enables some more descriptive content for those not 100% up to speed on CSP.

Looking pretty good, if I have to say so myself!

[mdevolde]

@systemcrash Wouldn't it be more appropriate, for here and for http.uc, to create a dedicated section in /etc/config/uhttpd where we put the different CSP default values, rather than hardcoding them in the js and ucode?

[josteink]

Yes and no?

On the one hand I'd like to avoid code-repetition, embrace modularity and extensibility. That talks in favour of your suggestions.

On the other hand I'd like to make it simple to use. Not just now, but over time.

I think "strict" and "permissive" are tightly connected to the OpenWRT codebase and release itself. What is possible and/or required for "strict" may change over time. If we store this in /etc/config/uhttpd somewhere, user's will typically choose to preserve these settings when doing upgrades.

If the default values for those presets have changed between releases, that will be a upgrade which can fail in unepexted/undesirable ways. Keeping these presets fixed in code avoids that.

And I think that is a bigger problem/risk than these presets being hardcoded in code. That's an argument for keeping things as is.

I'm leaning more towards that direction, but I'm not 100% set on this if someone can explain a way we can make it "upgrade-safe".

[mdevolde]

@josteink I don't understand your concerns. It doesn't change anything from the user's point of view, the strings will just be stored in the httpd config file, and not in the code any more.
It will be easier for developers to maintain, and it doesn't change anything from the user's point of view.
The section containing them will not be editable from JS.
I am waiting for the opinion of a maintainer, in case there are any contraindications.

[josteink]

Just chiming in with a concrete example while awaiting the @systemcrash 's opinion:

1. The user installs OpenWRT release with CSP support (lets say 25.12.1)
2. CSP presets/profiles are stored in /etc/config/uhttpd - based on that release
3. A new Luci/OpenWRT release comes out later (lets says 26.10.0), with some different JS in the browser which requires permissions not granted by the existing CSP preset/profiles. We update the profiles as well to ensure it works.
4. User upgrades to 26.12.0 and checks the option "keep existing config".
5. He will get the config file from 25.12.1 with the old presets, but the Luci code which requires the 26.10.0 presets.
6. Luci may fail to load as a result. User may be unhappy. Depending on his ability to SSH in and edit the relevant config files, he may just say the upgrade "bricked" his router.

That's the scenario I am worried about.

If I'm worrying about something which is trivial to solve, by all means, I'm happy to go that route. Just let me know how!

Just in case it's not obvious: This is my first OpenWRT contribution and I'm fairly new to the source-code 😄

[mdevolde]

@josteink I understand your concern better now, I don't know if there's a way to remedy this.
What's unfortunate at the moment, from a maintainability standpoint, is the repetition of the "strict" CSP in JS and ucode, as well as their decentralisation (not all written CSP at the same place).

[mdevolde]

Maybe a script in /etc/uci-defaults/ could be a solution, I'm curious to hear a maintainer's opinion on this matter.

[josteink]

Custom is going to be custom, no matter what, so that's obviously something which should go in /etc/config/uhttpd as it does now.

is the repetition of the "strict" CSP in JS and ucode

Technically speaking the CSP in the JS is not "strict" but a default-value for "custom", as a convenience, based on "strict". I thought it would be a "nice-to-have" sort of thing, but if it makes things complicated/decentralized, we could just remove the default as well, leaving the only 2 profiles in the ucode part of the codebase.

Either way I'm curious to hear about other, possibly better solutions.

[mdevolde]

@systemcrash ping

[systemcrash]

The problems I see at the moment are as follows:

• settings in an app that might not be installed

This potentially introduces new behaviour, yet does not surface a control element. The behaviour is in base, but the config element is in a separate app. luci-app-uhttpd seems like a good place for it. But luci-app-uhttpd configures uhttpd behaviour. This change is about controlling the front-end behaviour. It seems to me the better placement would be under system->admin->http, which is about the only decent place in base.

This addition correctly 'fails open' - if there is no config present, it remains in a disabled state. You might want to flag that in the GUI - that disabled is the default state.

It also might be a bit inconsistent, but use " to wrap the string, and ' to quote the elements in it.

What settings can render the UI inaccessible?

[josteink]

The problems I see at the moment are as follows:

• settings in an app that might not be installed

I may be misunderstanding something, but the file I modified (uhttpd.js) for the settings is where these kind of HTTP controls used to be already.

This potentially introduces new behaviour, yet does not surface a control element.

This only introduces a new behaviour, if configured. What control elements are you looking for, beside the UI provided?

The behaviour is in base, but the config element is in a separate app. luci-app-uhttpd seems like a good place for it. But luci-app-uhttpd configures uhttpd behaviour. This change is about controlling the front-end behaviour. It seems to me the better placement would be under system->admin->http, which is about the only decent place in base.

The setting you are referring to seems (to my untrained eye) to be uhttpd.js. Check the URL for the admin-page you are referring to:

When grepping the source for "Redirect to HTTPS", this setting is only found in uhttpd.js.

% git grep "Redirect all HTTP to HTTPS" | grep .js
applications/luci-app-uhttpd/htdocs/luci-static/resources/view/uhttpd/uhttpd.js:		o = s.taboption('general', form.Flag, 'redirect_https', _('Redirect all HTTP to HTTPS'));

This addition correctly 'fails open' - if there is no config present, it remains in a disabled state. You might want to flag that in the GUI - that disabled is the default state.

Good idea. I'll make it so.

It also might be a bit inconsistent, but use " to wrap the string, and ' to quote the elements in it.

Fine by me 😄

What settings can render the UI inaccessible?

Only setting which can render the UI inaccessible is "custom" with a bad policy which excludes required JS scripts, in the area clearly marked as "experts only", and with a safe default provided.

Edit: I see my grep was incorrect and there is a different module I can (should?) insert these changes into.

I'll make it so too!

@systemcrash : I assume modules/luci-mod-system/htdocs/luci-static/resources/view/system/uhttpd.js is the right file then, to avoid dependency on non-default app? :)

Or should I keep it in both?

[josteink]

All comments should in theory be resolved? Anything else I should change? 😄

[systemcrash]

If you're aware of any settings already which might be problematic, we might surface a time limited warning when those are entered. I propose a .validate that returns true, but raises a ui.addTimeLimitedNotification(...), if you know a combo which can do this.

I suggest that such a long string warrants a textbox, so everything is visible at once.

If the setting is available in both places (app and mod system), that's OK.

[josteink]

If you're aware of any settings already which might be problematic, we might surface a time limited warning when those are entered.

No settings are by themselves "problematic" more so than the others.

• None. "Problematic" because it offers no browser-based security against external abuse, and you don't want your router to be insecure.
• Permissive. Works with any default OpenWRT install. Could be seen as "problematic" because it permits more attack-vectors than strictly needed (like custom attended sysupgrade feeds), unless the user has a specific need to loosen up security.
• Strict. Works with any default OpenWRT install. Could be "problematic" in the sense that it works now, but you install some other luci-apps and for whatever reason they don't seem to be working (only showing CSP warnings in the browser console) somewhat obscuring the cause of the failure. (This is what I will be running 100% of the time on all of my devices)
• Custom. "Problematic" because there's no way for us to know up front if a policy is going to work or not. This will be 100% dependent on the users deployment and customizations.

Right now, if you do a default OpenWRT install on any device, apply any of the suggested modes (including custom with default value policy) and click "Save & Apply", everything default in OpenWRT will keep on working 100%. You will not be prevented from entering the same admin-section to change the setting later.

Does that clear up the expected behaviour sufficiently?

I propose a .validate that returns true, but raises a ui.addTimeLimitedNotification(...), if you know a combo which can do this.

The only place where there is a real risk is when using "custom" mode. We cannot validate up front if that will be successful or not.

So I guess that means .validate() should return true if current value is "custom" ?

[josteink]

@systemcrash : I tried the following code to just to test. It works, kinda, but I always get the validate-handler invoked twice on changes, leading to the ui popup to appear twice as well.

		csp_mode_option.validate = function(section_id, value) {
			// triggers UI warning when using custom-mode with UI timer based fallback
			if (value === "custom")
			{
			  ui.addTimeLimitedNotification(_('Danger zone'), E('p', _('Custom CSP header will be used as is. If deemed invalid by the browser, you will need to use SSH to reset the value in /etc/config/uhttpd.')), 60000, 'warning');
			}
			return true;
		};

Is this a general luci problem, or is it anything else I'm doing wrong? 😄

[systemcrash]

No, validate is triggered on the cfgvalue and load and I think another one of the function calls, IIRC. :/

That custom string? I can't see anything. That's why it should be a text box.

What I mean are policies like the following:

Content-Security-Policy:
  default-src 'none';
  script-src 'none';
  style-src 'none';
  img-src 'none';
  font-src 'none';
  connect-src 'none';
  frame-src 'none';

So scanning for the key-word none is probably sufficient to trigger a warning.

[josteink]

No, validate is triggered on the cfgvalue and load and I think another one of the function calls, IIRC. :/

Is there a convention somewhere on how to avoid double warnings then, or should I just leave it?

That custom string? I can't see anything. That's why it should be a text box.

What's the best/right way to do that? I don't seen any Form.TextBox type to use here.

So scanning for the key-word none is probably sufficient to trigger a warning.

To be absoultely clear, the default-value for custom (based on strict) intentionally defaults to none unless another policy is set (default-src 'none').

That is, you use the other policies to whitelist only the things you know you need. So this warning will also trigger for the default value.

Also, for instance, for script-src it's not enough to just say "from this origin", you also have to allow what type of unsafe behaviours (like eval) you want to allow.

Creating a CSP-validator which is meaningful is going to be a much bigger job than just getting the basic settings in there, and it will be 10x more complicated than the actual feature already proposed.

If someone later feels CSP is great, and they want a better interactive editor for that... I would delegate that responsibility to a separate, installable luci-app, like we have luci-app-uhttpd for exactly the same reasons for more of the "advanced" uhttpd settings.

It is my opinion that this PR, as is, will provide a substantial security improvement to LUCI compared to how it was before.

Let's resolve the above issues, and leave the CSP-editor/validator aspects to eventually be solved later, if there turns out to be a need?

[systemcrash]

Is there a convention somewhere on how to avoid double warnings then, or should I just leave it?

There is, IIRC. I just can't remember it now... let me have a think.

Ah, sorry - take a look at: https://openwrt.github.io/luci/jsapi/LuCI.form.TextValue.html

There are some in use in the code-base. Take a peek at how others are used.

That is, you use the other policies to whitelist only the things you know you need.

I took a look at MDN which says:

the single value 'none', indicating that the specific resource type should be completely blocked

So we can at least add a note about that in the GUI: e.g. 'none' completely blocks the specific resource type, and if the user saws off the branch they're standing on, it's on them :)

[josteink]

No new changes in latest pull. Have been busy during Christmas as everyone else 😄

Just did a rebase on upstream git master to ensure I'm not walking into any merge-conflicts.