1924154 - (CVE-2024-10466) An encrypted push message with rs=0 can freeze the parent process (DoS)

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect, P2)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

https://searchfox.org/mozilla-central/rev/0cd8e314881fc6c216586d1adc13eb8a03f5d040/dom/push/PushCrypto.sys.mjs#111-115

function getCryptoParamsFromPayload(payload) {
  if (payload.byteLength < 21) {
    throw new CryptoError("Truncated header", BAD_CRYPTO);
  }
  let rs =
    (payload[16] << 24) |
    (payload[17] << 16) |
    (payload[18] << 8) |
    payload[19];

if rs becomes 0, it means each record has size of 0. Here we iterate the buffer based on that size, resulting an infinite loop.

The implication here is that a remote server can send a push message with those bytes being 0, which then freezes the subscriber's user agent when it receives the message (which can happen in random time, as it's a push message).

Comment 1

[Andrew McCreight [:mccr8]]

Does this code run in the parent process or a content process? The latter sounds less bad.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

PushCrypto runs on the parent process as PushService runs on it.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1924154 - Disallow too small record r=asuth

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1924154 - Add test r=asuth

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Comment on attachment 9431075 [details]
Bug 1924154 - Disallow too small record r=asuth

Security Approval Request

• How easily could an exploit be constructed based on the patch?: (No sec triage yet, but feel free to discard if not needed)

One would have to understand what record size can do in aes128gcm decryption and what 0 can mean there. (The patch disallows any number below 18 and also touched aesgcm which I believe is not affected, but should help for not-drawing bulls-eye)

• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all branches
• 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?: Shouldn't cause regression; any other incorrect rs value causes decode failure anyway.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 6

[Daniel Veditz [:dveditz]]

This would be a nasty mean trick to pull on people, and probably hard for people to figure out where it's coming from, but it's still hard to rate it much higher "low" given you'd also have to convince people to accept your notifications.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6638d339037
Disallow too small record r=asuth

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1924154 - Disallow too small record r=asuth

Original Revision: https://phabricator.services.mozilla.com/D225687

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9431075 [details]
Bug 1924154 - Disallow too small record r=asuth

This is sec-low (rated after the approval was requested, so thank you), so no approval is needed.

Comment 11

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: An evil remote server can freeze push notification subscribers using Firefox anytime.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: low
• Explanation of risk level: Explicitly banning invalid values; those (other than 0) already throws a (more vague) error already
• String changes made/needed: None
• Is Android affected?: no

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1924154 - Disallow too small record r=asuth

Original Revision: https://phabricator.services.mozilla.com/D225687

Comment 13

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: An evil remote server can freeze push notification subscribers using Firefox anytime.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: low
• Explanation of risk level: Explicitly banning invalid values; those (other than 0) already throws a (more vague) error already
• String changes made/needed: None
• Is Android affected?: no

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/39f59c4e3f43

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/a2bfc916e9cc

Comment 16

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b88c4b5da6d
Add test r=asuth

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68328f7bb076
Fix lint failure in test_crypto_encrypt.js r=fix CLOSED TREE

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2b88c4b5da6d
https://hg.mozilla.org/mozilla-central/rev/68328f7bb076