Closed Bug 1625749 Opened 1 year ago Closed 1 year ago

Crash in [@ mozilla::SlicedInputStream::Length]

Categories

(Core :: Networking: HTTP, defect, P1)

74 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox-esr68 76+ fixed
firefox74 --- wontfix
firefox75 + wontfix
firefox76 + fixed
firefox77 + fixed

People

(Reporter: philipp, Assigned: valentin)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [necko-triaged][post-critsmash-triage][adv-main76+r][adv-ESR68.8+r])

Crash Data

Attachments

(2 files)

This bug is for crash report bp-9ae6ca03-bd87-4681-83b6-82c850200326.

Top 10 frames of crashing thread:

0 xul.dll mozilla::SlicedInputStream::Length xpcom/io/SlicedInputStream.cpp:584
1 xul.dll static mozilla::InputStreamLengthHelper::GetSyncLength xpcom/io/InputStreamLengthHelper.cpp:78
2 xul.dll mozilla::net::HttpChannelParent::DoAsyncOpen netwerk/protocol/http/HttpChannelParent.cpp:527
3 xul.dll mozilla::net::HttpChannelParent::Init netwerk/protocol/http/HttpChannelParent.cpp:135
4 xul.dll mozilla::net::NeckoParent::RecvPHttpChannelConstructor netwerk/ipc/NeckoParent.cpp:301
5 xul.dll mozilla::net::PNeckoParent::OnMessageReceived ipc/ipdl/PNeckoParent.cpp:1857
6 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:6314
7 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2137
8 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1976
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1220

this crash signature is spiking up in firefox release since version 74 & it's happening across platforms. many comments are saying they were uploading large files when the crash took place - https://www.swisstransfer.com is a url that's frequently showing up in reports.

The crash is happening inside HttpChannelParent::DoAsyncOpen(), at least on the handful of crashes I looked at.

Group: core-security → network-core-security
Component: General → Networking
Component: Networking → Networking: HTTP

Nhi, please find someone to look at this.

Flags: needinfo?(nhnguyen)
Duplicate of this bug: 1628624

See https://bugzilla.mozilla.org/show_bug.cgi?id=1628624 there are STRs which consists or creating an account on https://tales.harpy.gg/ and upload this image:

https://cdn.discordapp.com/attachments/697712641698955265/697713242880868402/Lucky_You.png

This was reported publicly here:
https://forums.mozfr.org/viewtopic.php?f=5&t=143720&p=900574#p900574

This person provided a crash report with a this comment (same explanations than in the forums:

In Happens when I try to select a file after clicking an input file.
This image triggers the bug: https://cdn.discordapp.com/attachments/695950877462233138/695951131700101160/sirene.png
This one doesn’t: https://cdn.discordapp.com/attachments/695950877462233138/695951220229275669/sirene_300.png

Tried on my local env and I don’t have the bug.
Tried on Firefox 72 and no bug (only got it after updating to FF 75)

Also the main difference between my local env and the prod is that I have a service worker in production.

[Tracking Requested - why for this release]: sec-high with publicly reported possible STR

Valentin, could you take a look? Thanks!

Flags: needinfo?(nhnguyen) → needinfo?(valentin.gosu)
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Priority: -- → P1
Whiteboard: [necko-triaged]

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Quite easily.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: Bug 1434553
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: Applies cleanly on esr68
  • How likely is this patch to cause regressions; how much testing does it need?: Very low chance of regressions.
Attachment #9140529 - Flags: sec-approval?
  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: Bug 1434553

What happened in 74 that caused this Fx62 regression to suddenly start exhibiting itself? Is there more to this bug? Should we fix it on ESR/Fennec anyway just in case, or can we rely on something else to be protecting us there?

Flags: needinfo?(valentin.gosu)
Regressed by: 1434553

Do we have the same problem of missing initialization in one of multiple constructors with PartiallySeekableInputStream?

https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/netwerk/base/PartiallySeekableInputStream.cpp#43-44,52-54

(In reply to Daniel Veditz [:dveditz] from comment #9)

  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: Bug 1434553

What happened in 74 that caused this Fx62 regression to suddenly start exhibiting itself? Is there more to this bug? Should we fix it on ESR/Fennec anyway just in case, or can we rely on something else to be protecting us there?

I assumed we introduced some code that caused us to call the other constructor in these cases (IPC deserialization)
I didn't look into it too much, since I caught the e4e4e4 pointer in gdb, and it was quite clear what the problem was.
But you're correct to ask why that's happening. I did a mozregression and reduced it to this:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=720c1e5a8dd3f5bda4ae32137d1c624a1ad55301&tochange=be9a6289486a6f366e431782b84a0c0633f8fec2
Not sure why the reporter says it worked for them in 72. I got several crashes in 72 while finding the regression.

I would uplift to ESR just in case - it's still an uninitialized pointer and there are likely codepaths that exercise it in ESR too.

(In reply to Daniel Veditz [:dveditz] from comment #10)

Do we have the same problem of missing initialization in one of multiple constructors with PartiallySeekableInputStream?

https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/netwerk/base/PartiallySeekableInputStream.cpp#43-44,52-54

Yes, great find! I'll send another patch for that.

Flags: needinfo?(valentin.gosu)
Regressed by: 1456995

Comment on attachment 9140554 [details]
Bug 1625749 - Make sure PartiallySeekableInputStream initializes all its members r=kershaw

Same as comment 8

(In reply to Valentin Gosu [:valentin] (he/him) from comment #8)

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Quite easily.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: Bug 1434553
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: Applies cleanly on esr68
  • How likely is this patch to cause regressions; how much testing does it need?: Very low chance of regressions.
Attachment #9140554 - Flags: sec-approval?

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

sec-approval+ and beta uplift approval for 76. We'll also want this on ESR just in case something shifts and makes this reachable there as well, but you should have a separate uplift request for that. We may want to wait a week for the ESR uplift to land. (comment applies to approval for both patches).

Attachment #9140529 - Flags: sec-approval?
Attachment #9140529 - Flags: sec-approval+
Attachment #9140529 - Flags: approval-mozilla-beta+
Attachment #9140554 - Flags: sec-approval?
Attachment #9140554 - Flags: sec-approval+
Attachment #9140554 - Flags: approval-mozilla-beta+

[Tracking Requested - why for this release]: These patches look trivial, and fix an issue which is apparently causing crashing on Twitter (bug 1628624), so we might want to consider this for a ride along if we're doing a dot release anyways.

Group: network-core-security → core-security-release

Please nominate this for ESR approval when you get a chance.

Flags: needinfo?(valentin.gosu)

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high. Uninitialized pointers.
  • User impact if declined: Possible crashes and security issues.
  • Fix Landed on Version: 77
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Initializes previously uninitialized member pointers to nullptr.
  • String or UUID changes made by this patch:
Flags: needinfo?(valentin.gosu)
Attachment #9140529 - Flags: approval-mozilla-esr68?
Attachment #9140554 - Flags: approval-mozilla-esr68?

Comment on attachment 9140529 [details]
Bug 1625749 - Make sure SlicedInputStream initializes all its members r=nika

Approved for 68.8esr.

Attachment #9140529 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9140554 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-critsmash-triage]
Whiteboard: [necko-triaged][post-critsmash-triage] → [necko-triaged][post-critsmash-triage][adv-main76+r]
Whiteboard: [necko-triaged][post-critsmash-triage][adv-main76+r] → [necko-triaged][post-critsmash-triage][adv-main76+r][adv-ESR68.8+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.