Closed Bug 549566 Opened 14 years ago Closed 14 years ago

It is possible to cut off beforeunload message by using null bytes

Categories

(Core :: DOM: Events, defect)

x86
Windows 7
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- wanted
status1.9.1 --- ?

People

(Reporter: jwkbugzilla, Assigned: smaug)

Details

Attachments

(1 file, 1 obsolete file)

This is similar to bug 310037 but affects the message popping up for pages with beforeunload event handler. To reproduce copy this into the location bar:

javascript:void(window.onbeforeunload=function(){return "foo\0"})

If you then try to navigate away from the current page you will see the message:

> Are you sure you want to navigate away from this page?
> 
> foo

The last sentence ("Press OK to continue, or Cancel to stay on the current page.") is cut off. I don't think this is a big issue with English but it might be more problematic with other languages, marking it as security sensitive just in case. Stripping null characters in nsDOMBeforeUnloadEvent::SetReturnValue certainly won't do any harm either way.

Firefox 3.6 and Minefield build 20100301 are affected, Internet Explorer 8 is not.
Assignee: nobody → Olli.Pettay
Attached patch patch (obsolete) — Splinter Review
Do the same as what was done in Bug 310037.
Attachment #429764 - Flags: review?(jonas)
Comment on attachment 429764 [details] [diff] [review]
patch

>+nsContentUtils::StripNullChars(const nsAString& aInStr, nsAString& aOutStr)
>+{
>+  // In common cases where we don't have nulls in the
>+  // string we can simple simply bypass the checking code.
>+  PRInt32 firstNullPos = aInStr.FindChar('\0');
>+  if (firstNullPos == kNotFound) {
>+    aOutStr.Assign(aInStr);
>+    return;
>+  }
>+
>+  nsAString::const_iterator start, end;
>+  aInStr.BeginReading(start);
>+  aInStr.EndReading(end);
>+
>+  while (start != end) {
>+    if (*start != '\0')
>+      aOutStr.Append(*start);
>+    ++start;
>+  }
>+}

Would probably be worth doing a

aOutStr.SetCapacity(aInStr.Length() - 1);

call before the last while-loop. Yes, it can waste some memory, but it'll avoid O(n^2) behavior.

r=me with that
Attachment #429764 - Flags: review?(jonas) → review+
I don't think this needs to be a security bug, the buttons really should be self-explanatory given the initial text. I think we do want to fix this on the old branches though.
Group: core-security
status1.9.1: --- → ?
Attached patch SetCapacitySplinter Review
Attachment #429764 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/d6c758c21659
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: