310037 - alert() text is cut off by null byte

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

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."

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

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...

Comment 2

[Randy Aubry]

Attachment: Reproduces reported bug and also #50348

Confirmed on Firefox 1.0.7

Comment 3

[Boris Zbarsky [:bzbarsky]]

Why do we care?  I'm pretty tempted to wontfix this...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

A simple fix would be to make nsGlobalWindow::Alert strip any null characters before passing on the string.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Why just alert?  What about prompt?  What about other places like this where we use wstring APIs?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Neil Fraser]

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

[Neil Fraser]

Attachment: Demonstration of truncated alert(), confirm() and prompt()

Comment 9

[penagate]

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.

Comment 10

[Daniel Veditz [:dveditz]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

[Brian Crowder]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Ryan Jones-Ward [:sciguyryan]]

Attachment: 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].

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 252489 [details] [diff] [review]
Possible fix v1

>  *   Mark Hammond <[e-mail]>
>+  *   Ryan Jones <[e-mail]>

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 :)

Comment 16

[Ryan Jones-Ward [:sciguyryan]]

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

Comment 17

[Ryan Jones-Ward [:sciguyryan]]

Attachment: 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.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

... cases.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 252540 [details] [diff] [review]
Patch v1.1

Actually, this looks good to me.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

perhaps this should use an nsAutoString instead of an nsString? I'd assume that most strings used for alert() are pretty short.

Comment 23

[Ryan Jones-Ward [:sciguyryan]]

(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 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[Ryan Jones-Ward [:sciguyryan]]

Attachment: 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.

Comment 26

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Same as patch 2 without the spelling error.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 253857 [details] [diff] [review]
Patch v2

oh, or was this for the case of JS passing "foo\0"?

Comment 29

[Ryan Jones-Ward [:sciguyryan]]

(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 30

[Boris Zbarsky [:bzbarsky]]

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

Comment 31

[Ryan Jones-Ward [:sciguyryan]]

(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 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 <[e-mail]>
>  *   Brendan Eich <[e-mail]>
>  *   David Hyatt ([e-mail])
>  *   Dan Rosen <[e-mail]>
>  *   Vidur Apparao <[e-mail]>
>  *   Johnny Stenback <[e-mail]>
>  *   Mark Hammond <[e-mail]>
>+ *   Ryan Jones <[e-mail]>
>  *
>  * 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

Comment 33

[Ryan Jones-Ward [:sciguyryan]]

Attachment: For checkin

For checkin

* Final patch updated too Jonas' last comments.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 35

[Ryan Jones-Ward [:sciguyryan]]

Attachment: For checkin v2

For checkin v2

* Final patch for checkin updated to Biesi's comment :)

Comment 36

[Nickolay_Ponomarev]

mozilla/dom/src/base/nsGlobalWindow.cpp  1.905