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

luci-base: add hardening header #1555

Closed

Conversation

feckert
Copy link
Member

@feckert feckert commented Jan 16, 2018


if http.getenv("HTTPS") == "on" then
local httpsexpire = conf.main.stricthttps or ""
if httpsexpire ~= "" then
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the or "" and simply test for if httpsexpire then

if http.getenv("HTTPS") == "on" then
local httpsexpire = conf.main.stricthttps or ""
if httpsexpire ~= "" then
http.header("Strict-Transport-Security", 'max-age=%s' % { httpsexpire })
Copy link
Contributor

Choose a reason for hiding this comment

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

If only interpolating a single element, leave out the extra table:
http.header("Strict-Transport-Security", 'max-age=%s' % httpsexpire)

@feckert feckert force-pushed the pr/luci-base-add-hardening-header branch from dd478dd to aa8415f Compare January 16, 2018 15:24
@feckert
Copy link
Member Author

feckert commented Jan 16, 2018

@jow- i am note if the name "stricthttps" is the best name for this feature? What do you think

With this change the browser will only send the cookie over a
secure https connection.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
@feckert feckert force-pushed the pr/luci-base-add-hardening-header branch from aa8415f to d9c781f Compare January 16, 2018 15:31
@jow-
Copy link
Contributor

jow- commented Jan 16, 2018

Hm, what about enforce_hsts ?

@feckert
Copy link
Member Author

feckert commented Jan 16, 2018

I think i will use forcehsts because the other configuration under config core 'main' do not use _?

@jow-
Copy link
Contributor

jow- commented Jan 16, 2018

ok

The HTTP Strict-Transport-Security response header (often abbreviated as
HSTS) lets a web site tell browsers that it should only be accessed
using HTTPS, instead of using HTTP.

The Strict-Transport-Security header is ignored by the browser when your
site is accessed using HTTP; this is because an attacker may intercept
HTTP connections and inject the header or remove it.

So the header will only be send if luci is accessed over HTTPS. The "max-age"
expire time could be configured in "/etc/config/luci" main section with
the option "stricthttps".

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
@feckert feckert force-pushed the pr/luci-base-add-hardening-header branch from d9c781f to b02eeec Compare January 16, 2018 16:06
@feckert
Copy link
Member Author

feckert commented Mar 22, 2018

@jow- any progress ?

@@ -406,7 +406,7 @@ function dispatch(request)
return
end

http.header("Set-Cookie", 'sysauth=%s; path=%s' %{ sid, build_url() })
http.header("Set-Cookie", 'sysauth=%s; path=%s; HttpOnly; secure' %{ sid, build_url() })
Copy link
Member

Choose a reason for hiding this comment

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

The secure attribute should only be enabled if HTTPS is on, otherwise browsers will ignore Set-Cookie and users cannot login. It is indeed the case when I patched 17.01.04 armvirt qemu image to test this.

if forcehsts then
http.header("Strict-Transport-Security", 'max-age=%s' % forcehsts)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

This change should better go to uhttpd2

Copy link
Member Author

Choose a reason for hiding this comment

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

@yousong i do not know uhttpd2?

Copy link
Member

Choose a reason for hiding this comment

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

Uhttpd2 is the default HTTP server in OpenWrt. Since the certs, keys etc. required for https are configured on uhttpd, the hsts knob naturally should also go there.

[1] http://lxr.mein.io/source/uhttpd2/proc.c#L150

@yousong yousong closed this in 2f0f456 May 13, 2018
@yousong
Copy link
Member

yousong commented May 13, 2018

Applied the change on sysauth= cookie with minor changes. Thank you.

@feckert feckert deleted the pr/luci-base-add-hardening-header branch June 26, 2019 11:10
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.

None yet

3 participants