alert() text is cut off by null byte

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
--
minor
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Jesse Ruderman, Assigned: sciguyryan)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
Steps to reproduce:
1. Load data:text/html,<script>x = "foo%00bar"; alert("x.length == " + x.length
+ "\nx is " + x + ".");</script>

Result: the last line of the alert is "x is foo"

Expected: the last line of the alert is "x is foobar." or "x is foo?bar."
http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsIPromptService.idl#78

So, given that we don't really want to modify nsIPromptService, the caller will
need to remove null characters or something if we want to fix this bug...
OS: MacOS X → All
Hardware: Macintosh → All

Comment 2

13 years ago
Created attachment 202958 [details]
Reproduces reported bug and also #50348

Confirmed on Firefox 1.0.7
Why do we care?  I'm pretty tempted to wontfix this...
A simple fix would be to make nsGlobalWindow::Alert strip any null characters before passing on the string.
Why just alert?  What about prompt?  What about other places like this where we use wstring APIs?
Good point. Ideally we should change it in the central place, but if we can't due to frozen APIs, I'm fine with either patching the callsites exposed to webauthors, or leaving as is.

Comment 7

12 years ago
This bug also affects IE, and Opera.  It forms a small security risk since it opens an injection hole where user-provided text which the programmer thinks is safe can terminate the remainder of the message.  Thus allowing for alternate dialogs.

Comment 8

12 years ago
Created attachment 251219 [details]
Demonstration of truncated alert(), confirm() and prompt()

Comment 9

12 years ago
Where would this pose a security risk? I don't see any with the above three functions, since all they do is display user-inputted text. Thus, in the demonstration attachment, they show the expected result.

I don't know why one would put a null character in the middle of a string, especially a literal, but I don't see a reason to change the behaviour just because it _might_ pose a risk; and if someone did rely on this behaviour, it would break compatibility with the other aforementioned browsers.
Any vulnerability would come if browser code concatenated user input containing a null with additional text and hiding the additional text somehow encouraged users to do the wrong thing (confirm), or was able to hide the details informing the user about their own evilness (alert).

Seems remote (I did a quick check and nothing popped out at me), but wouldn't hurt to strip nulls in the prompt service just in case some new feature gets added or some addon does something dangerous.
We can't strip nulls in the prompt service.  See comment 1.  All the prompt service APIs take wstring, so by the time we're in the prompt service we're SOL for nulls.

Comment 12

12 years ago
You can't strip null BYTES in a wstring, but you can strip null /characters/ (wstrings are only terminated by a null character anyway, iirc), can't you?  This of course requires that you know the length of the string data without having to check for a null-terminator.
Yes, yes.  But we do NOT know the length on the callee side.  That's the whole beauty of the crappiness of using wstring in IDL.  If we knew the length this would all be a non-issue and we would have fixed this bug months ago.
(Assignee)

Updated

12 years ago
Assignee: general → bugs
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

12 years ago
Created attachment 252489 [details] [diff] [review]
Possible fix v1

Possible patch v1

* This patch creates a function called |StripNullChars| that simply copies all non-null characters to an output string. This new sting is then used in place of the original.

This probably isn't the most effective way of doing this but suggestion as to a better method of doing this are welcome. This patch works on attachment 251219 [details].
Attachment #252489 - Flags: review?(jonas)
Comment on attachment 252489 [details] [diff] [review]
Possible fix v1

>  *   Mark Hammond <mhammond@skippinet.com.au>
>+  *   Ryan Jones <sciguyryan@gmail.com>

Please fix the indentation.

>+void StripNullChars(const nsAString& inStr,
>+                    nsAString& outStr)
>+{
>+  const PRUnichar *copy = ToNewUnicode(inStr);
>+  PRInt32 len = inStr.Length();
>+  for (PRInt32 i = 0; i < len; i++) {
>+    if (copy[i] != '\0')
>+      outStr.Append(copy[i]);
>+  }
>+}

This is very wasteful since you're actually doing two copies (and leaking one of them). Just loop through inStr directly rather than creating the copy by using BeginReading().

Also, please follow naming conventions and call the arguments aInStr and aOutStr.

Looks good otherwise, but please attach a new patch since string stuff is tricky :)
Attachment #252489 - Flags: review?(jonas) → review-
(Assignee)

Comment 16

12 years ago
Thanks for the quick review Jonas. I'll get a new patch attached as soon as I can :)
(Assignee)

Comment 17

12 years ago
Created attachment 252540 [details] [diff] [review]
Patch v1.1

Patch v1.1

* Updated too Jonas' comments.

I could probably make StripNullChars return a character array come to think of it but I'm not sure how much difference it would make.
Attachment #252489 - Attachment is obsolete: true
Attachment #252540 - Flags: review?(jonas)
Comment on attachment 252540 [details] [diff] [review]
Patch v1.1

Looks good to me. Returning a char-array wouldn't really help since you need an nsString with the new value anyway.

FWIW, had this been performance critical code (which it is not) it would probably have been faster to have a fast-path that simply assigns aInStr to aOutStr when there are no null chars. This is faster since we refcount string buffers which would avoid the extra allocation.

Giving bz the sr to make sure that we're covering all
Attachment #252540 - Flags: superreview?(bzbarsky)
Attachment #252540 - Flags: review?(jonas)
Attachment #252540 - Flags: review+
Comment on attachment 252540 [details] [diff] [review]
Patch v1.1

Actually, this looks good to me.
Attachment #252540 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 252540 [details] [diff] [review]
Patch v1.1

hrm, wouldn't hurt to get a second set of eyes on this given that it's a security bug. Anyone knows the prompt service well enough feel free to sr.
Attachment #252540 - Flags: superreview+ → superreview?(bzbarsky)
perhaps this should use an nsAutoString instead of an nsString? I'd assume that most strings used for alert() are pretty short.
(Assignee)

Comment 23

12 years ago
(In reply to comment #22)
> perhaps this should use an nsAutoString instead of an nsString? I'd assume that
> most strings used for alert() are pretty short.
> 

No problem. Simple change :) I'll attach a new patch before requesting checkin. I've also got to make |StripNullChars| follow the file's style by placing the return type for the function on its own line. Missed that before too.
Comment on attachment 252540 [details] [diff] [review]
Patch v1.1

>Index: dom/src/base/nsGlobalWindow.cpp

>+void StripNullChars(const nsAString& aInStr,
>+                    nsAString& aOutStr)

static, please?

And make aOutStr an nsString& here.

>+  aInStr.BeginReading(start); 
>+  aInStr.EndReading(end); 
>+
>+  while (start != end) {
>+    if (*start != '\0')
>+      aOutStr.Append(*start);
>+    ++start;

So if we decide we care about how this peforms, we could probably do better by appending in chunks instead of char by char...  Probably doesn't matter much, since the string buffer doesn't grow linearly (so you only get O(N log N) growth here).

It might still be nice to just do a single Append() in the common case of no nulls (since that should just refcount the buffer and do no copying); up to you, I guess.

sr=bzbarsky
Attachment #252540 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 25

12 years ago
Created attachment 253856 [details] [diff] [review]
Patch v2

Patch v2

* Addresses biesi's nit about using |nsAutoString| in place of |nsString|. Addresses bz 's comments. Made |StripNullChars| static and added a "quick escape" for strings that contain 0 characters or one if its at the end of the string.
Attachment #252540 - Attachment is obsolete: true
Attachment #253856 - Flags: superreview?(bzbarsky)
Attachment #253856 - Flags: review?(jonas)
(Assignee)

Updated

12 years ago
Attachment #253856 - Flags: superreview?(bzbarsky)
Attachment #253856 - Flags: review?(jonas)
(Assignee)

Comment 26

12 years ago
Created attachment 253857 [details] [diff] [review]
Patch v2

Same as patch 2 without the spelling error.
Attachment #253856 - Attachment is obsolete: true
Attachment #253857 - Flags: superreview?(bzbarsky)
Attachment #253857 - Flags: review?(jonas)
Comment on attachment 253857 [details] [diff] [review]
Patch v2

+      (nullCount == 1 && aInStr.Last() == '\0')) {

no... this isn't how strings work. you don't get to see the final 0.
Attachment #253857 - Flags: review-
Comment on attachment 253857 [details] [diff] [review]
Patch v2

oh, or was this for the case of JS passing "foo\0"?
Attachment #253857 - Flags: review-
(Assignee)

Comment 29

12 years ago
(In reply to comment #28)
> (From update of attachment 253857 [details] [diff] [review])
> oh, or was this for the case of JS passing "foo\0"?
> 

Yea. Seeing as it won't have any affect on the visible string there is little point going through the trouble of looping through every character to remove them :)
Comment on attachment 253857 [details] [diff] [review]
Patch v2

>Index: dom/src/base/nsGlobalWindow.cpp
>+StripNullChars(const nsAString& aInStr,
>+  if ((nullCount == 0) ||

That's overparenthesized.  Take out the extras?

sr=bzbarsky with that
Attachment #253857 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 31

12 years ago
(In reply to comment #30)
> (From update of attachment 253857 [details] [diff] [review])
> >Index: dom/src/base/nsGlobalWindow.cpp
> >+StripNullChars(const nsAString& aInStr,
> >+  if ((nullCount == 0) ||
> 
> That's overparenthesized.  Take out the extras?

Can do. Assuming a r+ from Sicking I'll re-attach a patch for checkin.
Comment on attachment 253857 [details] [diff] [review]
Patch v2

>Index: dom/src/base/nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
>retrieving revision 1.901
>diff -u -8 -p -r1.901 nsGlobalWindow.cpp
>--- dom/src/base/nsGlobalWindow.cpp	23 Jan 2007 00:48:02 -0000	1.901
>+++ dom/src/base/nsGlobalWindow.cpp	3 Feb 2007 14:32:04 -0000
>@@ -23,16 +23,17 @@
>  * Contributor(s):
>  *   Travis Bogard <travis@netscape.com>
>  *   Brendan Eich <brendan@mozilla.org>
>  *   David Hyatt (hyatt@netscape.com)
>  *   Dan Rosen <dr@netscape.com>
>  *   Vidur Apparao <vidur@netscape.com>
>  *   Johnny Stenback <jst@netscape.com>
>  *   Mark Hammond <mhammond@skippinet.com.au>
>+ *   Ryan Jones <sciguyryan@gmail.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either of the GNU General Public License Version 2 or later (the "GPL"),
>  * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -316,16 +317,40 @@ IsAboutBlank(nsIURI* aURI)
>     return PR_FALSE;
>   }
>     
>   nsCAutoString str;
>   aURI->GetSpec(str);
>   return str.EqualsLiteral("about:blank");  
> }
> 
>+static void
>+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 nullCount = aInStr.CountChar('\0');
>+  if ((nullCount == 0) ||
>+      (nullCount == 1 && aInStr.Last() == '\0')) {
>+    aOutStr.Assign(aInStr);
>+    return;
>+  }

Don't do the last-char-is-null optimization. I'd rather get rid of all nulls to be on the safe side in case this function is reused elsewhere. Also, there's no need to optimize for things people really shouldn't be doing.

And use FindChar instead of CountChar.

r=me with that
Attachment #253857 - Flags: review?(jonas) → review+
(Assignee)

Comment 33

12 years ago
Created attachment 254353 [details] [diff] [review]
For checkin

For checkin

* Final patch updated too Jonas' last comments.
Attachment #253857 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
Comment on attachment 254353 [details] [diff] [review]
For checkin

+  PRInt32 nullCount = aInStr.FindChar('\0');

please rename this variable, now that it's no longer a count.
(Assignee)

Comment 35

12 years ago
Created attachment 254431 [details] [diff] [review]
For checkin v2

For checkin v2

* Final patch for checkin updated to Biesi's comment :)
Attachment #254353 - Attachment is obsolete: true

Updated

12 years ago
Flags: blocking1.9? → blocking1.9+

Comment 36

12 years ago
mozilla/dom/src/base/nsGlobalWindow.cpp  1.905
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.