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

[josteink]

• [X] This PR is not from my main or master branch :poop:, but a separate branch :white_check_mark:

• [X] Each commit has a valid :black_nib: Signed-off-by: <[email protected]> row (via git commit --signoff)

• [X] Each commit and PR title has a valid :memo: <package name>: title first line subject for packages

• [X] Incremented :up: any PKG_VERSION in the Makefile

• [X] Tested on: (Atheros AR9132 rev 2, 25.12-SNAPSHOT, Chromium) :white_check_mark:

• [X] ( Preferred ) Mention: @josteink

• [X] ( Preferred ) Screenshot or mp4 of changes:

• [X] ( Optional ) Closes: e.g. openwrt/luci#8160

• [X] Description: (describe the changes proposed in this PR)

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

Content Security Policy is a very effective security hardenining for browser-based applications.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CSP

The provided CSP effectively limits the Luci web-app to only allow:

• scripts from its own origin/IP, and nowhere else.
• css from its own origin/IP, and nowhere else.
• images from its own origin/IP, and nowhere else.
• fetch/etc from own origin/IP, and nowhere else.
• (etc)

Softening exceptions:

• Allow javascript to use inline scripts, eval() and trusted types.
• Allow CSS to use inline styles.
• Allow IMG-tags with data:-type URLs.

trusted-types-eval is required by Safari on MacOS, supported in Firefox Nightly, and ignored in Chrome/Chromium.

This CSP should prevent almost any kind of XSS or script-injection while at the same time allow Luci to perform any task it needs to do.

[github-actions[bot]]

[!WARNING]

Some formality checks failed.

Consider (re)reading submissions guidelines.

Failed checks

Issues marked with an :x: are failing checks.

Commit 2d0c4b29c65a548ffe4c2f1665e4c42d92693b0f

• :large_orange_diamond: Commit subject length: recommended max 50, required max 60 characters

For more details, see the full job log.

Something broken? Consider providing feedback.

[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 https://github.com/openwrt/luci/blob/a0531c7822bc928626b801c7f3bbdc49163a25e8/applications/luci-app-attendedsysupgrade/htdocs/luci-static/resources/view/attendedsysupgrade/overview.js#L116

[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?

[github-actions[bot]]

[!WARNING]

Some formality checks failed.

Consider (re)reading submissions guidelines.

Failed checks

Issues marked with an :x: are failing checks.

Commit 082d034f07b6e3cd8a839ee634c30fd43b1bd73b

• :x: Commit subject line MUST start with <package name>:
• :x: Commit subject line should not end with a period
• :large_orange_diamond: Commit subject length: recommended max 50, required max 60 characters
• :x: Signed-off-by is missing or doesn't match author (should be Signed-off-by: Jostein Kjønigsen <[email protected]>)
• :x: Commit message is missing. Please describe your changes.

For more details, see the full job log.

Something broken? Consider providing feedback.

[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.

[github-actions[bot]]

[!WARNING]

Some formality checks failed.

Consider (re)reading submissions guidelines.

Failed checks

Issues marked with an :x: are failing checks.

Commit 12847166f7890b29763db19b40134b83678d3914

• :x: Commit subject line MUST start with <package name>:
• :large_orange_diamond: Commit subject length: recommended max 50, required max 60 characters

For more details, see the full job log.

Something broken? Consider providing feedback.

[josteink]

The formality check fails because the second commit starts with package name luci-app-uhttpd: which seemingly is wrong.

However, this is the package name used in all previous git commits on this file, and also matches the root folder it's in.

Help anyone? 😄

Edit: I see the formality checks are a fairly new thing. Is this an error with my PR or with the formality check?

[hnyman]

The formality check fails because the second commit starts with package name luci-app-uhttpd:

It actually starts with a space before "luci-app-uhttpd", if you closely to the check results.

Checking subject:
 luci-app-uhttpd: add configuration options for CSP
[fail] Commit subject line MUST start with `<package name>: `

Visible difference to the succeeding commit:

Checking subject:
luci-base: http.uc: add CSP HTTP response headers
[pass] Commit subject line format seems OK

[GeorgeSapkin]

It actually starts with a space before "luci-app-uhttpd", if you closely to the check results.

Haven't seen this one before. I guess this is something to check for.

[josteink]

It actually starts with a space before "luci-app-uhttpd", if you closely to the check results.

Got it. Fixed now!

@Ramon00 wrote:

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).

I instead went with "None", "Permissive", "Strict" & "Custom", but now there's dropdowns there 😄

I'll test more thoroughly tomorrow to ensure there's no stupid bugs, but any comments on the "form" of the UI solution would be appreciated.

[github-actions[bot]]

[!WARNING]

Some formality checks failed.

Consider (re)reading submissions guidelines.

Failed checks

Issues marked with an :x: are failing checks.

Commit 60057d2674e11a4fa17b0210ff43ccdf5eb1f978

• :large_orange_diamond: Commit body line 10 is longer than 75 characters (is 84): The default "strict" provided CSP effectively limits the Luci web-app to only allow:

For more details, see the full job log.

Something broken? Consider providing feedback.

[Ramon00]

I instead went with "None", "Permissive", "Strict" & "Custom", but now there's dropdowns there 😄

Close enough 😄

But question: strict, does that block everything? I mean I probably would not mind having a default that blocks everyting, if i need to upload a new firmware image or somthing, i can always lower it temporarily, you know.

[josteink]

But question: strict, does that block everything? I mean I probably would not mind having a default that blocks everyting, if i need to upload a new firmware image or somthing, i can always lower it temporarily, you know.

Everything works. The intent is for it to be as strict as it can be without breaking any default OpenWRT/Luci functionality.

[github-actions[bot]]

[!WARNING]

Some formality checks failed.

Consider (re)reading submissions guidelines.

Failed checks

Issues marked with an :x: are failing checks.

Commit 60057d2674e11a4fa17b0210ff43ccdf5eb1f978

• :large_orange_diamond: Commit body line 10 is longer than 75 characters (is 84): The default "strict" provided CSP effectively limits the Luci web-app to only allow:

For more details, see the full job log.

Something broken? Consider providing feedback.