1912074 - Invalid UTF-16 strings are still being passed as-is for notification actions

Status: RESOLVED FIXED

(Toolkit :: Alerts Service, defect)

Description

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

test_invalid_utf16.html is now passing solely because the parent process is not getting any action at all, as nsIAlertNotification does not have relevant IPC serialization step. Moving the test to chrome-mochitest reveals failure.

It does not cause any crash on my Win11 22631.3880 machine (only action_title test fails by NS_ERROR_FAILURE while others somehow still pass), but I'm tagging this as a sec bug just in case.

(FYI: Bug 1891807 wants to make nsIAlertsService parent-process only, which revealed this)

Comment 1

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

Actions are not exposed to web yet (it's in backlog) so I think this has no real harm for now.

Comment 2

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

The test does not cause crash anymore even without EnsureUTF16Validity, I guess Yannis let Microsoft fix their bug. For now I'll still add the function just for not-yet-updated Windows users.

Comment 3

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

Attachment: Bug 1912074 - Add more validity checks r=nalexander,nrishel

Comment 4

[Andrew McCreight [:mccr8]]

Is somebody going to look into this further to figure out if this is a security issue? Even though it isn't exposed to the web, could this end up running in the parent process with data sent over IPC from a compromised content process?

Comment 5

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

The parent process entry point is PContent::ShowAlert which receives the result of the aforementioned IPC serialization, where the action list will always be empty. So I don't think IPC can pass any action. CC'ing more people just in case.

Comment 6

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

I'm told sec-audit is not really a rating, should I go ahead and land it though?

Comment 7

[Andrew McCreight [:mccr8]]

If you think there's probably not a security issue, then feel free to land it.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/056846758066
Add more validity checks r=nalexander,nrishel

Comment 9

[Iulian Moraru]

Backed out for causing mochitest-chrome failures on test_principal.html.

• Push with failures

• Failure log

• Backout link

[task 2024-08-23T00:52:16.265Z] 00:52:16     INFO - TEST-START | toolkit/components/alerts/test/chrome/test_principal.html
[task 2024-08-23T00:52:16.383Z] 00:52:16     INFO - TEST-INFO | started process screentopng
[task 2024-08-23T00:52:16.650Z] 00:52:16     INFO - TEST-INFO | screentopng: exit 0
[task 2024-08-23T00:52:16.650Z] 00:52:16     INFO - Buffered messages logged at 00:52:16
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Alerts service exists in this application 
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - add_setup | Entering 
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - add_setup | Leaving 
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - add_task | Entering 
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - Buffered messages finished
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/alerts/test/chrome/test_principal.html | Alerts should not be present at the start of the test. 
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:426:16
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - runTest/<@chrome://mochitests/content/chrome/toolkit/components/alerts/test/chrome/test_principal.html:116:7
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - async*add_task/nextTick/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2189:34
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - async*nextTick@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2233:11
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:922:41
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - add_task@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2137:17
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - add_setup@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2253:10
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - runTest@chrome://mochitests/content/chrome/toolkit/components/alerts/test/chrome/test_principal.html:105:12
[task 2024-08-23T00:52:16.651Z] 00:52:16     INFO - @chrome://mochitests/content/chrome/toolkit/components/alerts/test/chrome/test_principal.html:125:1
[task 2024-08-23T00:52:16.652Z] 00:52:16     INFO - add_task | Leaving 
[task 2024-08-23T00:52:16.652Z] 00:52:16     INFO - add_task | Entering testNoPrincipal
[task 2024-08-23T00:52:16.652Z] 00:52:16     INFO - GECKO(6549) | 1724374336392	RemoteAgent	TRACE	Received observer notification domwindowopened
[task 2024-08-23T00:52:16.652Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Should hide alert 
[task 2024-08-23T00:52:16.653Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Should omit source without principal 
[task 2024-08-23T00:52:16.653Z] 00:52:16     INFO - add_task | Leaving testNoPrincipal
[task 2024-08-23T00:52:16.653Z] 00:52:16     INFO - add_task | Entering testSystemPrincipal
[task 2024-08-23T00:52:16.653Z] 00:52:16     INFO - GECKO(6549) | 1724374336576	RemoteAgent	TRACE	Received observer notification domwindowopened
[task 2024-08-23T00:52:16.683Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Should hide alert 
[task 2024-08-23T00:52:16.684Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Should omit source for system principal 
[task 2024-08-23T00:52:16.684Z] 00:52:16     INFO - add_task | Leaving testSystemPrincipal
[task 2024-08-23T00:52:16.685Z] 00:52:16     INFO - add_task | Entering testNullPrincipal
[task 2024-08-23T00:52:16.689Z] 00:52:16     INFO - GECKO(6549) | 1724374336689	RemoteAgent	TRACE	Received observer notification domwindowopened
[task 2024-08-23T00:52:16.768Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Should hide alert 
[task 2024-08-23T00:52:16.770Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Should omit source for null principal 
[task 2024-08-23T00:52:16.770Z] 00:52:16     INFO - add_task | Leaving testNullPrincipal
[task 2024-08-23T00:52:16.770Z] 00:52:16     INFO - add_task | Entering testNodePrincipal
[task 2024-08-23T00:52:16.775Z] 00:52:16     INFO - GECKO(6549) | 1724374336774	RemoteAgent	TRACE	Received observer notification domwindowopened
[task 2024-08-23T00:52:16.854Z] 00:52:16     INFO - TEST-PASS | toolkit/components/alerts/test/chrome/test_principal.html | Should hide alert 
[task 2024-08-23T00:52:16.857Z] 00:52:16     INFO - Not taking screenshot here: see the one that was previously logged
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/alerts/test/chrome/test_principal.html | Should include source for node principal - got null, expected "via mochi.test:8888"
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:509:14
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - testNodePrincipal@chrome://mochitests/content/chrome/toolkit/components/alerts/test/chrome/test_principal.html:94:5
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - async*add_task/nextTick/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2189:34
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - async*nextTick@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2233:11
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:922:41
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - add_task@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2137:17
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - add_setup@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2253:10
[task 2024-08-23T00:52:16.858Z] 00:52:16     INFO - runTest@chrome://mochitests/content/chrome/toolkit/components/alerts/test/chrome/test_principal.html:105:12
[task 2024-08-23T00:52:16.859Z] 00:52:16     INFO - @chrome://mochitests/content/chrome/toolkit/components/alerts/test/chrome/test_principal.html:125:1
[task 2024-08-23T00:52:16.859Z] 00:52:16     INFO - add_task | Leaving testNodePrincipal
[task 2024-08-23T00:52:16.859Z] 00:52:16     INFO - GECKO(6549) | MEMORY STAT | vsize 11544MB | residentFast 501MB | heapAllocated 251MB
[task 2024-08-23T00:52:16.916Z] 00:52:16     INFO - TEST-OK | toolkit/components/alerts/test/chrome/test_principal.html | took 647ms
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - TEST-START | Shutdown
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - Passed:  48
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - Failed:  2
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - Todo:    0
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - Mode:    non-e10s
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - Slowest: 21412ms - chrome://mochitests/content/chrome/toolkit/components/alerts/test/chrome/test_alerts_noobserve.html
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - SimpleTest FINISHED
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - TEST-INFO | Ran 1 Loops
[task 2024-08-23T00:52:16.987Z] 00:52:16     INFO - SimpleTest FINISHED
[task 2024-08-23T00:52:17.024Z] 00:52:17     INFO - TEST-INFO | Main app process: exit 0
[task 2024-08-23T00:52:17.024Z] 00:52:17     INFO - runtests.py | Application ran for: 0:00:35.706136

Comment 10

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

I wanted to retry the test and somehow it just doesn't include toolkit/components/alerts at all? 🤔

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50f610ea3378
Add more validity checks r=nalexander,nrishel,win-reviewers,gstoll

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/50f610ea3378

Comment 13

[(Out Sep 17-23) Ryan VanderMeulen]

This grafts cleanly to ESR128. I'm thinking it might be worth taking this early in its lifecycle for general hardening and ease of maintenance down the road. Please nominate for uplift if you agree :). Otherwise, feel free to set the status to wontfix.

Comment 14

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

Attachment: Bug 1912074 - Add more validity checks r=nalexander,nrishel

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

Comment 15

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Probably none, but may potentially prevent some undisclosed misbehavior.
• 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: Only some validations, no visible behavior change.
• String changes made/needed: None
• Is Android affected?: no

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/0c7d42681e00

Comment 17

[(Out Sep 17-23) Ryan VanderMeulen]

Had to land a follow-up for ESLint failures due to browser_bug1682866.js being moved.
https://hg.mozilla.org/releases/mozilla-esr128/rev/e707e7e6c9e9

What I don't understand is why we didn't see this same issue on central where it appears we have the same issue?

Comment 18

[Mark Banner (:standard8)]

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

Had to land a follow-up for ESLint failures due to browser_bug1682866.js being moved.
https://hg.mozilla.org/releases/mozilla-esr128/rev/e707e7e6c9e9

What I don't understand is why we didn't see this same issue on central where it appears we have the same issue?

Central has bug 1835983 which changed http -> https in the test, so the issue no longer exists there.

Unfortunately it looks like it didn't update the .eslintrc-rollouts.js file. I'll get something on file to do that.