493664 - 1 byte overflow in nr_NextName

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Brian Hackett [Laid off!]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10

Function nr_NextName in modules/libreg/src/reg.c can have a write overflow of 1 null byte.  The relevant code is pasted below.

static REGERR nr_NextName(const char *pPath, char *buf, uint32 bufsize, const char **newPath)
{
    uint32 len = 0;
    REGERR err = REGERR_OK;
...
    while ( *pPath != '\0' && *pPath != PATHDEL )
    {
        if ( len == bufsize ) {
            err = REGERR_NAMETOOLONG;
            break;
        }
        if ( *pPath < ' ' && *pPath > 0 )
            return REGERR_BADNAME;

        *buf++ = *pPath++;
        len++;
    }
    *buf = '\0';
...
}

If the 'len == bufsize' test passes, or the 'while' test fails when len == bufsize, then buf[bufsize] will be written with 0.  This function is called with stack buffers by nr_Find and nr_RegAddKey (both are in reg.c).


Reproducible: Always

Comment 1

[Daniel Veditz [:dveditz]]

> Reproducible: Always

Does that mean you've got a testcase that triggers this flaw?

This code has been ripped out of Firefox 3.5, and is no longer used by XPInstall in Firefox 3.0. It'd be pretty easy to make an old-style install.js installer hit this in Firefox 2.0, but then you're an installer so if you had evil designs you wouldn't bother with this.

Maybe InstallTrigger.GetVersion() or InstallTrigger.CompareVersion() would be able to trigger this on a whitelisted domain in Firefox 2.0 and earlier. If someone found an XSS flaw on addons.mozilla.org then they might be able to use this to attack Firefox 2.0 folks.

The remaining use in Firefox 3.0 is in the profile migration code. Ancient Mozilla Suite/Netscape 6 users had their list of profiles stored in a libreg data file and this code is used to read that file. The structure of the data being looked up is baked into the code. Maybe a super-long profile name could trigger the bug, but the profile name is under the control of the victim, not the attacker.

As buffer overflows go this is as benign as it gets for Firefox 3.0.x. Might as well fix this off-by-one though, some of the linux distros support Firefox 2.0 still and will want this there just in case that InstallTrigger.GetVersion() really can trigger it as I'm guessing.

Comment 2

[Daniel Veditz [:dveditz]]

Attachment: patch

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 378339 [details] [diff] [review]
patch

test cases?

dveditz, any other similar problems in this file?  i see a few comparisons between len and bufsize (mostly inequalities).

Comment 4

[Samuel Sidler (old account; do not CC)]

Comment on attachment 378339 [details] [diff] [review]
patch

Approved for 1.9.0.12 and 1.8.1.22. a=ss for release-drivers

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: v2

> dveditz, any other similar problems in this file?  i see a few comparisons
> between len and bufsize (mostly inequalities).

A couple of places fail when the remaining buffer length is 0 -- there's code like buf[len-1] = '\0'. If len is 0 then we're trying to write that at 0xFFFFFFFF. This version of the patch fixes those.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 380580 [details] [diff] [review]
v2

everything is more restrictive but this check:

     len = XP_STRLEN(path);
-    if ( len > bufsize )
+    if ( len >= bufsize )
         return REGERR_PARAM;

Comment 7

[Daniel Veditz [:dveditz]]

fix checked into 1.8 branch and cvs trunk (1.9.0)

Checking in modules/libreg/src/reg.c;
/cvsroot/mozilla/modules/libreg/src/reg.c,v  <--  reg.c
new revision: 3.64.28.1; previous revision: 3.64
done
Checking in ../../ff3/mozilla/modules/libreg/src/reg.c;
/cvsroot/mozilla/modules/libreg/src/reg.c,v  <--  reg.c
new revision: 3.66; previous revision: 3.65
done

This has not yet been fixed on trunk or 1.9.1.x, but it's not really an accessible bug from Firefox 3 on since the only use is in profile migration for first-time users and a hypothetical attacker would have had to have written out a booby-trapped old Mozilla Suite profile registry. Not a very likely attack scenario.

We should just rip this code out of the trunk and probably 1.9.1. We could still import old profiles, but for the handful of people who care we just make them find the profiles manually.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Is there a way to actually verify this on 1.8.1? (I'm using Seamonkey 1.8 nightlies.) It doesn't look like it.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Also, shouldn't this be resolved "fixed" since it is marked as a 1.8 bug?

Comment 10

[Samuel Sidler (old account; do not CC)]

Dan: What do you want to do here with regards to landing on 1.9.1/trunk or removing the code? Should we try to get this in 1.9.1.x to be on par with 1.9.0.12?

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to comment #9)
> Also, shouldn't this be resolved "fixed" since it is marked as a 1.8 bug?

It's not fixed on trunk, and still applies there. This is why we need to fix the "branch tracking" problem one way or the other (see the "wanted" flags which attempt to capture this information). The Version field _might_ mean 1.8 branch only, or it might just mean someone first found it in 1.8, or even 1.8 is the earliest it applies (although not in this case as this is an old bug).

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #10)
> Dan: What do you want to do here with regards to landing on 1.9.1/trunk or
> removing the code? Should we try to get this in 1.9.1.x to be on par with
> 1.9.0.12?

As long as 1.9.1 contains this code we're better off fixing it. Although it's not a vulnerability there it will save time preventing future people and scanners from stumbling over the same code error.

I'll file a bug on trunk/1.9.2 to remove the code.

Comment 13

[Samuel Sidler (old account; do not CC)]

Alright, let's fix this in 1.9.1.1 and then resolve this as FIXED.

Comment 14

[Daniel Veditz [:dveditz]]

> I'll file a bug on trunk/1.9.2 to remove the code.

There already is one, bug 465662 (and a dupe bug 479105)

Comment 15

[Samuel Sidler (old account; do not CC)]

Dan: Which patch should land on 1.9.1?

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Dan: can you please get us a branch patch for 1.9.1 and land it, or mark this as unaffected for the 191 branch?

Comment 17

[Samuel Sidler (old account; do not CC)]

1.9.1 isn't unaffected, but it's okay if this waits until 1.9.1.3.

Comment 18

[Samuel Sidler (old account; do not CC)]

Mossop: Any way you can take this patch on for 1.9.1? Just needs porting from the 1.9.0 version, but I can't imagine that code has changed much. Dan's on vacation.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 380580 [details] [diff] [review]
v2

I can't see any changes in the code that affects this and the patch already applies to 1.9.1 so I think we can just take this as is.

Comment 20

[Samuel Sidler (old account; do not CC)]

Comment on attachment 380580 [details] [diff] [review]
v2

Sounds good to me.

Approved for 1.9.1.3. a=ss

Thanks again Dave!

Comment 21

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c59e7b941d7c