370860 - A very long URI ('/////...') hangs under phishing protection code

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase coming up.  I dug a bit, and we seem to be hitting GC a lot (at least in one of my builds).  Also, PROT_URLCanonicalizer.fullyDecodeURL_ seems to be popping up.

The URL in this case is 130KB long, for what it's worth.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase -- just click the button to hang

Note: this comes from bug 370445 comment 34 and following.

Comment 2

[Boris Zbarsky [:bzbarsky]]

It might be worth profiling this with venkman too, I guess...

Comment 3

[Tony Chang (Google)]

Attachment: move canonicalizeURL to c++

I'm seeing about 2000ms spent in CanonicalizeURL for the original code with a string of length 100k.  This version takes about 45ms.

This isn't perfect, since the time spent is linear to the length of the URL.  We're discussing possibly changing the protocol with the server so we truncate after a few k of URL.  We still would have regular expressions to check for phishing URLs so we wouldn't lose coverage if we truncate.  IE only handles URLs of about 2k so that seems pretty safe.

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 255872 [details] [diff] [review]
move canonicalizeURL to c++

>--- build/nsToolkitCompsCID.h	19 Oct 2006 02:24:29 -0000	1.21
>+++ build/nsToolkitCompsCID.h	21 Feb 2007 08:46:56 -0000
>+#define NS_URLCLASSIFIERUTILS_CONTRACTID \
>+    "@mozilla.org/url-classifier/urlutils;1"
>+

Seems like just "/utils" would be more consistent with the class name.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ url-classifier/src/nsUrlClassifierUtils.cpp	21 Feb 2007 08:46:56 -0000
>+inline int int_to_hex_digit(int i) {
>+  NS_ASSERTION((i >= 0) && (i <= 15), "int too big in int_to_hex_digit");
>+  return ((i < 10) ? (i + '0') : ((i - 10) + 'A'));
>+}
>+
>+inline int hex_digit_to_int(char c) {
>+  NS_ASSERTION(isxdigit(c), "not a hex digit in hex_digit_to_int");
>+  return ((c >= 'a') ? ((c - 'a') + 10) :
>+          (c >= 'A') ? ((c - 'A') + 10) :
>+          (c - '0'));
>+}

I'd declare these "static", and lose the "inline" unless you know for sure that there's a big benefit to inlining and that the compiler won't do it for some reason.  Also, fix the brace style (beginning brace on a new line).

>+bool
>+nsUrlClassifierUtils::Decode(const nsACString & url, nsACString & _retval)

bool -> PRBool (unless the portability rules have changed recently).  Same in a few other spots.

>+bool
>+nsUrlClassifierUtils::SpecialEncode(const nsACString & url, nsACString & _retval)
>+{
>+  bool changed = false;
>+  const char* curChar = url.BeginReading();
>+  const char* end = url.EndReading();
>+
>+  while (curChar != end) {
>+    unsigned char c = static_cast<unsigned char>(*curChar);

NS_STATIC_CAST, same below and in the unit test.

>+bool
>+nsUrlClassifierUtils::ShouldURLEscape(const unsigned char c) {
>+  return c <=32 || c == '%' || c >=127;

space before 32.  And you could make this method const.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ url-classifier/src/nsUrlClassifierUtils.h	21 Feb 2007 08:46:56 -0000
>+  // Disallow copy constructor
>+  nsUrlClassifierUtils(nsUrlClassifierUtils&);

I think the parameter needs to be "const" to override the built-in copy constructor.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ url-classifier/tests/TestUrlClassifierUtils.cpp	21 Feb 2007 08:46:56 -0000
>@@ -0,0 +1,155 @@

You should probably put the standard license header here.

>+#include <stdio.h>
>+#include <ctype.h>
>+#include "nsString.h"
>+#include "nsUrlClassifierUtils.h"
>+
>+#define STRING_EQUALS(expected, actual) \
>+  { if (!(expected).Equals((actual))) { \
>+      fprintf(stderr, "FAILED: expected |%s| but was |%s|\n", \
>+              (expected).get(), (actual).get()); \
>+    } else { gPassedTests++; } \
>+    gTotalTests++; \
>+  }
>+

As it is, this could probably be a function rather than a macro, but as long as it's a macro, maybe you should use __FILE__ and __LINE__ to indicate where the error happened?  Also, the name of the macro kind of implies that it returns whether the strings are equal.  Maybe better to call it CHECK_EQUALS or something?

>+inline int int_to_hex_digit(int i) {
>+  NS_ASSERTION((i >= 0) && (i <= 15), "int too big in int_to_hex_digit");
>+  return ((i < 10) ? (i + '0') : ((i - 10) + 'A'));
>+}
>+

Same comments as in the other file.

>+void TestDecodeHelper(const char* in, const char* expected) {

brace to next line.

Comment 5

[Tony Chang (Google)]

Attachment: v2: move canonicalizeURL to c++

All changes along with using print32 instead of int

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #4)
> >+bool
> >+nsUrlClassifierUtils::SpecialEncode(const nsACString & url, nsACString & _retval)
> >+{
> >+  bool changed = false;
> >+  const char* curChar = url.BeginReading();
> >+  const char* end = url.EndReading();
> >+
> >+  while (curChar != end) {
> >+    unsigned char c = static_cast<unsigned char>(*curChar);
> 
> NS_STATIC_CAST, same below and in the unit test.

Actually, I think this is okay now; see bug 348748.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 256512 [details] [diff] [review]
v2: move canonicalizeURL to c++

Does it work to just have int_to_hex_digit return a char, instead?  That might save some casting.

Need to apply the PRInt32 fix to the unit test.

Looks good to check in with those changes.

Comment 8

[Tony Chang (Google)]

Attachment: v3: move canonicalizeURL to c++

print32 in unittests and remove casts for int_to_hex_digit

Comment 9

[Darin Fisher]

Comment on attachment 256564 [details] [diff] [review]
v3: move canonicalizeURL to c++

>Index: url-classifier/src/nsUrlClassifierUtils.cpp

>+static char int_to_hex_digit(PRInt32 i)
>+{
>+  NS_ASSERTION((i >= 0) && (i <= 15), "int too big in int_to_hex_digit");
>+  return NS_STATIC_CAST(char, ((i < 10) ? (i + '0') : ((i - 10) + 'A')));
>+}
>+
>+static PRInt32 hex_digit_to_int(char c)
>+{
>+  NS_ASSERTION(isxdigit(c), "not a hex digit in hex_digit_to_int");
>+  return ((c >= 'a') ? ((c - 'a') + 10) :
>+          (c >= 'A') ? ((c - 'A') + 10) :
>+          (c - '0'));
>+}

I'd be surprised if these don't already exist somewhere else in Moz
or NSPR.


>+nsUrlClassifierUtils::CanonicalizeURL(const nsACString & url, nsACString & _retval)
>+{
>+  nsCString decodedUrl(url);
>+  nsCString temp;

nsCAutoString would probably be better here so that you benefit from
stack storage if the strings are short.


>+PRBool
>+nsUrlClassifierUtils::Decode(const nsACString & url, nsACString & _retval)
>+{
>+  PRBool changed = PR_FALSE;
>+
>+  const char* curChar = url.BeginReading();
>+  const char* end = url.EndReading();
>+
>+  _retval.Truncate();
>+  while (curChar != end) {
>+    if (*curChar == '%' && curChar + 1 != end && curChar + 2 != end && 
>+        isxdigit(*(curChar + 1)) && isxdigit(*(curChar + 2))) {
>+      // one hex digit == 4 bits,so need to shift the high hex digit up 4 bits.
>+      char c = (hex_digit_to_int(*(curChar + 1)) << 4 |
>+                hex_digit_to_int(*(curChar + 2)));
>+      _retval.Append(c);
>+      changed = PR_TRUE;
>+
>+      // Skip self plus two hex digits.
>+      curChar += 3;
>+    } else {
>+      _retval.Append(*curChar);
>+      curChar++;
>+    }
>+  }
>+  return changed;
>+}

Did you consider using one of the unescape routines from nsEscape.h?


>+PRBool
>+nsUrlClassifierUtils::SpecialEncode(const nsACString & url, nsACString & _retval)
>+{

It'd be good to document what is special about this encoding ;-)


>Index: url-classifier/src/nsUrlClassifierUtils.h

>+  // This function will encode all "special" characters in typical url encoding,
>+  // that is %hh where h is a valid hex digit.  The characters which are encoded
>+  // by this function are any ascii characters under 32(control characters and
>+  // space), 37(%), and anything 127 or above (special characters).  Url is the
>+  // string to encode, ret is the encoded string.  Function returns true if
>+  // ret != url.
>+  PRBool SpecialEncode(const nsACString & url, nsACString & _retval);

Oh, I see... perhaps a "link" to this comment should be near the implementation
in the .cpp file?

Comment 10

[Tony Chang (Google)]

Attachment: v4: move canonicalizeURL to c++

use nsEscape.h:NS_UnescapeURL instead of Decode, use nsCAutoString.  I didn't see any common code in nsEscape to hex encode an int.  It seems to be done manually in multiple places (similar implementation but uses an array of hex chars instead).

Comment 11

[Darin Fisher]

Comment on attachment 256601 [details] [diff] [review]
v4: move canonicalizeURL to c++

>+class nsUrlClassifierUtils : public nsIUrlClassifierUtils
>+{
>+public:
>+  nsUrlClassifierUtils();
>+  ~nsUrlClassifierUtils() {};

Kill the trailing semi-colon.  It might cause compilation problems on some systems.

sr=darin

Comment 12

[Tony Chang (Google)]

on trunk. branch patch coming up.

Checking in build/nsToolkitCompsCID.h;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsCID.h,v  <--  nsToolkitCompsCID.h
new revision: 1.22; previous revision: 1.21
done
Checking in build/nsToolkitCompsModule.cpp;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v  <--  nsToolkitCompsModule.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in url-classifier/content/enchash-decrypter.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/enchash-decrypter.js,v  <--  enchash-decrypter.js
new revision: 1.12; previous revision: 1.11
done
Checking in url-classifier/content/trtable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/trtable.js,v  <--  trtable.js
new revision: 1.14; previous revision: 1.13
done
Removing url-classifier/content/url-canonicalizer.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/url-canonicalizer.js,v  <--  url-canonicalizer.js
new revision: delete; previous revision: 1.5
done
Checking in url-classifier/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/url-classifier/public/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierUtils.idl,v
done
Checking in url-classifier/public/nsIUrlClassifierUtils.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierUtils.idl,v  <--  nsIUrlClassifierUtils.idl
initial revision: 1.1
done
Checking in url-classifier/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/url-classifier/src/Makefile.in,v  <--  Makefile.in
new revision: 1.12; previous revision: 1.11
done
Checking in url-classifier/src/nsUrlClassifierTable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierTable.js,v  <--  nsUrlClassifierTable.js
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp,v
done
Checking in url-classifier/src/nsUrlClassifierUtils.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp,v  <--  nsUrlClassifierUtils.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.h,v
done
Checking in url-classifier/src/nsUrlClassifierUtils.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.h,v  <--  nsUrlClassifierUtils.h
initial revision: 1.1
done
Checking in url-classifier/tests/Makefile.in;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/TestUrlClassifierUtils.cpp,v
done
Checking in url-classifier/tests/TestUrlClassifierUtils.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/TestUrlClassifierUtils.cpp,v  <--  TestUrlClassifierUtils.cpp
initial revision: 1.1
done
Checking in url-classifier/tests/test_bug356355.xhtml;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/test_bug356355.xhtml,v  <--  test_bug356355.xhtml
new revision: 1.5; previous revision: 1.4
done

Comment 13

[Tony Chang (Google)]

Attachment: patch for branch

same as v4: move canonicalizeURL to C++ sans mochitest bits

Comment 14

[Robert Sayre]

robcee/waldo: are these new c++ tests running in the TUnit tinderbox run?

Comment 15

[Jeff Walden [:Waldo]]

https://bugzilla.mozilla.org/attachment.cgi?id=256601&action=diff#mozilla/toolkit/components/url-classifier/tests/Makefile.in_sec1

...seems to indicate that they are (see CPPSRCS and SIMPLE_PROGRAMS).  Then again, nothing's actually being tested about the programs -- not the return value, not anything printed to stdout/stderr, not that they don't crash, etc. -- so if this tree were to fall, no one would hear it.  (This might even be standard operating procedure for compiled tests; I don't recall seeing any that included some sort of halt-the-build failure mechanism, but I could have just forgotten.)

Comment 16

[Benjamin Smedberg]

Yes, they need to be added explicitly to a make check target.

Comment 17

[Tony Chang (Google)]

Attachment: non-zero return code on error

Following the instructions on the d.m.o, return a non-zero error code when a test fails.
http://developer.mozilla.org/en/docs/How_to_add_a_build-time_test

Comment 18

[Tony Chang (Google)]

Comment on attachment 256775 [details] [diff] [review]
non-zero return code on error

Err, this doesn't quite work. I'll try something else.

Comment 19

[Jeff Walden [:Waldo]]

Returning a non-zero code works fine so long as it's checked.  Prepending running the test with $(EXIT_ON_ERROR) should (untested) suffice to make the build halt for such a failure.  (It expands to |set -e;|, which according to the bash man page causes execution to stop if a command exits with non-zero status.)

Benjamin, is EXIT_ON_ERROR in config/rules.mk a "public" definition for makefiles?  The number of makefiles currently using it is small enough that it's possible they should be using something else.  If it's public, we probably need to prepend that before all compiled-code tests that run so that if they fail they break the build.

Comment 20

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

well, it's not needed for most makefiles - you only need it when you use for loops or something like that (because if one line in a makefile has a nonzero exit status, it fails, so the only cases that need it is if you execute multiple stuff in a single line)

Comment 21

[Tony Chang (Google)]

Attachment: stop build if test fails

This seems to work.  Going to submit since it's just a test change.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Verified.  Time is now less than 3 seconds over here for sure; possibly less than one second.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 256730 [details] [diff] [review]
patch for branch

Once upon a time we tried pushing nsPIFoo for private interfaces, you could have used that for nsPIUrlClassifierUtils to reinforce the "DONT USE THIS DIRECTLY" comment. Never really caught on, but seemed like a useful distinction.

Given the size of the patch we'd like a second review before landing on the branch.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 256730 [details] [diff] [review]
patch for branch

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 25

[Tony Chang (Google)]

on branch

Comment 26

[Daniel Veditz [:dveditz]]

Filed bug 379390 on a very similar issue posted to Full-disclosure

Comment 27

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.4 using :

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.4pre) Gecko/20070508 BonEcho/2.0.0.4pre ID:2007050804

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/2007050105 Firefox/2.0.0.4 Linux Fedora FC 6

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.4) Gecko/2007050106 Firefox/2.0.0.4 (RC1)

and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4) Gecko/2007050106 Firefox/2.0.0.4

no hang on testcase, Firefox works stable and normal - adding verified keyword