Closed Bug 488796 Opened 11 years ago Closed 11 years ago

form history should not save extremely large values

Categories

(Toolkit :: Form Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: Dolske, Assigned: MattN)

References

Details

(Keywords: verified1.9.1, verified1.9.2, Whiteboard: [sg:low?] Persistent DoS?)

Attachments

(1 file, 1 obsolete file)

Satchel currently just blindly stores the input name and value from a form field. We should limit both to some upper limit... Gigantic values could cause performance issues, and are not really useful for autocompleting.

It would be interesting to gather some data from existing history DBs, but I'd guess that reasonable limits would be 50 characters for the fieldname and 200 characters for the field value.
Assignee: nobody → dolske
(In reply to comment #0)
> 50 characters for the fieldname

Looking at my main profile's form history, that seems too few. The longest remembered field name is 146 characters and the longest field with more than one remembered value is 62 characters long. Most of these long field names come from ASP.net apps (ctl00$ContentPlaceHolderApplication...).
Assignee: dolske → mmn100+mozilla
Only store form history of form submission if name and value fields are not longer than 200 characters.
Attachment #375884 - Flags: review?(dolske)
Comment on attachment 375884 [details] [diff] [review]
Limit field name and values to 200

Just a couple of tiny nits...

>+// Limit the length of names and values stored in form history
>+#define MAX_HISTORY_NAME_LEN    200
>+#define MAX_HISTORY_VALUE_LEN    200

No tabs!

>-          if (!name.IsEmpty())
>+          if (!name.IsEmpty() && name.Length() <= MAX_HISTORY_NAME_LEN && value.Length() <= MAX_HISTORY_VALUE_LEN)

I think it would be a little cleaner to read if this had the length checks in a separate if-clause from the isempty check, having each bail out with a |continue| if there's a problem.
Attachment #375884 - Flags: review?(dolske) → review-
Attachment #375884 - Attachment is obsolete: true
Attachment #375903 - Flags: review?(dolske)
Comment on attachment 375903 [details] [diff] [review]
removed tabs and separated if clauses

>+          if(name.Length() > MAX_HISTORY_NAME_LEN 
>+               || value.Length() > MAX_HISTORY_VALUE_LEN)
>+            continue;

The conditional operator should be at the end of the line, like:

    if(name.Length() > MAX_HISTORY_NAME_LEN ||
       value.Length() > MAX_HISTORY_VALUE_LEN)

Otherwise r+, I'll just fix this upon commit so no need for a new patch (unless gavin has additional comments).
Attachment #375903 - Flags: review?(gavin.sharp)
Attachment #375903 - Flags: review?(dolske)
Attachment #375903 - Flags: review+
Comment on attachment 375903 [details] [diff] [review]
removed tabs and separated if clauses

"if (" (with a space), too :)
Attachment #375903 - Flags: review?(gavin.sharp) → review+
(In reply to comment #0)
> Gigantic values could cause performance issues, and are not really useful
> for autocompleting.

Has this ever been a real-world issue? I.e. have we ever actually observed (malicious) sites use forms with huge names/values? And are there really no use cases where a value longer than 200 characters might be reasonable?

To me, this mainly looks like a potential future dataloss issue (and a subtle one at that)...

> It would be interesting to gather some data from existing history DBs

Has this already happened or will this ever happen before the patch lands?
Pushed http://hg.mozilla.org/mozilla-central/rev/0ba276249f03
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #375903 - Flags: approval1.9.1?
(In reply to comment #7)

> Has this ever been a real-world issue? I.e. have we ever actually observed
> (malicious) sites use forms with huge names/values?

That shouldn't matter -- it's a potential issue that could happen, so we should close it off.

> And are there really no use
> cases where a value longer than 200 characters might be reasonable?

I think it's rather unlikely, and is an edge case that we shouldn't design for. [That said, we can certainly tweak the value, as it's a somewhat arbitrary limit.] Form history UI really isn't designed to work with large values -- it's crammed into an autocomplete popup, and doesn't do substring matches.

FWIW, looking at the form history in my daily-use profile, all the long values are basically useless. Some data: image URIs, a number of fields from one site with a bunch of HTML stuck in them (?!), a few bugzilla fields for with enormous CC lists or bug dependency lists, and a few tinyurl(?) values where I had entered gigantic URLs. All of these values were only used once (the initial entry).

The query, in case anyone is interested, was:

SELECT length(value),timesUsed,fieldname,value FROM moz_formhistory ORDER BY length(value);

> To me, this mainly looks like a potential future dataloss issue (and a subtle
> one at that)...

Calling that dataloss is a bit of a stretch, IMO.

> > It would be interesting to gather some data from existing history DBs
> 
> Has this already happened or will this ever happen before the patch lands?

I've got a half-written version of a blog post I've been meaning to post, talking about form expiration and containing a crude version of what the Places Stats Project did (basically, cut'n'paste some code into the JS console, get a window showing some form history stats). I gathered some data a couple of months ago, although not specifically string lengths.
(In reply to comment #9)
> That shouldn't matter -- it's a potential issue that could happen

Is an academic concern is enough to take this without Beta exposure, though?

> Calling that dataloss is a bit of a stretch, IMO.

About as stretchy as calling this an "issue"? ;-) Anyway, who knows what creative use extensions have for form history which might reasonably need more than 200 characters. Maybe that value should be upped to a number of characters you can't see all at once in a very long text field (say 512) - just to be on the safe side?
(In reply to comment #10)
> Anyway, who knows what creative use extensions have for form history which
> might reasonably need more than 200 characters.

For what it's worth, the limit only applies to what form history itself will save in response to form submissions. It doesn't apply to extensions that use nsIFormHistory directly.
Whiteboard: [sg:low?] Persistent DoS?
Comment on attachment 375903 [details] [diff] [review]
removed tabs and separated if clauses

a191=beltzner
Attachment #375903 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b1pre) Gecko/20090925 Namoroka/3.6b1pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.