Closed
Bug 562475
Opened 15 years ago
Closed 14 years ago
Bugzilla should use strict-transport-security (STS) headers
Categories
(Bugzilla :: Bugzilla-General, enhancement, P2)
Bugzilla
Bugzilla-General
Tracking
()
RESOLVED
FIXED
Bugzilla 4.0
People
(Reporter: clyon, Assigned: reed)
References
Details
(Whiteboard: [infrasecq2][wanted-bmo])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
Bugzilla content that is served over https should use STS headers on https responses for better security.
Reference:
http://lists.w3.org/Archives/Public/www-archive/2009Sep/att-0051/draft-hodges-strict-transport-sec-05.plain.html
Assignee | ||
Updated•15 years ago
|
Severity: major → enhancement
Comment 1•15 years ago
|
||
We've discussed this before, and I seem to recall that aspects of the policy were inappropriate for Bugzilla in general, though it may be a reasonable customization or Advanced parameter.
Looking over it again, I can see that one problem is that it denies self-signed certificates, which many Bugzilla installations have.
Comment 2•15 years ago
|
||
There actually won't be a problem with self signed certs since the browser requires a fully validated certificate chain and an negotiated HTTPS session with 0 errors before the STS header is considered. So in the scenario of a self-signed cert the browser would detect the self signed cert and then ignore the STS header.
Aside from the above issue, can't we just add this feature as a configurable preference? If someone wants to take advantage of STS then they can enable the feature.
Comment 3•15 years ago
|
||
(In reply to comment #2)
> There actually won't be a problem with self signed certs since the browser
> requires a fully validated certificate chain and an negotiated HTTPS session
> with 0 errors before the STS header is considered. So in the scenario of a
> self-signed cert the browser would detect the self signed cert and then ignore
> the STS header.
Okay. Could you point me to the part of the spec that says that?
> Aside from the above issue, can't we just add this feature as a configurable
> preference? If someone wants to take advantage of STS then they can enable the
> feature.
That's what I said in comment 1.
Comment 4•15 years ago
|
||
(In reply to comment #3)
> (In reply to comment #2)
> > There actually won't be a problem with self signed certs since the browser
> > requires a fully validated certificate chain and an negotiated HTTPS session
> > with 0 errors before the STS header is considered. So in the scenario of a
> > self-signed cert the browser would detect the self signed cert and then ignore
> > the STS header.
>
> Okay. Could you point me to the part of the spec that says that?
Section 7.1
If an HTTP response, received over a secure transport, includes a Strict-Transport-Security HTTP Response Header field and there are no underlying secure transport errors, the UA must:
Comment 5•15 years ago
|
||
Hmm, the spec should probably be clarified there to indicate that certificate errors are underlying transport errors (as is made clear in the other places in the document "errors" is used). However, if your interpretation is indeed correct, then I wouldn't see any harm in Bugzilla always sending these headers if the "ssl" option is on and we're in an https connection.
Priority: -- → P2
Target Milestone: --- → Bugzilla 3.8
Comment 6•15 years ago
|
||
I was talking with Sid yesterday, who's implementing STS in firefox and that's how he explained it to me. Any SSL related errors results in the STS being ignored by the browser.
Agreed, the spec could be more clear.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [infrasecq2] → [infrasecq2][wanted-bmo]
Assignee | ||
Comment 8•14 years ago
|
||
Something like this? Untested, but I'll test it soon. Just wanted to see if I'm doing the right thing.
Assignee | ||
Comment 9•14 years ago
|
||
Basic testing complete, and it works. I'll do some more testing later.
Assignee | ||
Comment 10•14 years ago
|
||
Comment on attachment 453564 [details] [diff] [review]
patch - v1
>+ # Note: 2628743 seconds is equal to one month.
>+ if ($self->https && Bugzilla->params->{'ssl_redirect'}) {
>+ unshift(@_, '-strict-transport-security' => 'max-age=2628743');
Actually, it should be 2629744, as per http://www.google.com/search?hl=en&q=number+of+seconds+in+a+month.
Comment 11•14 years ago
|
||
Comment on attachment 453564 [details] [diff] [review]
patch - v1
I think a month is too long--there's a chance that a site could legitimately downgrade to a self-signed cert (perhaps they couldn't afford a new signed cert?) and then users would be locked out for a month.
It we set this to a day, do we lose most of its value?
Attachment #453564 -
Flags: review?(mkanat) → review-
Comment 12•14 years ago
|
||
> It we set this to a day, do we lose most of its value?
A day? yes. The point is that the browser needs to remember for longer than the gap between most users' visits. I suppose you could try to say only "active" BMO users have something valuable to lose, but even I go on vacation sometimes and lots of people fail to work weekends (laggards!).
The spec (or at least an early version of it) recommends that user agents have some way of manually forgetting the state for just this kind of purpose. Firefox's implementation definitely will.
Comment 13•14 years ago
|
||
Okay, in that case, put the max-age value into a constant, so people can change it if they really want to. It should still be shorter than a month, though.
The thing is, you're putting this into Bugzilla by default, and it's a thing that very few people know about or understand yet. So this will affect every installation using SSL, and although you and I may know that Firefox will have a UI to clear the STS memory, no other users will know that, and neither will the site admins, since they didn't explicitly decide to start sending STS headers--we made that decision for them.
Comment 14•14 years ago
|
||
> (perhaps they couldn't afford a new signed cert?) and then
> users would be locked out for a month.
Then they're doing it wrong. http://cert.startcom.org/
Or get a 30-day-free trial cert from one of the other guys to fill the gap.
But really there doesn't need to be a gap. You can turn down the max-age and it will replace the previous data. So if you know your cert is expiring and you're not going to replace it start sending a max-age of 1 for the last month and problem's solved. If you don't have a full month notice at least you'll catch most people. And then there's always the override in the client.
I kind of agree with your r- for generic bugzilla now that I look at the patch. Hardcoded is fine for BMO but generically it should be a tunable config setting for admins to tweak.
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #453564 -
Attachment is obsolete: true
Attachment #454201 -
Flags: review?(mkanat)
Comment 16•14 years ago
|
||
Comment on attachment 454201 [details] [diff] [review]
patch - v2
Okay, this looks good.
I hope that the UA error messages are clear, here, when this fails.
Attachment #454201 -
Flags: review?(mkanat) → review+
Updated•14 years ago
|
Flags: approval+
Assignee | ||
Comment 17•14 years ago
|
||
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/CGI.pm
modified Bugzilla/Constants.pm
Committed revision 7253.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•