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

Implement HSTS #14363

Closed
Ms2ger opened this issue Nov 24, 2016 · 7 comments · Fixed by #14716 or #25404
Closed

Implement HSTS #14363

Ms2ger opened this issue Nov 24, 2016 · 7 comments · Fixed by #14716 or #25404
Labels
A-network C-assigned There is someone working on resolving the issue

Comments

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 24, 2016

No description provided.

@frewsxcv
Copy link
Contributor

Relevant section:

https://fetch.spec.whatwg.org/#main-fetch

Part of step 9:

Matching request’s current url’s host per Known HSTS Host Domain Name Matching results in either a superdomain match with an asserted includeSubDomains directive or a congruent match (with or without an asserted includeSubDomains directive) [HSTS]

@frewsxcv frewsxcv assigned frewsxcv and unassigned frewsxcv Dec 7, 2016
@frewsxcv
Copy link
Contributor

frewsxcv commented Dec 8, 2016

Related: #8580

@nmvk nmvk mentioned this issue Dec 25, 2016
4 tasks
bors-servo pushed a commit that referenced this issue Dec 28, 2016
Implement HSTS fetch step

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix #14363
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14363

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14716)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Dec 28, 2016
Implement HSTS fetch step

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix #14363
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14363

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14716)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Dec 29, 2016
Implement HSTS fetch step

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix #14363
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14363

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14716)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 4, 2017
…r=jdm

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix servo/servo#14363
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14363

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: c7991d596f7453d09c2b2a98eecce72f182a4e24
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…r=jdm

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix servo/servo#14363
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14363

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: c7991d596f7453d09c2b2a98eecce72f182a4e24

UltraBlame original commit: fb3cb6e8f720e2772efba1fae1ea4dc87b8e8a72
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…r=jdm

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix servo/servo#14363
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14363

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: c7991d596f7453d09c2b2a98eecce72f182a4e24

UltraBlame original commit: fb3cb6e8f720e2772efba1fae1ea4dc87b8e8a72
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…r=jdm

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix servo/servo#14363
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14363

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: c7991d596f7453d09c2b2a98eecce72f182a4e24

UltraBlame original commit: fb3cb6e8f720e2772efba1fae1ea4dc87b8e8a72
@Darkspirit
Copy link
Sponsor Contributor

Darkspirit commented Dec 24, 2019

Please reopen. This issue was filed for the regression caused by #14360 (comment):
It removed the function that added received HSTS headers to the internal HSTS database. Since then, only Preloaded entries are applied. The PR that closed this issue did not add it back.

Only these cases are tested:

Not tested:

  • HSTS header received via https is added to the internal database

Likely not tested:

  • ws:// is upgraded to wss://
  • HSTS header received via wss:// is added to the internal database

Originally I was just asking myself, if ws:// gets upgraded to wss:// by current code. It doesn't seem so.
https://fetch.spec.whatwg.org/#websocket-opening-handshake

This upgrades http to https:

// Step 10.
context
.state
.hsts_list
.read()
.unwrap()
.switch_known_hsts_host_domain_url_to_https(request.current_url_mut());

If websocket requests go through this function, ws to wss upgrades could be done here as well:
/// Step 10 of https://fetch.spec.whatwg.org/#concept-main-fetch.
pub fn switch_known_hsts_host_domain_url_to_https(&self, url: &mut ServoUrl) {
if url.scheme() != "http" {
return;
}
if url
.domain()
.map_or(false, |domain| self.is_host_secure(domain))
{
url.as_mut_url().set_scheme("https").unwrap();
}
}

Otherwise, a call to switch_known_hsts_host_domain_url_to_https() could be added after this code:

let scheme = req_builder.url.scheme();
let mut req_url = req_builder.url.clone();
if scheme == "ws" {
req_url.as_mut_url().set_scheme("http").unwrap();
} else if scheme == "wss" {
req_url.as_mut_url().set_scheme("https").unwrap();
}

@jdm
Copy link
Member

jdm commented Dec 24, 2019

Thanks for checking!

@jdm jdm reopened this Dec 24, 2019
@Darkspirit
Copy link
Sponsor Contributor

Darkspirit commented Dec 24, 2019

Along this, it would be good to add these prefs to switch_known_hsts_host_domain_url_to_https():

  • "network.enforce_https.enabled": false, with the following exceptions:
    • "network.enforce_https.localhost": false, (incl. loopback IPs)
    • "network.enforce_https.onion": false,

This would also address #20120. The same should be done for Firefox: bug 1335590

@Darkspirit
Copy link
Sponsor Contributor

@highfive assign me

@highfive highfive added the C-assigned There is someone working on resolving the issue label Dec 25, 2019
@highfive
Copy link

Hey @Darkspirit! Thanks for your interest in working on this issue. It's now assigned to you!

@Darkspirit Darkspirit mentioned this issue Dec 30, 2019
4 tasks
bors-servo pushed a commit that referenced this issue Jan 8, 2020
Fix HSTS

The headers crate does not [expose](https://github.com/hyperium/headers/blob/0c42ad8cf56f9af28973c3da71616fa478781fdf/src/common/strict_transport_security.rs#L42) HSTS struct fields. At the moment, it's only usable for HSTS header encoding. An update of the headers crate would require a huge update of http, hyper, hyper_serde, net::decoder as well. Therefore I've copied the `typed_get::<StrictTransportSecurity>` decoding feature  for now, but with exposed struct fields. Let's remove this custom struct with the next hyper upgrade. I tried to prevent needless HSTS database lookups when network.enforce_tls.enabled is set.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14363.

- [x] There are tests for these changes
bors-servo pushed a commit that referenced this issue Jan 8, 2020
Fix HSTS

The headers crate does not [expose](https://github.com/hyperium/headers/blob/0c42ad8cf56f9af28973c3da71616fa478781fdf/src/common/strict_transport_security.rs#L42) HSTS struct fields. At the moment, it's only usable for HSTS header encoding. An update of the headers crate would require a huge update of http, hyper, hyper_serde, net::decoder as well. Therefore I've copied the `typed_get::<StrictTransportSecurity>` decoding feature  for now, but with exposed struct fields. Let's remove this custom struct with the next hyper upgrade. I tried to prevent needless HSTS database lookups when network.enforce_tls.enabled is set.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14363.

- [x] There are tests for these changes
bors-servo pushed a commit that referenced this issue Jan 8, 2020
Fix HSTS

The headers crate does not [expose](https://github.com/hyperium/headers/blob/0c42ad8cf56f9af28973c3da71616fa478781fdf/src/common/strict_transport_security.rs#L42) HSTS struct fields. At the moment, it's only usable for HSTS header encoding. An update of the headers crate would require a huge update of http, hyper, hyper_serde, net::decoder as well. Therefore I've copied the `typed_get::<StrictTransportSecurity>` decoding feature  for now, but with exposed struct fields. Let's remove this custom struct with the next hyper upgrade. I tried to prevent needless HSTS database lookups when network.enforce_tls.enabled is set.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14363.

- [x] There are tests for these changes
bors-servo pushed a commit that referenced this issue Jan 8, 2020
Fix HSTS

The headers crate does not [expose](https://github.com/hyperium/headers/blob/0c42ad8cf56f9af28973c3da71616fa478781fdf/src/common/strict_transport_security.rs#L42) HSTS struct fields. At the moment, it's only usable for HSTS header encoding. An update of the headers crate would require a huge update of http, hyper, hyper_serde, net::decoder as well. Therefore I've copied the `typed_get::<StrictTransportSecurity>` decoding feature  for now, but with exposed struct fields. Let's remove this custom struct with the next hyper upgrade. I tried to prevent needless HSTS database lookups when network.enforce_tls.enabled is set.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14363, fix #20120.

- [x] There are tests for these changes
bors-servo pushed a commit that referenced this issue Jan 8, 2020
Fix HSTS

The headers crate does not [expose](https://github.com/hyperium/headers/blob/0c42ad8cf56f9af28973c3da71616fa478781fdf/src/common/strict_transport_security.rs#L42) HSTS struct fields. At the moment, it's only usable for HSTS header encoding. An update of the headers crate would require a huge update of http, hyper, hyper_serde, net::decoder as well. Therefore I've copied the `typed_get::<StrictTransportSecurity>` decoding feature  for now, but with exposed struct fields. Let's remove this custom struct with the next hyper upgrade. I tried to prevent needless HSTS database lookups when network.enforce_tls.enabled is set.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14363, fix #20120.

- [x] There are tests for these changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network C-assigned There is someone working on resolving the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants