__gconv_translit_find: Actually append ".so" to module name [BZ #17187]

The previous code wrote the string after the terminating NUL character 
of the existing module name.  This had two effects: the ".so" extension 
was not actually visible in the module name string, and a NUL byte was 
written byond the end of the allocated buffer.

I'm not sure how to add a functionality test for this.  The test suite 
does not show any changes in behavior.

The original reporter (Tavis) considers this a security issue.  I don't see
anything in bugzilla or in your posting that indicates your assessment of
the security impact of the bug.  I can only surmise from the fact that you
made the bug and fix public rather than following CVE/embargo processes
that you don't deem it especially sensitive.  If that was a mistake and you
do consider it sensitive, then probably we should take the discussion
private immediately (though perhaps enough of the cat is already out of the
bag that it makes no difference).  If it is at all important for security,
even if not sensitive enough to be kept secret, then it would be helpful to
say something in the posting that gives the appropriate impression of urgency.

The fix itself looks fine.  It should certainly have a test first if at all
possible, though.

IIUC the bug has two effects: a one-byte buffer overrun of a malloc'd
internal buffer; and failure to open the conversion module DSO.  So you
should be able write a test that attempts to use some valid conversion
module and fails to open it.  You can also call mcheck in the beginning of
the test and mcheck_check_all later in it, so that the checking code will
reliably discover the buffer overrun.


Thanks,
Roland

On 07/29/2014 01:02 AM, Roland McGrath wrote:
> The original reporter (Tavis) considers this a security issue.  I don't see
> anything in bugzilla or in your posting that indicates your assessment of
> the security impact of the bug.  I can only surmise from the fact that you
> made the bug and fix public rather than following CVE/embargo processes
> that you don't deem it especially sensitive.

The bug was reported publicly to a security-related mailing list.  At 
this point, it's difficult to put back the toothpaste into the tube.

My assessment is "not exploitable" because it's a NUL byte written into 
malloc metadata.  But Tavis disagrees.  He is usually right.  And that's 
why I'm not really sure.

> The fix itself looks fine.  It should certainly have a test first if at all
> possible, though.
>
> IIUC the bug has two effects: a one-byte buffer overrun of a malloc'd
> internal buffer; and failure to open the conversion module DSO.

I'm a bit at a loss how to trigger the second part, but I'll give it a try.

On 07/29/2014 07:26 PM, Tavis Ormandy wrote:
>
>
>
> On Mon, Jul 28, 2014 at 10:50 PM, Florian Weimer <fweimer@redhat.com
> <mailto:fweimer@redhat.com>> wrote:
>
>     On 07/29/2014 01:02 AM, Roland McGrath wrote:
>
>         The original reporter (Tavis) considers this a security issue.
>           I don't see
>         anything in bugzilla or in your posting that indicates your
>         assessment of
>         the security impact of the bug.  I can only surmise from the
>         fact that you
>         made the bug and fix public rather than following CVE/embargo
>         processes
>         that you don't deem it especially sensitive.
>
>
>     The bug was reported publicly to a security-related mailing list.
>       At this point, it's difficult to put back the toothpaste into the
>     tube.
>
>     My assessment is "not exploitable" because it's a NUL byte written
>     into malloc metadata.  But Tavis disagrees.  He is usually right.
>       And that's why I'm not really sure.
>
>
> With some caveats, it is certainly exploitable for privilege escalation.
> It is possible to attack malloc metadata and turn that into controlled
> memory corruption, pivoting them into more general primitives (such as
> writing to an arbitrary address). In this case I believe you're
> modifying the adjacent chunk size, and compensating to keep the metadata
> consistent is possible and has been demonstrated in similar cases.

Ah, but only on 32-bit architectures, and not with 64-bit architectures 
(without writing a couple of NULs), right?

I totally I agree that this can be exploitable on 32-bit architectures. 
  I don't know why I disregarded them. *sigh*

I'll ask for a CVE assignment on the oss-security list, and we should 
treat this as a security vulnerability.  Thanks for reporting this, Tavis.

> Because this functionality has been broken for so long, it's possible
> re-enabling it might have some unintended consequences for security
> (i.e. in setuid programs). If you think this transliteration should
> certainly be allowed for setuid programs, then it makes sense to give
> the converters a quick parse for vulnerabilities (I volunteer to do so).
> However, if you're open to leaving the functionality disabled for
> AT_SECURE (as it hasn't worked for years any way) that would reduce
> attack surface for setuid programs... What do you think?

I need to figure out all this, but I think these gconv modules are 
available by other means, so fixing this shouldn't open up totally new 
exploit vectors.  And the missing file extension doesn't matter for the 
dlopen call, either—you still lose with the current code if the 
directory is attacker-controlled for some reason.

On Tue, Jul 29, 2014 at 12:05 PM, Florian Weimer <fweimer@redhat.com> wrote:
>
>
> Ah, but only on 32-bit architectures, and not with 64-bit architectures
> (without writing a couple of NULs), right?
>
> I totally I agree that this can be exploitable on 32-bit architectures.  I
> don't know why I disregarded them. *sigh*
>
> I'll ask for a CVE assignment on the oss-security list, and we should treat
> this as a security vulnerability.  Thanks for reporting this, Tavis.

No problem.

> I need to figure out all this, but I think these gconv modules are available
> by other means, so fixing this shouldn't open up totally new exploit
> vectors.

Do you have an example? I looked into it and couldn't find a way of
doing it. But I did spot a few additional bugs while I was looking.

The code doesn't work to prevent traversal if slash_count goes too
high, for example CHARSET=////../../../../../foo

However, the path is passed through upstr() in gconv_charset.h.
upstr() hardcodes the locale, so it doesn't seem possible to get
lowercase alpha characters through to dlopen().

Nonetheless, this is still a *very* serious bug, if there is a
directory that doesn't contain lowercase characters but is writable by
an attacker, then this is a trivial root shell. To verify this, follow
these steps:

1. Temporarily create a symlink to /tmp to simulate a directory
without lowercase characters (numeric, uppercase, whitespace or
special characters are fine).

# ln -s /tmp /TMP

2. As an unprivileged user create a DSO with no lowercase characters
in this directory.

$ cat > test.c
void __attribute__((constructor)) init(void)
{
  printf("hello euid=%d\n", geteuid());
}
$ gcc -w -fPIC -shared -o TEST test.c

3. Execute root as euid=0:
$ CHARSET=///../../../../../tmp/test pkexec --version
hello euid=0


Additionally, this path is parsed through expand_dst_tokens _after_
the upstr() call, so you can still insert DSTs (i.e. $PLATFORM, and so
on). This gives the attacker some additional leeway, you can try
CHARSET=////../../../${LIB}/${LIB}GL for example, which should expand
to something like "/lib/libGL". $ORIGIN behaviour varies
system-to-system. On Debian, it's just left as-is, but redhat modify
the behaviour (see
http://pkgs.fedoraproject.org/cgit/glibc.git/tree/glibc-fedora-elf-ORIGIN.patch).

This results in some oddness that may make it exploitable without any
path requirements, but I haven't thought through all the consequences
yet (personally, I would drop that patch). I suspect this patch makes
this exploitable by making it possible to open a DSO in a relative
directory, but I need to think through the consequences a bit more.

Additionally, the DST expansion looks like it's vulnerable to an
integer overflow on 32-bit, perhaps not exploitable on Fedora where
$PLATFORM and $LIB don't expand to very big strings, but on Debian
$LIB is "x86_64-linux-gnu" which is a 4x increase. Obviously that
wouldn't matter very much if you can't get a DST expanded by a setuid
boundary, but there are at least a few where you can via gconv (sudo,
pkexec, etc).

> And the missing file extension doesn't matter for the dlopen call,
> either—you still lose with the current code if the directory is
> attacker-controlled for some reason.

True, but isn't this why GCONV_PATH is in unsecvars? I.e. it's never
supposed to be attacker controlled.

Tavis.

On Thu, 31 Jul 2014, Tavis Ormandy wrote:

> Additionally, the DST expansion looks like it's vulnerable to an
> integer overflow on 32-bit, perhaps not exploitable on Fedora where
> $PLATFORM and $LIB don't expand to very big strings, but on Debian
> $LIB is "x86_64-linux-gnu" which is a 4x increase. Obviously that
> wouldn't matter very much if you can't get a DST expanded by a setuid
> boundary, but there are at least a few where you can via gconv (sudo,
> pkexec, etc).

If this is about strings from the environment, note that the Linux kernel 
limits such strings to a length of MAX_ARG_STRLEN == (PAGE_SIZE * 32).  So 
you'd also need a large page size for such an exploit on Linux (but of 
course we should fix integer overflows even if they aren't exploitable on 
some glibc platforms).