1801315 - BaseWebSocketChannel releases LoadInfo on a wrong thread.

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

I happened to look at crash stats after https://phabricator.services.mozilla.com/D160595 landed and found https://crash-stats.mozilla.org/report/index/5f19157e-513f-4ebc-bbfc-323de0221118

Worth to audit also other member variables, not only https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/netwerk/protocol/websocket/BaseWebSocketChannel.h#103

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

There is this: https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/netwerk/protocol/websocket/WebSocketChannel.cpp#1228, but the crash happens with WebSocketChannelChild, not WebSocketChannel. Those both extend BaseWebSocketChannel.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Looks like we had a variant of this bug also 8 years ago https://bugzilla.mozilla.org/show_bug.cgi?id=1072316.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I guess I could just fix this.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1801315, move some generic code to the baseclass, r=jesup

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9304147 [details]
Bug 1801315, move some generic code to the baseclass, r=jesup

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The crash report does hint quite a bit how one might write an exploit, though I couldn't immediately write a test which causes a crash.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: esr102 needs a minor tweak. Basically one of the lines removed from WebSocketChannel::~WebSocketChannel() isn't there on esr102 because of bug 1778272. I'll upload the esr102 patch in a moment.
• How likely is this patch to cause regressions; how much testing does it need?: I think this should be rather safe. We don't end up to this code path too often on a worker thread (otherwise we'd have more crash reports) and this makes the WebSocketChannelChild work like WebSocketChannel
• Is Android affected?: Yes

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1801315, move some generic code to the baseclass, esr102, r=jesup

Comment 7

[BugBot [:suhaib / :marco]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:smaug, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 8

[BugBot [:suhaib / :marco]]

The bug is marked as tracked for firefox108 (beta) and tracked for firefox109 (nightly). However, the bug still has low severity.

:ghess, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9304147 [details]
Bug 1801315, move some generic code to the baseclass, r=jesup

Approved to land and uplift

Comment 10

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

move some generic code to the baseclass, r=jesup,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/3714e8faf0915a5b20dc8875138714635f974529
https://hg.mozilla.org/mozilla-central/rev/3714e8faf091

Comment 11

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:smaug, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox108 to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9304147 [details]
Bug 1801315, move some generic code to the baseclass, r=jesup

Beta/Release Uplift Approval Request

• User impact if declined: sec-high crash
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: The patch is based on the crash report's stack trace. I tried to write a testcase but failed.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I believe this should be quite low risk. The crash itself is happening rarely so the new behavior shouldn't be triggered too often either.
• String changes made/needed:
• Is Android affected?: Yes

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9304191 [details]
Bug 1801315, move some generic code to the baseclass, esr102, r=jesup

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: sec-high crash
• Fix Landed on Version: 109
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I believe this should be quite low risk. The crash itself is happening rarely so the new behavior shouldn't be triggered too often either.

This patch has the same behavior as the m-c one, but esr102 just needed a small tweak

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9304147 [details]
Bug 1801315, move some generic code to the baseclass, r=jesup

Approved for 108.0b6.

Comment 15

[Donal Meehan [:dmeehan]]

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

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9304191 [details]
Bug 1801315, move some generic code to the baseclass, esr102, r=jesup

Approved for 102.6esr.

Comment 17

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/c9b1d0395176