1313244 - string buffer overrun in url unescape

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Douglas Bagnall]

Attachment: 319511.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160920074044

Steps to reproduce:

Following Thunderbird BUG 1310467#19, I had a quick look for similar problems. There was this one.

The effect is that url-unescaping can easily wander beyond the end of its string, in the process shifting bytes leftward and plunking a NUL byte in a random place.

It may be that this function is not actually used anywhere -- there are several seemingly more sophisticated unescape functions hanging around.

This is in mozilla-central.

Comment 1

[Andrew McCreight [:mccr8]]

Marking sec-high because it is a write, and this method is called from a bunch of places.

Comment 2

[Douglas Bagnall]

I have asked  BUG 1310467 to wait before committing.

There are 524 repositories on github and a few more revealed by google that contain the comment starting "Remove URL hex escapes from s..." (the variant from BUG 1310467). Many of these have fixed the bug, but many haven't (including e.g. opensolaris and illumos). 256 repositories have the "/* walk over escape */" found here, which mostly seem to be Mozilla forks.

https://codesearch.debian.net/search?q=if+%5C%28+%5C*%5C%2B%5C%2Bs+%21%3D+%27%5C%5C0%27+%5C%29+ finds only icedove and 389-dsgw. Searching for "/* walk over escape */" finds only firefox and firefox-esr.

Comment 3

[Douglas Bagnall]

I have informed Red Hat Product Security about the version of the bug in 389-dsgw. They may join in here to coordinate things.

I couldn't find a security contact for illumos.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Looks like this issue goes back to when this file was added in 1998. Douglas, did you intend to request review on this patch? Nathan Froyd (:froydnj) might be a good choice.

Comment 5

[Marcia Knous [:marcia]]

Tracking 52+ for this sec high issue.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8804944 [details] [diff] [review]
319511.patch

Review of attachment 8804944 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch.  The commit message references nsEscape, but the patch actually modifies nsUnescapeCount.  This is just a typo in the commit message, right?

I don't think this code has the same issues as bug 1310467, though, because the check to see whether we can unescape things is much stricter than the LDAP code.  I think NS_UnescapeURL is similarly more strict, so there's no issue there as well.

For testing out my comments below, I added:

TEST(EscapeURL, BadEscapeSequences)
{
  {
    char bad[] = "%s";

    int32_t count = nsUnescapeCount(bad);
    EXPECT_EQ(count, 2);
    EXPECT_STREQ(bad, "%s");
  }
  {
    char bad[] = "%a";
    int32_t count = nsUnescapeCount(bad);
    EXPECT_EQ(count, 2);
    EXPECT_STREQ(bad, "%a");
  }
  {
    char bad[] = "%";
    int32_t count = nsUnescapeCount(bad);
    EXPECT_EQ(count, 1);
    EXPECT_STREQ(bad, "%");
  }
  {
    char bad[] = "%s/%s";
    int32_t count = nsUnescapeCount(bad);
    EXPECT_EQ(count, 5);
    EXPECT_STREQ(bad, "%s/%s");
  }
}

to xpcom/tests/gtest/TestEscapeURL.cpp, in an attempt to mimic some of the problematic cases suggested by bug 1310467.  I ran |mach gtest EscapeURL.\*|, using an ASan build to try and catch out of bounds writes.  All the tests passed.  I suppose really thorough testing might include ensuring that bytes beyond the trailing null aren't modified in any way, but I was thinking that ASan would catch such cases...

Comments welcome if you see a flaw in my reasoning!

::: xpcom/io/nsEscape.cpp
@@ +186,5 @@
>        c2[0] = *(src + 2);
>      }
>  
>      if (*src != HEX_ESCAPE || PL_strpbrk(pc1, hexCharsUpperLower) == 0 ||
>          PL_strpbrk(pc2, hexCharsUpperLower) == 0) {

This check prevents us from falling into the unescape path for things like "%s", or "%a", or just plain "%": we've carefully copied *(src + 1) and, if necessary, *(src + 2) into *pc1 and *pc2 so we can check to see whether we have a proper hex escape.

@@ +190,5 @@
>          PL_strpbrk(pc2, hexCharsUpperLower) == 0) {
>        *dst++ = *src++;
>      } else {
>        src++; /* walk over escape */
> +      if (*src == '\0') {

By the time we get to this check, we know that *src and *(src + 1) must be hex characters, so this condition can't ever be true, and the check this code is replacing couldn't ever be true either.

Comment 7

[Douglas Bagnall]

> Comments welcome if you see a flaw in my reasoning!

No, you are quite right. Sorry for the erroneous report, and thanks for the review.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> For testing out my comments below, I added TEST(EscapeURL, BadEscapeSequences)
> to xpcom/tests/gtest/TestEscapeURL.cpp

Would it be worth adding these officially as a sanity check? Though I recognize that this code hasn't changed significantly in nearly 20 years :)

Also, I assume we want to wait for bug 1310467 to land before opening this one up?

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> (In reply to Nathan Froyd [:froydnj] from comment #6)
> > For testing out my comments below, I added TEST(EscapeURL, BadEscapeSequences)
> > to xpcom/tests/gtest/TestEscapeURL.cpp
> 
> Would it be worth adding these officially as a sanity check? Though I
> recognize that this code hasn't changed significantly in nearly 20 years :)

Yes!  I think those additions should be a separate bug, though.

> Also, I assume we want to wait for bug 1310467 to land before opening this
> one up?

Yes, I think so.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> Yes!  I think those additions should be a separate bug, though.

Filed bug 1316127 with a reasonably generic summary :)