1705094 - (CVE-2022-22749) QR code scanner allows opening non-HTTP URLs

Status: RESOLVED FIXED

(Fenix :: General, defect)

Description

[Wladimir Palant]

As implemented right now, QR code scanner imposes no restrictions on the URL scheme. As I understand the logic under https://github.com/mozilla-mobile/fenix/blob/d702af5a536f3a0040980acbaba4b0ca40c9c06e/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt#L423, the same restrictions are imposed here as on manually entered URLs. This means no javascript: URLs, but privileged URLs like about:config are allowed. These pages are considered not safe to be opened by websites due to potential vulnerabilities, but QR codes can currently open them.

There is also lack of URL normalization (see bug 1691472), so the confirmation message displayed cannot be considered sufficient protection.

Comment 1

[Daniel Veditz [:dveditz]]

I thought this was already a bug we fixed once?! maybe that was fennec and we reproduced the bug in fenix.

most of our about: urls are safe enough, but there are a couple (and always risks for more) that take parameters that would allow for mischief. Also same for resource: urls.

Comment 2

[Wladimir Palant]

I did test resource: URLs, they don’t load here (and neither when entered manually).

Comment 3

[Christian Sadilek [:csadilek]]

I did test resource: URLs, they don’t load here (and neither when entered manually).

Yes, we block loads of resource://, content:// and file:// URIs in general: https://github.com/mozilla-mobile/android-components/blob/master/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt#L152

So those shouldn't load via the QR scanner either. We should still limit to just http(s) for the scanner.

Comment 4

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

ni to check why this was allowed

Comment 5

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

OK I verified that using LOAD_FLAGS_EXTERNAL fixes this problem (and a host of other ones). Unfortunately, EXTERNAL is not the default as I thought. We maybe should consider switching the default so you have to pass in LOAD_FLAGS_INTERNAL for example for internal page loads, instead of the other way around.

Comment 6

[Roger Yang [:royang]]

Attachment: add_load_url_flag_external_1.patch

Comment 7

[Roger Yang [:royang]]

Attachment: qr_deny_non_https_1.patch

Comment 8

[Christian Sadilek [:csadilek]]

Comment on attachment 9247782 [details] [diff] [review]
qr_deny_non_https_1.patch

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

Something wrong with the condition, see inline. Did you mean to check for both `http` and `https`? We should also call into our normalize URL functions first.

::: app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt
@@ +504,4 @@
>                  requestPermissions(permissions, REQUEST_CODE_CAMERA_PERMISSIONS)
>              },
>              onScanResult = { result ->
> +                if (!URLUtil.isHttpsUrl(result) && !URLUtil.isHttpsUrl(result)) {

Looks like you're checking the same condition twice here.

Comment 9

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

If you pass the external flag you shouldn't be able to load javascript: URIs, so the http/https check is redundant (mentioning in case you see something else in your testing).

Comment 10

[Roger Yang [:royang]]

Attachment: qr_deny_non_https_2.patch

Comment 11

[Christian Sadilek [:csadilek]]

If you pass the external flag you shouldn't be able to load javascript: URIs, so the http/https check is redundant (mentioning in case you see something else in your testing).

Yes, the external flag is working fine and already landed. So the page load is already prevented. This patch here is to improve the user experience, as we would otherwise still open a tab for those invalid URLs...not load it but still display it.

Comment 12

[Christian Sadilek [:csadilek]]

Comment on attachment 9247798 [details] [diff] [review]
qr_deny_non_https_2.patch

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

Looks good to me now!

Comment 13

[Roger Yang [:royang]]

Fix merged in Fenix Nightly with https://github.com/mozilla-mobile/fenix/pull/22268

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is this something we wanted to backport to v95 or should it ride 96?

Comment 15

[Roger Yang [:royang]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)

Is this something we wanted to backport to v95 or should it ride 96?

This will just ride the v96 train. The critical fix is already in v94 and v95. This fix is just UI feedback to the user. Thanks,

Comment 16

[Frederik Braun [:freddy]]

Attachment: advisory.txt