1713638 - (CVE-2021-29971) The site permissions feature is leaking permissions to sites with the same scheme/port.

Status: RESOLVED FIXED

(Fenix :: General, defect)

Description

[Arturo Mejia [:amejia]]

Attachment: db_permissions_example.png

Reading over the weekend about Origin, origin attributes and principals, I noticed there is a security gap on my implementation of site permissions, I'm storing only the host name (see the attachment db_permissions_example.png), instead of the origin (Schema + host + port) on the Site permissions DB, causing that sites under different schema and port but with the same host share the same permission with each other, for example:

https://example.com/
http://example.com:8080/
https://example.com:4343/

Based on the rules of same origin, these three sites, should be treated as completely different sites, but in my implementation as I'm only storing the host, all of the three will shared permissions between each other.

For example, an user navigates to https://example.com/ and grants location permission, this will cause that http://example.com:8080/ and https://example.com:4343/ will also have the location permission granted.

In 91, we are expecting to land a patch that will migrate AC site permissions db to geckoView storage ( that will store site permissions based on principals) , but this will only cover Content permission media permission (Camera and microphone) are still going to be stored on AC see https://bugzilla.mozilla.org/show_bug.cgi?id=1712756.

We would like the security team to assess the severity of the issue to see the steps to follow.

Thanks in advance, and sorry for all the trouble.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Adding some GeckoView folks to figure out whether we can move saving media permissions to GV too (like content permissions in bug 1712756 / 91+). Ideally A-C doesn't need to store anything here and then we can't do it wrong (afaik we do not know about all origin properties to do the right thing?). :)

Comment 2

[Arturo Mejia [:amejia]]

That will be ideal, this way AC will not have to store principals to support container or other origin attributes for Media permissions, and we will share the same logic as FF Desktop.

Comment 3

[Daniel Veditz [:dveditz]]

I was initially thinking sec-moderate because at least it requires a user to grant the permission, but by not taking the scheme into account anyone local could set up a MITM insecure http site to take advantage of these permissions.

And that's even before getting to originAttributes, where the engine keeps track of "super origins" for various privacy and security reasons that we'll want Fenix to also do.

Principals are the only real thing -- compare those, and that can't really be done in the front end.

Comment 4

[Arturo Mejia [:amejia]]

Attachment: 0001-Improvements-to-site-permissions.patch

This is a draft with minimal tests just to validate the idea, on it I'm doing two principal things:

1. Triggering a migration on the site permissions DB to update the origin column that now only holds the host, and updating it to have the following pattern https://$origin_column:443, as we were not getting permission requests on http sites, none http permissions should be on the db, I'm defaulting all the already existing permissions to be updated to schema https and port 443 that it's the default for https.
2. Making sure all new permissions will store the origin (schema + host + port).

Comment 5

[Arturo Mejia [:amejia]]

I chatted with Christian about the patch and we would like to get feedback from the security team about #1 (the migration aspect).
We would like to confirm if it's an acceptable solution to migrate existing permissions this way, as the only other alternative we see is deleting/reseting all site permissions, meaning users will be prompted again.

To provide some context about the approach above. We don't get permission requests for HTTP content, so I have a good level of confidence that all the data on the db should be for HTTPS sites. However, we're not sure about the ports, because we didn't store that information when the permissions was requested, but in most of the cases, in my opinion I think default port will a good option, and as part of the migration I'm assuming that all sites were from the default port 443 for HTTPS sites. The origin column on the db entries will end up with this pattern https://$origin_column:443 after the migration.

Thanks in advance.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

I agree the front-end shouldn't have to deal with this stuff. Ideally we can store all permissions in GV (so basically do Bug 1712756), in hindsight we should have provided a way for the front-end to easily check whether two permissions belong to the same origin.

Comment 7

[Arturo Mejia [:amejia]]

(In reply to Arturo Mejia from comment #5)

I chatted with Christian about the patch and we would like to get feedback from the security team about #1 (the migration aspect).
We would like to confirm if it's an acceptable solution to migrate existing permissions this way, as the only other alternative we see is deleting/reseting all site permissions, meaning users will be prompted again.

To provide some context about the approach above. We don't get permission requests for HTTP content, so I have a good level of confidence that all the data on the db should be for HTTPS sites. However, we're not sure about the ports, because we didn't store that information when the permissions was requested, but in most of the cases, in my opinion I think default port will a good option, and as part of the migration I'm assuming that all sites were from the default port 443 for HTTPS sites. The origin column on the db entries will end up with this pattern https://$origin_column:443 after the migration.

Thanks in advance.

Hi Daniel, I would like to confirm the question above, should we own that decision or the security team do?
Thanks in advance!

Comment 8

[Daniel Veditz [:dveditz]]

The security team will insist that we compare origins going forward. How we get from here to there is a collaboration since the work falls on engineering and the product folks will likely have opinions on the user experience.

Your plan to assume https:// and port 443 is fine. It's sure to be correct almost always in practice, and in the rare case the port was incorrect it's still more secure than today because the permission could only be abused by someone with a valid cert for the domain and no longer over an unencrypted http connection.

Comment 9

[Arturo Mejia [:amejia]]

Attachment: AC-Improvements-to-site-permissions.patch

This patch covers the AC changes.

Comment 10

[Arturo Mejia [:amejia]]

Attachment: FENIX-Improvements-to-site-permissions.patch

This patch covers the Fenix changes.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 9226579 [details] [diff] [review]
AC-Improvements-to-site-permissions.patch

Review of attachment 9226579 [details] [diff] [review]:
-----------------------------------------------------------------

This LGTM for checking the full origin (protocol, host, port).

But still feels like we should get discuss bug 1712756 with the GV team as soon as possible, so that we can check the full principal and do not have to maintain this logic in A-C.

::: components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt
@@ +138,5 @@
> +fun String.getOrigin(): String {
> +    val url = URL(this)
> +    val port = if (url.port == -1) url.defaultPort else url.port
> +    return URL(url.protocol, url.host, port, "").toString()
> +}

The URL class can throw `MalformedURLException` if it cannot parse the URL. Do we need to protect against this here?

@@ +144,5 @@
> +/**
> + * Returns an origin without the default port.
> + * For example for an input of "https://mozilla.org:443" you will get "https://mozilla.org".
> + */
> +fun String.stripDefaultProtocol(): String {

The name of the function says "protocol", but it strips the "port"? :)

Comment 12

[Arturo Mejia [:amejia]]

Thanks for the review, I will update the patches with your feedback!

But still feels like we should get discuss bug 1712756 with the GV team as soon as possible, so that we can check the full principal and do not have to maintain this logic in A-C.

I'm agree we should continue the discussion. From Agi's comment 6, sounds like the GV team is opened to store all permissions. He could help us to clarify that point.

Comment 13

[[ex-Mozilla] Agi Sferro | :agi]

I agree, I'll discuss with the team wrt priority

Comment 14

[Arturo Mejia [:amejia]]

Comment on attachment 9226579 [details] [diff] [review]
AC-Improvements-to-site-permissions.patch

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, as only sites with previous permissions granted but with a different schema or port can exploit
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All of them
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. Not much as the migration will happen right after the user starts the app.
  but basic testing of having an affected version and seeing sites are migrated to the correct origin(schema + domain + port) is required.

Comment 15

[Tom Ritter [:tjr]]

Approved to land and uplift when ready

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9226579 [details] [diff] [review]
AC-Improvements-to-site-permissions.patch

I missed the other one

Comment 17

[Julien Cristau [:jcristau]]

android-components:

• master: https://github.com/mozilla-mobile/android-components/commit/54c391787c1ed679c0ab9b0f58557b78da81eb3b
• 90 branch: https://github.com/mozilla-mobile/android-components/commit/b9acdc18981236245d79a91554cf1df8a5a60cd2

fenix:

• master: https://github.com/mozilla-mobile/fenix/commit/251bfc7fe900c5a741bc630aeb77cf97b0feb646
• 90 branch: coming soon

Comment 18

[Ryan VanderMeulen [:RyanVM]]

90 branch: https://github.com/mozilla-mobile/fenix/commit/34b1e77dbc54d1f8db536545e37cdaf86b39bd00

Included in the 90.0.0-beta.5 build.

Comment 19

[Tom Ritter [:tjr]]

Attachment: advisory.txt