Closed Bug 1715960 Opened 5 years ago Closed 2 years ago

Crash in [@ mozilla::net::nsStandardURL::Release]

Categories

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

Product:
Core
Component:
Networking: HTTP
Type:
defect

Tracking

()

Status:
RESOLVED INCOMPLETE

People

(Reporter: valentin, Assigned: kershaw)

References

Details

(Keywords: crash, sec-moderate, Whiteboard: [necko-triaged][necko-monitor])

Crash Data

Attachments

(3 files)

Bug 1715960 - Add a simple parity check to URLSegment, r=#necko
48 bytes, text/x-phabricator-request
Details | Review
Bug 1715960 - Remove sanity check from nsStandardURL's destructor, r=#necko
48 bytes, text/x-phabricator-request
Details | Review
Odd pattern of ::Release() crashes
78.63 KB, image/png
Details

+++ This bug was initially created as a clone of Bug #1700895 +++
+++ This bug was initially created as a clone of Bug #1675540 +++

Even after bug 1700895 landed, there are a few release assertions triggered.

https://crash-stats.mozilla.org/report/index/c819a90f-5ba7-4537-8a34-ec0790210611

-		this	0x000001fc91413710 {mRefCnt={mValue={...} } mSpec={...} mDefaultPort=0x000001bb ...}	mozilla::net::nsStandardURL *
		nsIFileURL	{...}	nsIFileURL
		nsIStandardURL	{...}	nsIStandardURL
		nsISerializable	{...}	nsISerializable
+		nsISizeOf	{...}	nsISizeOf
		nsISensitiveInfoHiddenURI	{...}	nsISensitiveInfoHiddenURI
+		mRefCnt	{mValue={...} }	mozilla::ThreadSafeAutoRefCnt
-		mSpec	{...}	nsTString<char>
-		nsTSubstring<char>	{...}	nsTSubstring<char>
-		mozilla::detail::nsTStringRepr<char>	{mData=0x000001fcdce95808 <Error reading characters of string.> mLength=0x00000042 mDataFlags=0x0005 ...}	mozilla::detail::nsTStringRepr<char>
+		mData	0x000001fcdce95808 <Error reading characters of string.>	char *
		mLength	0x00000042	unsigned int
		mDataFlags	0x0005	mozilla::detail::StringDataFlags
		mClassFlags	NULL_TERMINATED (0x0002)	const mozilla::detail::StringClassFlags
		mDefaultPort	0x000001bb	int
		mPort	0xffffffff	int
+		mScheme	{mPos=0x00000000 mLen=0x00000005 }	mozilla::net::nsStandardURL::URLSegment
+		mAuthority	{mPos=0x02000008 mLen=0x0000001a }	mozilla::net::nsStandardURL::URLSegment
+		mUsername	{mPos=0x00000008 mLen=0xffffffff }	mozilla::net::nsStandardURL::URLSegment
+		mPassword	{mPos=0x00000008 mLen=0xffffffff }	mozilla::net::nsStandardURL::URLSegment
+		mHost	{mPos=0x00000008 mLen=0x0000001a }	mozilla::net::nsStandardURL::URLSegment
+		mPath	{mPos=0x00000022 mLen=0x00000020 }	mozilla::net::nsStandardURL::URLSegment
+		mFilepath	{mPos=0x00000022 mLen=0x00000011 }	mozilla::net::nsStandardURL::URLSegment
+		mDirectory	{mPos=0x00000022 mLen=0x00000011 }	mozilla::net::nsStandardURL::URLSegment
+		mBasename	{mPos=0x00000033 mLen=0x00000000 }	mozilla::net::nsStandardURL::URLSegment
+		mExtension	{mPos=0x00000033 mLen=0xffffffff }	mozilla::net::nsStandardURL::URLSegment
+		mQuery	{mPos=0x00000034 mLen=0x0000000e }	mozilla::net::nsStandardURL::URLSegment
+		mRef	{mPos=0x00000022 mLen=0xffffffff }	mozilla::net::nsStandardURL::URLSegment
+		mParser	{...}	nsCOMPtr<nsIURLParser>
+		mFile	{...}	nsCOMPtr<nsIFile>
+		mDisplayHost	{...}	nsTString<char>
		mURLType	0x00000002	unsigned int
		mSupportsFileURL	0x00000000	unsigned int
		mCheckedIfHostA	0x00000001	unsigned int

This crash has the URL fields in view.
It's all fine, except mAuthority {mPos=0x02000008 mLen=0x0000001a }

I suspect this is the result of a random bit flip/rowhammer, given the result only differs from the correct value by 1 bit.

Assignee: nobody → valentin.gosu

(In reply to Julien Cristau [:jcristau] from comment #1)

3 crashes so far on 90.0b11, with the extra annotations from bug 1716543:

mLen:17, mScheme (0,5), mAuthority (8,E), mUsername (8,FFFFFFFF), mPassword (8,FFFFFFFF), mHost (408,E), mPath (16,1), mFilepath (16,1), mDirectory (16,1), mBasename (17,0), mExtension (0,FFFFFFFF), mQuery (0,FFFFFFFF), mRef (0,FFFFFFFF)

mHost is 0x408 instead of 0x008
1 bit error.

mLen:19, mScheme (0,5), mAuthority (8,10), mUsername (8,FFFFFFFF), mPassword (8,FFFFFFFF), mHost (8,10), mPath (18,1), mFilepath (C000001,10), mDirectory (C200001,10), mBasename (C000003,10), mExtension (0,FFFFFFFF), mQuery (0,FFFFFFFF), mRef (0,FFFFFFFF)

This is a bit more interesting.

mFilepath is (0xC000001,0x10) instead of (0x18, 0x01)
mDirectory is (0xC200001,0x10) instead of (0x18,0x01)
mBasename is (0xC000003,0x10) instead of (0x18,0x01)

It's possible the error occurred during parsing, or path setter, and the error got propagated to the other segments, but it makes me a bit anxious.

mLen:1D, mScheme (0,5), mAuthority (8,14), mUsername (8,FFFFFFFF), mPassword (8,FFFFFFFF), mHost (8,14), mPath (80001C,1), mFilepath (1C,1), mDirectory (1C,1), mBasename (1D,0), mExtension (0,FFFFFFFF), mQuery (0,FFFFFFFF), mRef (0,FFFFFFFF)

mPath is 0x80001C instead of 0x1C
1bit error

There are a lot of new reports from 90.
Most of them have just a 1 bit error, which seems to be all over the place.
I'm considering adding hamming error correction to the implementation, just to be able to differentiate between random bit flips and logic errors.

There are also a few crashes I've found that are not simply 1 bit off and need investigating:

https://crash-stats.mozilla.org/report/index/b31d251d-047b-496b-a89f-986e80210723#tab-annotations

mLen:14, mScheme (0,5), mAuthority (8,B), mUsername (0,FFFFFFFF), mPassword (0,FFFFFFFF), mHost (8,B), mPath (13,1), mFilepath (13,1), mDirectory (13,1), mBasename (16,0), mExtension (0,FFFFFFFF), mQuery (0,FFFFFFFF), mRef (0,FFFFFFFF)

mBasename.mPos is wrong

https://crash-stats.mozilla.org/report/index/30f53731-bbb6-4ba1-9d7a-aea900210722#tab-annotations

mLen:5, mScheme (4,1), mAuthority (4,0), mUsername (0,FFFFFFFF), mPassword (0,FFFFFFFF), mHost (0,FFFFFFFF), mPath (4,1), mFilepath (4,1), mDirectory (4,1), mBasename (5,0), mExtension (5,FFFFFFFF), mQuery (4,FFFFFFFF), mRef (4,FFFFFFFF)

mScheme.mPos = 4 !?
Ar these reversed, or bit flip?

https://crash-stats.mozilla.org/report/index/0b9a7e22-f9aa-4cf7-ba18-ff3300210802#tab-annotations

mLen:31, mScheme (0,5), mAuthority (8,A), mUsername (0,FFFFFFFF), mPassword (0,FFFFFFFF), mHost (8,A), mPath (12,1F), mFilepath (12,1F), mDirectory (52,15), mBasename (27,5), mExtension (2D,4), mQuery (0,FFFFFFFF), mRef (0,FFFFFFFF)

mDirectory set to 52? Way out of bounds!

The security rating came from the cloned bug, but most current crashes are due to the runtime breakpoints. Is there still a potentially exploitable problem hiding here that isn't caught by the runtime checks in release?

Keywords: csectype-uaf, sec-high

(In reply to Valentin Gosu [:valentin][ooo until 6 Sept] (he/him) from comment #3)

There are a lot of new reports from 90.
Most of them have just a 1 bit error, which seems to be all over the place.
I'm considering adding hamming error correction to the implementation, just to be able to differentiate between random bit flips and logic errors.

I'm afraid of adding hamming error correction may impact the performance., since we need to calculate the code every time when mLen and mPos change.
Maybe we can use simple parity check to filter out those 1 bit error.

Yeah, I wouldn't try to correct errors just in case they're actually intentional/malicious. But detecting errors and reporting (and ideally handling gracefully if it's possible to do so safely) would be fine.

Assignee: valentin.gosu → kershaw
Keywords: leave-open

The leave-open keyword is there and there is no activity for 6 months.
:dragana, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(dd.mozilla)

Kershaw, what wee should do here next? The crashes are not fixed or are this a separate issue?

Flags: needinfo?(dd.mozilla) → needinfo?(kershaw)

I just noticed that almost all crashes in mozilla::net::nsStandardURL::SanityCheck are happend in nsStandardURL's destructor.
This makes me wonder if it's safe to not doing sanity check in destructor, since this problematic object is about to be released.
:dveditz, what do you think?

Flags: needinfo?(kershaw) → needinfo?(dveditz)

I guess you could, but the SanityCheck() crashes aren't that big a deal: If I look at Firefox crashes in the last month I see 19 SanityCheck() crashes and 839 crashes in Release() itself. Of those 19, all were called from Release(). I picked Firefox so I could ignore the single Fenix 99.2.0 installation that crashed 100 times on the same day with no usable stack.

Most of the SanityCheck() calls are made when you construct or modify the URL, and these are all apparently succeeding (at least according to my 1-month sample). Then the URLs hang around for a while, and when we release them we detect a small number that are damaged. Somehow. It's kind of interesting to know that's happening, but doesn't help us figure out when, where, or how the URL got damaged. Does that fact that these URLs get damaged shed any light on why we might have the much larger number of crashes in Release? Maybe the SanityCheck is catching ones where the URL spec is stomped, but for people with whatever-is-happening maybe it's more likely to damage other parts of the nsStandardURL?

So sure, you can drop the call to SanityCheck() from the destructor.

The crash pattern in mozilla::net::nsStandardURL::Release is interesting: 93 and 94 were really crashy, 95 and 96 were great, and then 97+ were crashy again. On the Socorro-lens link set the selection to "release" to weed out ESR-91 background noise and you'll see the difference is quite stark.

Oh, I see! Most of the ::Release() crashes are also the SanityCheck() breakpoint. They are all reported against the NS_IMPL_RELEASE(nsStandardURL) line instead, but many are breakpoints with the same MOZ_CRASH reason. If I filter those out there are 203 crashes doing something else during the destructor. Most are read and write access violations, and some look like use-after-frees rather than corrupt memory (for example, bp-7ed0a430-a878-4284-af34-e9b730220419)

In that light, definitely get rid of the SanityCheck() in the destructor. We don't know what to do with that data, and getting rid of it will be a decent stability win: ~28 crashes a day reduced to ~7.

Flags: needinfo?(dveditz)

Most of the SanityCheck() calls are made when you construct or modify the URL, and these are all apparently succeeding (at least according to my 1-month sample). Then the URLs hang around for a while, and when we release them we detect a small number that are damaged. Somehow. It's kind of interesting to know that's happening, but doesn't help us figure out when, where, or how the URL got damaged. Does that fact that these URLs get damaged shed any light on why we might have the much larger number of crashes in Release?

Because we added a simple parity check to filter out those strange 1-bit flip errors in early beta and nightly. That's why we've seen higher number of crashes in Release.

Oh, I see! Most of the ::Release() crashes are also the SanityCheck() breakpoint. They are all reported against the NS_IMPL_RELEASE(nsStandardURL) line instead, but many are breakpoints with the same MOZ_CRASH reason. If I filter those out there are 203 crashes doing something else during the destructor. Most are read and write access violations, and some look like use-after-frees rather than corrupt memory (for example, bp-7ed0a430-a878-4284-af34-e9b730220419)

In that light, definitely get rid of the SanityCheck() in the destructor. We don't know what to do with that data, and getting rid of it will be a decent stability win: ~28 crashes a day reduced to ~7.

Thanks for the feedback. I'll submit a patch to do this.

I'm really curious about that ~2 month gap where the crashes were pretty low. Hard to believe our audience changed behavior (or hardware!) so much—and then switched back? Unless you know the parity check escaped to those two Releases and accounts for that, it might be a clue to something Firefox is doing that's trashing nsStandardURL memory. (Might not have anything to do with any networking changes, so not much to go on.)

The leave-open keyword is there and there is no activity for 6 months.
:kershaw, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(kershaw)
See Also: → 1825645

I've looked at some recent crashes. Unfortunately, those reports are not actionable because they are either caused by bit-flips or some odd values in URL segments (could be a result of a wild pointer or memory corruption).
I'll add a stalled flag and put this in our monitor queue.

Flags: needinfo?(kershaw)
Keywords: stalled
Whiteboard: [necko-triaged] → [necko-triaged][necko-monitor]
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit BugBot documentation.

Keywords: stalled
Group: network-core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: