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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

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.

Comment 1

[:Cykesiopka]

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

Comment 2

[:Cykesiopka]

Attachment: MozReview Request: Bug 1253958 - Make getHSTSPreloadList.js and genHPKPStaticPins.js gracefully handle trailing whitespace in URL entries.

Review commit: https://reviewboard.mozilla.org/r/38391/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38391/

Comment 3

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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.

Comment 4

[:Cykesiopka]

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.

Comment 5

[:Cykesiopka]

No try push because the changes here are NPOTB.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8eeb6a19324

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b8eeb6a19324

Comment 8

[:Cykesiopka]

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.

Comment 9

[:Cykesiopka]

Attachment: bug1253958_hsts-hpkp-handle-trailing-whitespace_esr38-esr45_v1.patch

Attachment 8727207 [details], rebased for ESR38 and ESR45.

[Approval Request Comment]
See comment 8.

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff51ee15fa46

Comment 11

[:Cykesiopka]

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.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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

Comment 13

[Phil Ringnalda (:philor)]

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.

Comment 14

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Well said. It's important we fix this on all branches that have these automated updates.

Comment 15

[Phil Ringnalda (:philor)]

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.

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

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

Comment 17

[Phil Ringnalda (:philor)]

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.