Last Comment Bug 364112 - After submitting a huge string in a textbox, profile becomes corrupted such that typing into any textbox hangs
: After submitting a huge string in a textbox, profile becomes corrupted such t...
[sg:low] persistent DoS [need branch ...
: fixed1.8.0.12, fixed1.8.1.4, hang, perf
Product: Toolkit
Classification: Components
Component: Form Manager (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
: Matthew N. [:MattN] (PM me if requests are blocking you)
: 371436 378159 (view as bug list)
Depends on:
Blocks: profile-corrupt
  Show dependency treegraph
Reported: 2006-12-17 02:29 PST by Marcel
Modified: 2008-07-31 04:30 PDT (History)
12 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase, induces delay at Google (644 bytes, text/html)
2007-03-07 17:34 PST, Daniel Veditz [:dveditz]
no flags Details
don't save form data that's too large (3.77 KB, patch)
2007-04-24 03:29 PDT, Daniel Veditz [:dveditz]
asaf: review-
Details | Diff | Splinter Review
fix mork-based history only (2.84 KB, patch)
2007-04-24 13:57 PDT, Daniel Veditz [:dveditz]
asaf: review+
Details | Diff | Splinter Review
1.8/1.8.0 branch patch (lengths changed per review comments) (2.44 KB, patch)
2007-04-27 21:59 PDT, Daniel Veditz [:dveditz]
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review
ignore -- double submit (2.44 KB, patch)
2007-04-27 22:02 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review

Description Marcel 2006-12-17 02:29:47 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1) Gecko/20061010 Firefox/2.0

On a site, there was an inputfield. Well, in notepad i typed a lot of charachters, millions perhaps. I pasted this line in an input field and posted it to the server. It took so long that the browsers hanged. From that moment, i cannot type in any input field in my profile. On the first charachter, firefox hangs and i am forced to do a ctrl+alt+del. This problem does not happen when i use a new profile

Reproducible: Always

Steps to Reproduce:
1.Paste lots of text in an input field (so no textarea)
2.Post form
3.Restarted firefox
4.Do whatever, a search in google orso. My firefox (profile) hangs

Actual Results:  
Crash, not responding
Comment 1 Marcel 2006-12-17 03:35:44 PST
Did some more testing
I made a backup of my profile using mozbackup. I checked all except "saved form details". Then resored the backup, and now i dont have any problems now...

Maybe this additional information will help
Comment 2 Jesse Ruderman 2007-02-23 18:54:25 PST
*** Bug 371436 has been marked as a duplicate of this bug. ***
Comment 3 Daniel Veditz [:dveditz] 2007-03-07 17:29:19 PST
I don't see how this can block a specific 1.8 release without some traction on a fix. Nominating for the trunk release and moving into the "wanted" bucket for the branch.

You hang on any input field? I was only able to reproduce hanging on input fields with the same name as my testcase (e.g. delay on Google, Yahoo still fine).
Comment 4 Daniel Veditz [:dveditz] 2007-03-07 17:34:11 PST
Created attachment 257780 [details]
testcase, induces delay at Google
Comment 5 Daniel Veditz [:dveditz] 2007-03-07 17:52:46 PST
During the hang stacks tend to look like the following. Mork is growing the buffer 512 bytes at a time until it gets big enough to read the string value, which is going to take 32K allocs until it's big enough for the 16Mb string. I think the hope is to replace Mork, so meanwhile we should put a sane upper bound on saved form data of a couple of K.

>	memset() Line 108	Asm
 	_heap_alloc_dbg() Line 448	C
 	_nh_malloc_dbg() Line 260	C
 	_nh_malloc() Line 209	C
 	operator new() Line 25	C++
 	orkinHeap::Alloc() Line 90	C++
 	morkBlob::GrowBlob() Line 90	C++
 	morkCoil::GrowCoil() Line 164	C++
 	morkSpool::SpillPutc() Line 127	C++
 	morkSink::Putc() Line 132	C++
 	morkParser::ReadValue() Line 938	C++
 	morkParser::ReadCell() Line 587	C++
 	morkParser::ReadRow() Line 678	C++
 	morkParser::ReadTable() Line 789	C++
 	morkParser::ReadContent() Line 1432	C++
 	morkParser::OnPortState() Line 1473	C++
 	morkParser::ParseLoop() Line 1531	C++
 	morkParser::ParseMore() Line 1570	C++
 	morkThumb::DoMore_OpenFileStore() Line 479	C++
 	morkThumb::DoMore() Line 399	C++
 	morkThumb::DoMore() Line 194	C++
 	nsFormHistory::UseThumb() Line 588	C++
 	nsFormHistory::OpenExistingFile() Line 441	C++
 	nsFormHistory::OpenDatabase() Line 381	C++
 	nsFormHistory::AutoCompleteSearch() Line 722	C++
 	nsFormFillController::StartSearch() Line 556	C++
 	nsAutoCompleteController::StartSearch() Line 993	C++
 	nsAutoCompleteController::Notify() Line 688	C++
 	nsTimerImpl::Fire() Line 397	C++
 	nsTimerManager::FireNextIdleTimer() Line 628	C++
 	nsAppShell::Run() Line 141	C++
 	nsAppStartup::Run() Line 151	C++
 	XRE_main() Line 2695	C++
 	main() Line 61	C++
 	mainCRTStartup() Line 398	C

Cure once affected: use clear private data to delete form history.
Comment 6 Jesse Ruderman 2007-03-07 18:03:42 PST
> Mork is growing the buffer 512 bytes at a time until it gets big enough to read the string value

Reminds me of bug 319004 comment 5.
Comment 7 Daniel Veditz [:dveditz] 2007-03-07 18:14:18 PST
Apparently you need to quit and restart to get the full effect of the hang, the current 16Mb string version might be far too malicious. (note, mork actually saves the data as escaped UTF-16 so a single 'a' is saved as 'a$00' -- quadrupling the amount it needs to re-re-re-reallocate, 512 bytes at a time.)

If you edit the testcase to set the value to 's' instead of 's16' the resulting hang is bearable. It does happen on the first input field regardless of the site, but once the form data is loaded into history there aren't further delays.
Comment 8 :Gavin Sharp [email:] 2007-04-18 16:34:48 PDT
Hmm, was this not fixed by bug 319004? Either way, we use mozstorage for form history on the trunk, so this bug doesn't apply there.
Comment 9 Daniel Veditz [:dveditz] 2007-04-19 16:40:25 PDT
No, it was not fixed by bug 319004. That hacked around the mork problem by limiting title strings specifically; it did not fix the underlying mork issue for other uses of mork. The branch fix I'm proposing here is a similar hack-around in the form-data code. The trunk "fix" is to stop using mork.

Not sure what Thunderbird/mailnews needs to worry about since they're still using mork on the trunk. A malicious vcard with arbitrary sized values? locally-saved IMAP mail?
Comment 10 Daniel Veditz [:dveditz] 2007-04-24 03:29:20 PDT
Created attachment 262627 [details] [diff] [review]
don't save form data that's too large

Prevent the branch DOS by limiting saved form data to 200 characters for the field ID and 2000 for the <input> text. Anything bigger and we simply don't save it. The limits are totally arbitrary, but it's hard to imagine a legit ID being 200 characters, or a 2000-char data string that you'd want to replay in future forms.

For the trunk I've kept the limits even though we now use mozstorage which doesn't have the performance suckage of mork. Large chunks of data still seem useless/malicious and not worth storing.
Comment 11 :Gavin Sharp [email:] 2007-04-24 07:07:12 PDT
I don't think we should arbitrarily limit what we store on the trunk unless we have a good reason to.
Comment 12 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-04-24 09:52:04 PDT
Comment on attachment 262627 [details] [diff] [review]
don't save form data that's too large

What gavin said, plus  I would rather cut aValue at its maximum length rather than just throwing (for "invalid" name we should probably keep throwing, I guess).
Comment 13 Daniel Veditz [:dveditz] 2007-04-24 13:46:57 PDT
The "good reason to" is because sites can store arbitrarily large amounts of data on disk. Although I guess even with a size limit they can just add more fields, so we'd really need a per-site quota of some sort (much like the cookie and globalStorage quotas). And there's no way to manage form data or even know how much you have other than periodically trashing it all. But OK, I'll leave the trunk alone and let that be a different issue (I'd still like to fix the potential uninitialized "exists" variable).

On the branch I don't see the value of saving truncated data. It's chopped off but you're not going to be able to tell that -- if users see the value as a choice they will naturally assume it's what they entered and could blame us for corrupted data. Much better to simply drop it (this is also the approach we take with overlarge cookies).

If you want larger limits I don't mind. 4K, 8K, 16K, whatever. Uselessly large but not yet close to causing the perf problem.
Comment 14 Daniel Veditz [:dveditz] 2007-04-24 13:57:58 PDT
Created attachment 262683 [details] [diff] [review]
fix mork-based history only

in nsFormHistory.cpp the return value doesn't matter since nothing checks it. Should someone want this in nsStorageFormHistory.cpp I'd probably want to return NS_OK so they just silently drop without bothering the caller.
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-04-27 13:40:38 PDT
Comment on attachment 262683 [details] [diff] [review]
fix mork-based history only

>Index: toolkit/components/satchel/src/nsFormHistory.cpp
>RCS file: /cvsroot/mozilla/toolkit/components/satchel/src/nsFormHistory.cpp,v
>retrieving revision 1.37
>diff -u -p -8 -r1.37 nsFormHistory.cpp
>--- toolkit/components/satchel/src/nsFormHistory.cpp	18 Jul 2006 00:18:32 -0000	1.37
>+++ toolkit/components/satchel/src/nsFormHistory.cpp	24 Apr 2007 20:54:18 -0000
>@@ -68,16 +68,20 @@ static void SwapBytes(PRUnichar* aDest, 
>     PRUnichar aChar = *aSrc++;
>     *aDest++ = (0xff & (aChar >> 8)) | (aChar << 8);
>   }
> }
> #define PREF_FORMFILL_BRANCH "browser.formfill."
> #define PREF_FORMFILL_ENABLE "enable"
>+// upper bounds on saved form data, more isn't useful.
>+#define FORMFILL_NAME_MAX_LEN  200

Sold, just increase those a bit (esp. the former); r=mano.
Comment 16 Daniel Veditz [:dveditz] 2007-04-27 21:59:53 PDT
Created attachment 263092 [details] [diff] [review]
1.8/1.8.0 branch patch (lengths changed per review comments)
Comment 17 Daniel Veditz [:dveditz] 2007-04-27 22:02:26 PDT
Created attachment 263093 [details] [diff] [review]
ignore -- double submit
Comment 18 Daniel Veditz [:dveditz] 2007-04-27 22:28:41 PDT
Checked into trunk
Comment 19 Daniel Veditz [:dveditz] 2007-04-30 10:14:35 PDT
Comment on attachment 263092 [details] [diff] [review]
1.8/1.8.0 branch patch (lengths changed per review comments)

approved for and, a=release-drivers
Comment 20 Daniel Veditz [:dveditz] 2007-04-30 11:27:06 PDT
fixed on 1.8 and 1.8.0 branches
Comment 21 Myk Melez [:myk] [@mykmelez] 2007-07-06 15:24:02 PDT
*** Bug 378159 has been marked as a duplicate of this bug. ***
Comment 22 Hasham 2008-06-23 10:49:31 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008062306 GranParadiso/3.0.1pre


Note You need to log in before you can comment on or make changes to this bug.