getHSTSPreloadList.js and genHPKPStaticPins.js should gracefully handle trailing whitespace in URL entries

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox-esr38 fixed, firefox-esr45 fixed)

Details

()

Attachments

(2 attachments)

See Bug 1244803 Comment 6, Bug 1244803 Comment 9 and Bug 1244803 Comment 11.

Basically, the Chromium HSTS JSON now contains an entry with the name/URL 'smartphone.continental.com ' (notice the trailing whitespace). When the script tries to connect to the domain, it errors out with an NS_ERROR_MALFORMED_URI exception.

We should instead just handle this gracefully and trim the whitespace before we connect and add the entry to nsSTSPreloadList.inc.
It struck me that the HPKP script should be updated as well to avoid future work (I don't want to have to deal with getting the same fix into four different branches again).
Summary: getHSTSPreloadList.js should gracefully handle trailing whitespace in URL entries → getHSTSPreloadList.js and genHPKPStaticPins.js should gracefully handle trailing whitespace in URL entries
Comment on attachment 8727207 [details]
MozReview Request: Bug 1253958 - Make getHSTSPreloadList.js and genHPKPStaticPins.js gracefully handle trailing whitespace in URL entries.

https://reviewboard.mozilla.org/r/38391/#review35231

Nice job figuring this out. I wonder if in the future we should be a bit more robust against malformed URIs, in case something similar happens that isn't covered by .trim(). In any case, that can be a follow-up, if we decide it's necessary.
Attachment #8727207 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/38391/#review35231

Thanks. I think it probably would be useful to be more robust against malformed URIs and other such input - could be done as part of Bug 905852 maybe.
No try push because the changes here are NPOTB.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b8eeb6a19324
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8727207 [details]
MozReview Request: Bug 1253958 - Make getHSTSPreloadList.js and genHPKPStaticPins.js gracefully handle trailing whitespace in URL entries.

Approval Request Comment
[Feature/regressing bug #]: None. External change that we have to deal with. See comment 0.

[User impact if declined]: Continued bustage of the periodic HSTS preload script, which in turn means users will get stale entries (domains that should be upgraded to HTTPS aren't; domains that should no longer be upgraded still are).

[Describe test coverage new/current, TreeHerder]: HSTS and HPKP are already covered by existing tests.

[Risks and why]: Low. Only makes changes to NPOTB scripts. If the scripts still don't run all the way to success, no code changes are applied.

[String/UUID change made/needed]: None.
Attachment #8727207 - Flags: approval-mozilla-aurora?
Attachment 8727207 [details], rebased for ESR38 and ESR45.

[Approval Request Comment]
See comment 8.
Attachment #8728179 - Flags: review+
Attachment #8728179 - Flags: approval-mozilla-esr45?
Attachment #8728179 - Flags: approval-mozilla-esr38?
Attachment #8727207 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Sylvestre,

When you have the time, could you take a look at the ESR38 and ESR45 approval requests in comment 9? I asked in #relman and was advised to contact you.

Thanks.
Flags: needinfo?(sledru)
If we had this issue since at least esr38, I don't think it matches the esr38 uplift criteria (only sec-high and sec-critical issues).

As the patch is trivial, I am not opposed to take it into esr45 if David thinks it makes sense.
But first, we will need to have into 46 (beta currently).
Flags: needinfo?(sledru) → needinfo?(dkeeler)
The esr criteria it meets are "NPOTB" and "required for automation" and "your branch _will_ be broken."

We do weekly automated updates of security/manager/ssl/nsSTSPreloadList.inc and security/manager/ssl/StaticHPKPins.h on mozilla-central, mozilla-aurora, and on esr branches every Saturday morning. Because the expiration date for those updates is long enough to make the lists last from the last update on mozilla-aurora to the end of a train's released lifecycle, we do not do updates on mozilla-beta or mozilla-release. Landing this patch on beta would not be a problem because the code it patches will never be used, but it would also be pointless because the code it patches will never be used.

Those updates started to fail because of the problem this NPOTB patch works around at the end of January, which means the existing lists on esr38 will expire on April 30th. When esr38.8 releases on April 19th, HPKP and HSTS will still work, users will be protected from connecting to a fake google.com which uses a cert signed by the wrong authority, and will be protected from connecting to gmail over http rather than https and thus being intercepted by a MiTM. Eleven days later, those two features will stop working, and esr users will not have the benefit of them until esr45.2 replaces esr38 on June 7th, and if we have to release an esr38 chemspill in between then, we will have permaorange tests.
Well said. It's important we fix this on all branches that have these automated updates.
Flags: needinfo?(dkeeler)
Comment on attachment 8728179 [details] [diff] [review]
bug1253958_hsts-hpkp-handle-trailing-whitespace_esr38-esr45_v1.patch

Landed as NPOTB in https://hg.mozilla.org/releases/mozilla-esr38/rev/d2c3577f3e3e and https://hg.mozilla.org/releases/mozilla-esr45/rev/fe549690f104 because it is NPOTB, and we need to catch tomorrow morning's update to maximize our number of chances to find out it's still broken by other things before we get to the critical releases that will break.
Attachment #8728179 - Flags: approval-mozilla-esr45?
Attachment #8728179 - Flags: approval-mozilla-esr38?
(In reply to Phil Ringnalda (:philor) from comment #13)
What's the situation on beta, which had stale manifests when we merged from aurora ? Might we break on m-r partway through the 46 cycle ?
As few comments as there are between the two, you wouldn't think I would have had this many opportunities to be confused about where some piece of information went when the answer is "in the other bug."

beta/46's status is in https://bugzilla.mozilla.org/show_bug.cgi?id=1244803#c8 - neither list will expire, though the actual content of the HSTS list is a tiny bit on the stale side.
You need to log in before you can comment on or make changes to this bug.