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

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

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

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.

-- 
Florian Weimer / Red Hat Product Security

[-- Attachment #2: 0001-__gconv_translit_find-Actually-append-.so-to-module-.patch --]
[-- Type: text/x-patch, Size: 1118 bytes --]

2014-07-21  Florian Weimer  <fweimer@redhat.com>

	[BZ #17187]
	* iconv/gconv_trans.c (__gconv_translit_find): Actually append
	".so" to form the module name.

diff --git a/NEWS b/NEWS
index 63be362..b30fd06 100644
--- a/NEWS
+++ b/NEWS
@@ -22,7 +22,7 @@ Version 2.20
   16927, 16928, 16932, 16943, 16958, 16965, 16966, 16967, 16977, 16978,
   16984, 16990, 16996, 17009, 17022, 17031, 17042, 17048, 17050, 17058,
   17061, 17062, 17069, 17075, 17078, 17079, 17084, 17086, 17088, 17092,
-  17097, 17125, 17135, 17137, 17153.
+  17097, 17125, 17135, 17137, 17153, 17187.
 
 * Optimized strchr implementation for AArch64.  Contributed by ARM Ltd.
 
diff --git a/iconv/gconv_trans.c b/iconv/gconv_trans.c
index 1e25854..921020b 100644
--- a/iconv/gconv_trans.c
+++ b/iconv/gconv_trans.c
@@ -389,7 +389,7 @@ __gconv_translit_find (struct trans_struct *trans)
 	      cp = __mempcpy (__stpcpy ((char *) newp->fname, runp->name),
 			      trans->name, name_len);
 	      if (need_so)
-		memcpy (cp, ".so", sizeof (".so"));
+		memcpy (cp - 1, ".so", sizeof (".so"));
 
 	      if (open_translit (newp) == 0)
 		{
-- 
1.9.3

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

[Roland McGrath]

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

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

[Florian Weimer]

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.

-- 
Florian Weimer / Red Hat Product Security

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

[Florian Weimer]

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.

-- 
Florian Weimer / Red Hat Product Security

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

[Tavis Ormandy]

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.

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

[Joseph S. Myers]

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

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

On 07/29/2014 01:02 AM, Roland McGrath wrote:
> The fix itself looks fine.  It should certainly have a test first if at all
> possible, though.

This entire functionality has been crippled from the beginning, it seems:

    <https://www.sourceware.org/ml/libc-alpha/2005-12/msg00091.html>

I couldn't find anything which defines a gconv_trans_context symbol, not 
within glibc, and neither as a third-party module.

Consequently, the attached patch removes gconv transliteration modules, 
changing the gconv ABI along the way.  Some gconv conversion modules now 
call __gconv_transliterate directly, and not through a function pointer 
obtained through internal gconv data structures.  There's a hidden alias 
for a PLT-less call from the internal gconv modules.

The previous patch is considerably less invasive and better for 
backporting.  You might also want to throw a wrench into open_translit 
in iconv/gconv_trans.c to prevent it from attempting to load any 
modules, just to be on the safe side.

-- 
Florian Weimer / Red Hat Product Security


[-- Attachment #2: gconv.patch --]
[-- Type: text/x-patch, Size: 27700 bytes --]

2014-08-08  Florian Weimer  <fweimer@redhat.com>

	[BZ #17187]
	* iconv/gconv_open.c (__gconv_open): Remove transliteration module
	loading.
	* iconv/Versions (__gconv_transliterate): Export for use from
	gconv modules.
	* iconv/gconv.h (__GCONV_TRANSLIT): New flag.
	(struct __gconv_trans_data, __gconv_trans_fct,
	__gconv_trans_context_fct, __gconv_trans_query_fct,
	__gconv_trans_init_fct, __gconv_trans_end_fct): Remove type
	definitions.
	(struct __gconv_step_data): Remove __trans member.
	(__gconv_transliterate): Declaration moved from gconv_int.h.  No
	longer hidden.  Remove unused trans_data argument.
	* iconv/gconv_int.h (struct trans_struct): Remove definition.
	(__gconv_translit_find): Remove declaration.
	(__gconv_transliterate): Declaration moved to gconv.h.  Add hidden
	prototype.
	* iconv/gconv_close.c (__gconv_close): Remove __trans cleanup.
	* iconv/gconv_trans.c (__gconv_transliterate): Remove unused
	trans_data argument.  Add hidden definition.
	(struct known_trans, search_tree, lock, trans_compare, open_translit,
	__gconv_translit_find): Remove.
	* iconv/loop.c (STANDARD_TO_LOOP_ERR_HANDLER): Call
	__gconv_transliterate directly if __GCONV_TRANSLIT is set.
	* iconv/skeleton.c: Remove transliteration initialization.
	* libio/fileops.c (_IO_new_file_fopen): Adjust struct
	__gconv_step_data initialization.
	* libio/iofwide.c (__libio_translit_): Remove.
	(_IO_fwide): Adjust struct __gconv_step_data initialization.
	* wcsmbs/btowc.c (__btowc): Likewise.
	* wcsmbs/mbrtoc16.c (mbrtoc16): Likewise.
	* wcsmbs/mbrtowc.c (__mbrtowc): Likewise.
	* wcsmbs/mbsnrtowcs.c (__mbsnrtowcs): Likewise.
	* wcsmbs/mbsrtowcs_l.c (__mbsrtowcs_l): Likewise.
	* wcsmbs/wcrtomb.c (__wcrtomb): Likewise.
	* wcsmbs/wcsnrtombs.c (__wcsnrtombs): Likewise.
	* wcsmbs/wcsrtombs.c (__wcsrtombs): Likewise.
	* wcsmbs/wctob.c (wctob): Likewise.

diff --git a/NEWS b/NEWS
index 5a240de..d41c63b 100644
--- a/NEWS
+++ b/NEWS
@@ -22,7 +22,7 @@ Version 2.20
   16918, 16922, 16927, 16928, 16932, 16943, 16958, 16965, 16966, 16967,
   16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031, 17042, 17048,
   17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079, 17084, 17086,
-  17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153, 17213.
+  17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153, 17187, 17213.
 
 * Reverted change of ABI data structures for s390 and s390x:
   On s390 and s390x the size of struct ucontext and jmp_buf was increased in
@@ -97,6 +97,10 @@ Version 2.20
   silently replaced with the "C" locale when running in AT_SECURE mode
   (e.g., in a SUID program).  This is no longer necessary because of the
   additional checks.
+
+* Support for loadable gconv transliteration modules has been removed
+  because it did not work at all.  Regular gconv conversion modules are
+  still supported.
 \f
 Version 2.19
 
diff --git a/iconv/Versions b/iconv/Versions
index 5d50cf1..60ab10a 100644
--- a/iconv/Versions
+++ b/iconv/Versions
@@ -6,5 +6,8 @@ libc {
   GLIBC_PRIVATE {
     # functions shared with iconv program
     __gconv_get_alias_db; __gconv_get_cache; __gconv_get_modules_db;
+
+    # function used by the gconv modules
+    __gconv_transliterate;
   }
 }
diff --git a/iconv/gconv.h b/iconv/gconv.h
index 108dccb..7d59bb0 100644
--- a/iconv/gconv.h
+++ b/iconv/gconv.h
@@ -56,7 +56,8 @@ enum
 {
   __GCONV_IS_LAST = 0x0001,
   __GCONV_IGNORE_ERRORS = 0x0002,
-  __GCONV_SWAP = 0x0004
+  __GCONV_SWAP = 0x0004,
+  __GCONV_TRANSLIT = 0x0008
 };
 
 
@@ -64,7 +65,6 @@ enum
 struct __gconv_step;
 struct __gconv_step_data;
 struct __gconv_loaded_object;
-struct __gconv_trans_data;
 
 
 /* Type of a conversion function.  */
@@ -80,38 +80,6 @@ typedef int (*__gconv_init_fct) (struct __gconv_step *);
 typedef void (*__gconv_end_fct) (struct __gconv_step *);
 
 
-/* Type of a transliteration/transscription function.  */
-typedef int (*__gconv_trans_fct) (struct __gconv_step *,
-				  struct __gconv_step_data *, void *,
-				  const unsigned char *,
-				  const unsigned char **,
-				  const unsigned char *, unsigned char **,
-				  size_t *);
-
-/* Function to call to provide transliteration module with context.  */
-typedef int (*__gconv_trans_context_fct) (void *, const unsigned char *,
-					  const unsigned char *,
-					  unsigned char *, unsigned char *);
-
-/* Function to query module about supported encoded character sets.  */
-typedef int (*__gconv_trans_query_fct) (const char *, const char ***,
-					size_t *);
-
-/* Constructor and destructor for local data for transliteration.  */
-typedef int (*__gconv_trans_init_fct) (void **, const char *);
-typedef void (*__gconv_trans_end_fct) (void *);
-
-struct __gconv_trans_data
-{
-  /* Transliteration/Transscription function.  */
-  __gconv_trans_fct __trans_fct;
-  __gconv_trans_context_fct __trans_context_fct;
-  __gconv_trans_end_fct __trans_end_fct;
-  void *__data;
-  struct __gconv_trans_data *__next;
-};
-
-
 /* Description of a conversion step.  */
 struct __gconv_step
 {
@@ -163,9 +131,6 @@ struct __gconv_step_data
   __mbstate_t *__statep;
   __mbstate_t __state;	/* This element must not be used directly by
 			   any module; always use STATEP!  */
-
-  /* Transliteration information.  */
-  struct __gconv_trans_data *__trans;
 };
 
 
@@ -177,4 +142,13 @@ typedef struct __gconv_info
   __extension__ struct __gconv_step_data __data __flexarr;
 } *__gconv_t;
 
+/* Transliteration using the locale's data.  */
+extern int __gconv_transliterate (struct __gconv_step *step,
+				  struct __gconv_step_data *step_data,
+				  const unsigned char *inbufstart,
+				  const unsigned char **inbufp,
+				  const unsigned char *inbufend,
+				  unsigned char **outbufstart,
+				  size_t *irreversible);
+
 #endif /* gconv.h */
diff --git a/iconv/gconv_close.c b/iconv/gconv_close.c
index 81f0e0b..f6394af 100644
--- a/iconv/gconv_close.c
+++ b/iconv/gconv_close.c
@@ -37,20 +37,6 @@ __gconv_close (__gconv_t cd)
   drunp = cd->__data;
   do
     {
-      struct __gconv_trans_data *transp;
-
-      transp = drunp->__trans;
-      while (transp != NULL)
-	{
-	  struct __gconv_trans_data *curp = transp;
-	  transp = transp->__next;
-
-	  if (__glibc_unlikely (curp->__trans_end_fct != NULL))
-	    curp->__trans_end_fct (curp->__data);
-
-	  free (curp);
-	}
-
       if (!(drunp->__flags & __GCONV_IS_LAST) && drunp->__outbuf != NULL)
 	free (drunp->__outbuf);
     }
diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h
index ace076b..13b0e99 100644
--- a/iconv/gconv_int.h
+++ b/iconv/gconv_int.h
@@ -92,21 +92,6 @@ struct gconv_module
 };
 
 
-/* Internal data structure to represent transliteration module.  */
-struct trans_struct
-{
-  const char *name;
-  struct trans_struct *next;
-
-  const char **csnames;
-  size_t ncsnames;
-  __gconv_trans_fct trans_fct;
-  __gconv_trans_context_fct trans_context_fct;
-  __gconv_trans_init_fct trans_init_fct;
-  __gconv_trans_end_fct trans_end_fct;
-};
-
-
 /* Flags for `gconv_open'.  */
 enum
 {
@@ -258,20 +243,7 @@ extern void __gconv_get_builtin_trans (const char *name,
 				       struct __gconv_step *step)
      internal_function;
 
-/* Try to load transliteration step module.  */
-extern int __gconv_translit_find (struct trans_struct *trans)
-     internal_function;
-
-/* Transliteration using the locale's data.  */
-extern int __gconv_transliterate (struct __gconv_step *step,
-				  struct __gconv_step_data *step_data,
-				  void *trans_data,
-				  const unsigned char *inbufstart,
-				  const unsigned char **inbufp,
-				  const unsigned char *inbufend,
-				  unsigned char **outbufstart,
-				  size_t *irreversible) attribute_hidden;
-
+libc_hidden_proto (__gconv_transliterate)
 
 /* If NAME is an codeset alias expand it.  */
 extern int __gconv_compare_alias (const char *name1, const char *name2)
diff --git a/iconv/gconv_open.c b/iconv/gconv_open.c
index bfbe22b..615f33d 100644
--- a/iconv/gconv_open.c
+++ b/iconv/gconv_open.c
@@ -39,7 +39,7 @@ __gconv_open (const char *toset, const char *fromset, __gconv_t *handle,
   int conv_flags = 0;
   const char *errhand;
   const char *ignore;
-  struct trans_struct *trans = NULL;
+  bool translit = false;
 
   /* Find out whether any error handling method is specified.  */
   errhand = strchr (toset, '/');
@@ -66,72 +66,10 @@ __gconv_open (const char *toset, const char *fromset, __gconv_t *handle,
 	  while (tok != NULL)
 	    {
 	      if (__strcasecmp_l (tok, "TRANSLIT", _nl_C_locobj_ptr) == 0)
-		{
-		  /* It's the builtin transliteration handling.  We only
-		     support it for working on the internal encoding.  */
-		  static const char *const internal_trans_names[1]
-		    = { "INTERNAL" };
-		  struct trans_struct *lastp = NULL;
-		  struct trans_struct *runp;
-
-		  for (runp = trans; runp != NULL; runp = runp->next)
-		    if (runp->trans_fct == __gconv_transliterate)
-		      break;
-		    else
-		      lastp = runp;
-
-		  if (runp == NULL)
-		    {
-		      struct trans_struct *newp;
-
-		      newp = (struct trans_struct *) alloca (sizeof (*newp));
-		      memset (newp, '\0', sizeof (*newp));
-
-		      /* We leave the `name' field zero to signal that
-			 this is an internal transliteration step.  */
-		      newp->csnames = (const char **) internal_trans_names;
-		      newp->ncsnames = 1;
-		      newp->trans_fct = __gconv_transliterate;
-
-		      if (lastp == NULL)
-			trans = newp;
-		      else
-			lastp->next = newp;
-		    }
-		}
+		translit = true;
 	      else if (__strcasecmp_l (tok, "IGNORE", _nl_C_locobj_ptr) == 0)
 		/* Set the flag to ignore all errors.  */
 		conv_flags |= __GCONV_IGNORE_ERRORS;
-	      else
-		{
-		  /* `tok' is possibly a module name.  We'll see later
-		     whether we can find it.  But first see that we do
-		     not already a module of this name.  */
-		  struct trans_struct *lastp = NULL;
-		  struct trans_struct *runp;
-
-		  for (runp = trans; runp != NULL; runp = runp->next)
-		    if (runp->name != NULL
-			&& __strcasecmp_l (tok, runp->name,
-					   _nl_C_locobj_ptr) == 0)
-		      break;
-		    else
-		      lastp = runp;
-
-		  if (runp == NULL)
-		    {
-		      struct trans_struct *newp;
-
-		      newp = (struct trans_struct *) alloca (sizeof (*newp));
-		      memset (newp, '\0', sizeof (*newp));
-		      newp->name = tok;
-
-		      if (lastp == NULL)
-			trans = newp;
-		      else
-			lastp->next = newp;
-		    }
-		}
 
 	      tok = __strtok_r (NULL, ",", &ptr);
 	    }
@@ -172,25 +110,6 @@ __gconv_open (const char *toset, const char *fromset, __gconv_t *handle,
   res = __gconv_find_transform (toset, fromset, &steps, &nsteps, flags);
   if (res == __GCONV_OK)
     {
-      /* Find the modules.  */
-      struct trans_struct *lastp = NULL;
-      struct trans_struct *runp;
-
-      for (runp = trans; runp != NULL; runp = runp->next)
-	{
-	  if (runp->name == NULL
-	      || __builtin_expect (__gconv_translit_find (runp), 0) == 0)
-	    lastp = runp;
-	  else
-	    {
-	      /* This means we haven't found the module.  Remove it.  */
-	      if (lastp == NULL)
-		trans  = runp->next;
-	      else
-		lastp->next  = runp->next;
-	    }
-	}
-
       /* Allocate room for handle.  */
       result = (__gconv_t) malloc (sizeof (struct __gconv_info)
 				   + (nsteps
@@ -199,8 +118,6 @@ __gconv_open (const char *toset, const char *fromset, __gconv_t *handle,
 	res = __GCONV_NOMEM;
       else
 	{
-	  size_t n;
-
 	  /* Remember the list of steps.  */
 	  result->__steps = steps;
 	  result->__nsteps = nsteps;
@@ -228,47 +145,12 @@ __gconv_open (const char *toset, const char *fromset, __gconv_t *handle,
 	      /* We use the `mbstate_t' member in DATA.  */
 	      result->__data[cnt].__statep = &result->__data[cnt].__state;
 
-	      /* Now see whether we can use any of the transliteration
-		 modules for this step.  */
-	      for (runp = trans; runp != NULL; runp = runp->next)
-		for (n = 0; n < runp->ncsnames; ++n)
-		  if (__strcasecmp_l (steps[cnt].__from_name,
-				      runp->csnames[n], _nl_C_locobj_ptr) == 0)
-		    {
-		      void *data = NULL;
-
-		      /* Match!  Now try the initializer.  */
-		      if (runp->trans_init_fct == NULL
-			  || (runp->trans_init_fct (&data,
-						    steps[cnt].__to_name)
-			      == __GCONV_OK))
-			{
-			  /* Append at the end of the list.  */
-			  struct __gconv_trans_data *newp;
-			  struct __gconv_trans_data **lastp;
-
-			  newp = (struct __gconv_trans_data *)
-			    malloc (sizeof (struct __gconv_trans_data));
-			  if (newp == NULL)
-			    {
-			      res = __GCONV_NOMEM;
-			      goto bail;
-			    }
-
-			  newp->__trans_fct = runp->trans_fct;
-			  newp->__trans_context_fct = runp->trans_context_fct;
-			  newp->__trans_end_fct = runp->trans_end_fct;
-			  newp->__data = data;
-			  newp->__next = NULL;
-
-			  lastp = &result->__data[cnt].__trans;
-			  while (*lastp != NULL)
-			    lastp = &(*lastp)->__next;
-
-			  *lastp = newp;
-			}
-		      break;
-		    }
+	      /* The builtin transliteration handling only
+		 supports the internal encoding.  */
+	      if (translit
+		  && __strcasecmp_l (steps[cnt].__from_name,
+				     "INTERNAL", _nl_C_locobj_ptr) == 0)
+		conv_flags |= __GCONV_TRANSLIT;
 
 	      /* If this is the last step we must not allocate an
 		 output buffer.  */
@@ -309,23 +191,7 @@ __gconv_open (const char *toset, const char *fromset, __gconv_t *handle,
 	  if (result != NULL)
 	    {
 	      while (cnt-- > 0)
-		{
-		  struct __gconv_trans_data *transp;
-
-		  transp = result->__data[cnt].__trans;
-		  while (transp != NULL)
-		    {
-		      struct __gconv_trans_data *curp = transp;
-		      transp = transp->__next;
-
-		      if (__glibc_unlikely (curp->__trans_end_fct != NULL))
-			curp->__trans_end_fct (curp->__data);
-
-		      free (curp);
-		    }
-
-		  free (result->__data[cnt].__outbuf);
-		}
+		free (result->__data[cnt].__outbuf);
 
 	      free (result);
 	      result = NULL;
diff --git a/iconv/gconv_trans.c b/iconv/gconv_trans.c
index 1e25854..65b5539 100644
--- a/iconv/gconv_trans.c
+++ b/iconv/gconv_trans.c
@@ -32,7 +32,6 @@
 int
 __gconv_transliterate (struct __gconv_step *step,
 		       struct __gconv_step_data *step_data,
-		       void *trans_data __attribute__ ((unused)),
 		       const unsigned char *inbufstart,
 		       const unsigned char **inbufp,
 		       const unsigned char *inbufend,
@@ -237,182 +236,4 @@ __gconv_transliterate (struct __gconv_step *step,
   /* Haven't found a match.  */
   return __GCONV_ILLEGAL_INPUT;
 }
-
-
-/* Structure to represent results of found (or not) transliteration
-   modules.  */
-struct known_trans
-{
-  /* This structure must remain the first member.  */
-  struct trans_struct info;
-
-  char *fname;
-  void *handle;
-  int open_count;
-};
-
-
-/* Tree with results of previous calls to __gconv_translit_find.  */
-static void *search_tree;
-
-/* We modify global data.   */
-__libc_lock_define_initialized (static, lock);
-
-
-/* Compare two transliteration entries.  */
-static int
-trans_compare (const void *p1, const void *p2)
-{
-  const struct known_trans *s1 = (const struct known_trans *) p1;
-  const struct known_trans *s2 = (const struct known_trans *) p2;
-
-  return strcmp (s1->info.name, s2->info.name);
-}
-
-
-/* Open (maybe reopen) the module named in the struct.  Get the function
-   and data structure pointers we need.  */
-static int
-open_translit (struct known_trans *trans)
-{
-  __gconv_trans_query_fct queryfct;
-
-  trans->handle = __libc_dlopen (trans->fname);
-  if (trans->handle == NULL)
-    /* Not available.  */
-    return 1;
-
-  /* Find the required symbol.  */
-  queryfct = __libc_dlsym (trans->handle, "gconv_trans_context");
-  if (queryfct == NULL)
-    {
-      /* We cannot live with that.  */
-    close_and_out:
-      __libc_dlclose (trans->handle);
-      trans->handle = NULL;
-      return 1;
-    }
-
-  /* Get the context.  */
-  if (queryfct (trans->info.name, &trans->info.csnames, &trans->info.ncsnames)
-      != 0)
-    goto close_and_out;
-
-  /* Of course we also have to have the actual function.  */
-  trans->info.trans_fct = __libc_dlsym (trans->handle, "gconv_trans");
-  if (trans->info.trans_fct == NULL)
-    goto close_and_out;
-
-  /* Now the optional functions.  */
-  trans->info.trans_init_fct =
-    __libc_dlsym (trans->handle, "gconv_trans_init");
-  trans->info.trans_context_fct =
-    __libc_dlsym (trans->handle, "gconv_trans_context");
-  trans->info.trans_end_fct =
-    __libc_dlsym (trans->handle, "gconv_trans_end");
-
-  trans->open_count = 1;
-
-  return 0;
-}
-
-
-int
-internal_function
-__gconv_translit_find (struct trans_struct *trans)
-{
-  struct known_trans **found;
-  const struct path_elem *runp;
-  int res = 1;
-
-  /* We have to have a name.  */
-  assert (trans->name != NULL);
-
-  /* Acquire the lock.  */
-  __libc_lock_lock (lock);
-
-  /* See whether we know this module already.  */
-  found = __tfind (trans, &search_tree, trans_compare);
-  if (found != NULL)
-    {
-      /* Is this module available?  */
-      if ((*found)->handle != NULL)
-	{
-	  /* Maybe we have to reopen the file.  */
-	  if ((*found)->handle != (void *) -1)
-	    /* The object is not unloaded.  */
-	    res = 0;
-	  else if (open_translit (*found) == 0)
-	    {
-	      /* Copy the data.  */
-	      *trans = (*found)->info;
-	      (*found)->open_count++;
-	      res = 0;
-	    }
-	}
-    }
-  else
-    {
-      size_t name_len = strlen (trans->name) + 1;
-      int need_so = 0;
-      struct known_trans *newp;
-
-      /* We have to continue looking for the module.  */
-      if (__gconv_path_elem == NULL)
-	__gconv_get_path ();
-
-      /* See whether we have to append .so.  */
-      if (name_len <= 4 || memcmp (&trans->name[name_len - 4], ".so", 3) != 0)
-	need_so = 1;
-
-      /* Create a new entry.  */
-      newp = (struct known_trans *) malloc (sizeof (struct known_trans)
-					    + (__gconv_max_path_elem_len
-					       + name_len + 3)
-					    + name_len);
-      if (newp != NULL)
-	{
-	  char *cp;
-
-	  /* Clear the struct.  */
-	  memset (newp, '\0', sizeof (struct known_trans));
-
-	  /* Store a copy of the module name.  */
-	  newp->info.name = cp = (char *) (newp + 1);
-	  cp = __mempcpy (cp, trans->name, name_len);
-
-	  newp->fname = cp;
-
-	  /* Search in all the directories.  */
-	  for (runp = __gconv_path_elem; runp->name != NULL; ++runp)
-	    {
-	      cp = __mempcpy (__stpcpy ((char *) newp->fname, runp->name),
-			      trans->name, name_len);
-	      if (need_so)
-		memcpy (cp, ".so", sizeof (".so"));
-
-	      if (open_translit (newp) == 0)
-		{
-		  /* We found a module.  */
-		  res = 0;
-		  break;
-		}
-	    }
-
-	  if (res)
-	    newp->fname = NULL;
-
-	  /* In any case we'll add the entry to our search tree.  */
-	  if (__tsearch (newp, &search_tree, trans_compare) == NULL)
-	    {
-	      /* Yickes, this should not happen.  Unload the object.  */
-	      res = 1;
-	      /* XXX unload here.  */
-	    }
-	}
-    }
-
-  __libc_lock_unlock (lock);
-
-  return res;
-}
+libc_hidden_def (__gconv_transliterate)
diff --git a/iconv/loop.c b/iconv/loop.c
index a480c0c..f4430ed 100644
--- a/iconv/loop.c
+++ b/iconv/loop.c
@@ -213,8 +213,6 @@
    points.  */
 #define STANDARD_TO_LOOP_ERR_HANDLER(Incr) \
   {									      \
-    struct __gconv_trans_data *trans;					      \
-									      \
     result = __GCONV_ILLEGAL_INPUT;					      \
 									      \
     if (irreversible == NULL)						      \
@@ -227,14 +225,10 @@
     UPDATE_PARAMS;							      \
 									      \
     /* First try the transliteration methods.  */			      \
-    for (trans = step_data->__trans; trans != NULL; trans = trans->__next)    \
-      {									      \
-	result = DL_CALL_FCT (trans->__trans_fct,			      \
-			      (step, step_data, trans->__data, *inptrp,	      \
-			       &inptr, inend, &outptr, irreversible));	      \
-	if (result != __GCONV_ILLEGAL_INPUT)				      \
-	  break;							      \
-      }									      \
+    if ((step_data->__flags & __GCONV_TRANSLIT) != 0)			      \
+      result = __gconv_transliterate					      \
+	(step, step_data, *inptrp,					      \
+	 &inptr, inend, &outptr, irreversible);			      \
 									      \
     REINIT_PARAMS;							      \
 									      \
diff --git a/iconv/skeleton.c b/iconv/skeleton.c
index 73dc186..acd60e2 100644
--- a/iconv/skeleton.c
+++ b/iconv/skeleton.c
@@ -501,8 +501,9 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data,
     }
   else
     {
-      /* We preserve the initial values of the pointer variables.  */
-      const unsigned char *inptr = *inptrp;
+      /* We preserve the initial values of the pointer variables,
+	 but only some conversion modules need it.  */
+      const unsigned char *inptr __attribute__ ((__unused__)) = *inptrp;
       unsigned char *outbuf = (__builtin_expect (outbufstart == NULL, 1)
 			       ? data->__outbuf : *outbufstart);
       unsigned char *outend = data->__outbufend;
@@ -592,8 +593,6 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data,
 
       while (1)
 	{
-	  struct __gconv_trans_data *trans;
-
 	  /* Remember the start value for this round.  */
 	  inptr = *inptrp;
 	  /* The outbuf buffer is empty.  */
@@ -640,13 +639,6 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data,
 	      return status;
 	    }
 
-	  /* Give the transliteration module the chance to store the
-	     original text and the result in case it needs a context.  */
-	  for (trans = data->__trans; trans != NULL; trans = trans->__next)
-	    if (trans->__trans_context_fct != NULL)
-	      DL_CALL_FCT (trans->__trans_context_fct,
-			   (trans->__data, inptr, *inptrp, outstart, outbuf));
-
 	  /* We finished one use of the loops.  */
 	  ++data->__invocation_counter;
 
diff --git a/libio/fileops.c b/libio/fileops.c
index 204cfea..e0d7b76 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -403,21 +403,16 @@ _IO_new_file_fopen (fp, filename, mode, is32not64)
 	  cc->__cd_in.__cd.__data[0].__flags = __GCONV_IS_LAST;
 	  cc->__cd_in.__cd.__data[0].__statep = &result->_wide_data->_IO_state;
 
-	  /* XXX For now no transliteration.  */
-	  cc->__cd_in.__cd.__data[0].__trans = NULL;
-
 	  cc->__cd_out.__cd.__nsteps = fcts.tomb_nsteps;
 	  cc->__cd_out.__cd.__steps = fcts.tomb;
 
 	  cc->__cd_out.__cd.__data[0].__invocation_counter = 0;
 	  cc->__cd_out.__cd.__data[0].__internal_use = 1;
-	  cc->__cd_out.__cd.__data[0].__flags = __GCONV_IS_LAST;
+	  cc->__cd_out.__cd.__data[0].__flags
+	    = __GCONV_IS_LAST | __GCONV_TRANSLIT;
 	  cc->__cd_out.__cd.__data[0].__statep =
 	    &result->_wide_data->_IO_state;
 
-	  /* And now the transliteration.  */
-	  cc->__cd_out.__cd.__data[0].__trans = &__libio_translit;
-
 	  /* From now on use the wide character callback functions.  */
 	  ((struct _IO_FILE_plus *) fp)->vtable = fp->_wide_data->_wide_vtable;
 
diff --git a/libio/iofwide.c b/libio/iofwide.c
index 64187e4..ecf5819 100644
--- a/libio/iofwide.c
+++ b/libio/iofwide.c
@@ -81,14 +81,6 @@ const struct _IO_codecvt __libio_codecvt =
 };
 
 
-#ifdef _LIBC
-const struct __gconv_trans_data __libio_translit attribute_hidden =
-{
-  .__trans_fct = __gconv_transliterate
-};
-#endif
-
-
 /* Return orientation of stream.  If mode is nonzero try to change
    the orientation first.  */
 #undef _IO_fwide
@@ -146,20 +138,14 @@ _IO_fwide (fp, mode)
 	cc->__cd_in.__cd.__data[0].__flags = __GCONV_IS_LAST;
 	cc->__cd_in.__cd.__data[0].__statep = &fp->_wide_data->_IO_state;
 
-	/* XXX For now no transliteration.  */
-	cc->__cd_in.__cd.__data[0].__trans = NULL;
-
 	cc->__cd_out.__cd.__nsteps = fcts.tomb_nsteps;
 	cc->__cd_out.__cd.__steps = fcts.tomb;
 
 	cc->__cd_out.__cd.__data[0].__invocation_counter = 0;
 	cc->__cd_out.__cd.__data[0].__internal_use = 1;
-	cc->__cd_out.__cd.__data[0].__flags = __GCONV_IS_LAST;
+	cc->__cd_out.__cd.__data[0].__flags
+	  = __GCONV_IS_LAST | __GCONV_TRANSLIT;
 	cc->__cd_out.__cd.__data[0].__statep = &fp->_wide_data->_IO_state;
-
-	/* And now the transliteration.  */
-	cc->__cd_out.__cd.__data[0].__trans
-	  = (struct __gconv_trans_data  *) &__libio_translit;
       }
 #else
 # ifdef _GLIBCPP_USE_WCHAR_T
diff --git a/wcsmbs/btowc.c b/wcsmbs/btowc.c
index 289736f..aafb392 100644
--- a/wcsmbs/btowc.c
+++ b/wcsmbs/btowc.c
@@ -75,7 +75,6 @@ __btowc (c)
       data.__internal_use = 1;
       data.__flags = __GCONV_IS_LAST;
       data.__statep = &data.__state;
-      data.__trans = NULL;
 
       /* Make sure we start in the initial state.  */
       memset (&data.__state, '\0', sizeof (mbstate_t));
diff --git a/wcsmbs/mbrtoc16.c b/wcsmbs/mbrtoc16.c
index 643aaf5..69105ba 100644
--- a/wcsmbs/mbrtoc16.c
+++ b/wcsmbs/mbrtoc16.c
@@ -67,7 +67,6 @@ mbrtoc16 (char16_t *pc16, const char *s, size_t n, mbstate_t *ps)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = ps;
-  data.__trans = NULL;
 
   /* A first special case is if S is NULL.  This means put PS in the
      initial state.  */
diff --git a/wcsmbs/mbrtowc.c b/wcsmbs/mbrtowc.c
index c57217a..8070bd8 100644
--- a/wcsmbs/mbrtowc.c
+++ b/wcsmbs/mbrtowc.c
@@ -49,7 +49,6 @@ __mbrtowc (wchar_t *pwc, const char *s, size_t n, mbstate_t *ps)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = ps ?: &state;
-  data.__trans = NULL;
 
   /* A first special case is if S is NULL.  This means put PS in the
      initial state.  */
diff --git a/wcsmbs/mbsnrtowcs.c b/wcsmbs/mbsnrtowcs.c
index f91e580..e611dde 100644
--- a/wcsmbs/mbsnrtowcs.c
+++ b/wcsmbs/mbsnrtowcs.c
@@ -58,7 +58,6 @@ __mbsnrtowcs (dst, src, nmc, len, ps)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = ps ?: &state;
-  data.__trans = NULL;
 
   if (nmc == 0)
     return 0;
diff --git a/wcsmbs/mbsrtowcs_l.c b/wcsmbs/mbsrtowcs_l.c
index 08ff3c9..5e10a7e 100644
--- a/wcsmbs/mbsrtowcs_l.c
+++ b/wcsmbs/mbsrtowcs_l.c
@@ -56,7 +56,6 @@ __mbsrtowcs_l (dst, src, len, ps, l)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = ps;
-  data.__trans = NULL;
 
   /* Get the conversion functions.  */
   fcts = get_gconv_fcts (l->__locales[LC_CTYPE]);
diff --git a/wcsmbs/wcrtomb.c b/wcsmbs/wcrtomb.c
index be93877..67c68d3 100644
--- a/wcsmbs/wcrtomb.c
+++ b/wcsmbs/wcrtomb.c
@@ -49,7 +49,6 @@ __wcrtomb (char *s, wchar_t wc, mbstate_t *ps)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = ps ?: &state;
-  data.__trans = NULL;
 
   /* A first special case is if S is NULL.  This means put PS in the
      initial state.  */
diff --git a/wcsmbs/wcsnrtombs.c b/wcsmbs/wcsnrtombs.c
index 6fe718d..015d08c 100644
--- a/wcsmbs/wcsnrtombs.c
+++ b/wcsmbs/wcsnrtombs.c
@@ -56,7 +56,6 @@ __wcsnrtombs (dst, src, nwc, len, ps)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = ps ?: &state;
-  data.__trans = NULL;
 
   if (nwc == 0)
     return 0;
diff --git a/wcsmbs/wcsrtombs.c b/wcsmbs/wcsrtombs.c
index 24e249c..988b468 100644
--- a/wcsmbs/wcsrtombs.c
+++ b/wcsmbs/wcsrtombs.c
@@ -52,7 +52,6 @@ __wcsrtombs (dst, src, len, ps)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = ps ?: &state;
-  data.__trans = NULL;
 
   /* Get the conversion functions.  */
   fcts = get_gconv_fcts (_NL_CURRENT_DATA (LC_CTYPE));
diff --git a/wcsmbs/wctob.c b/wcsmbs/wctob.c
index 24c33ce..8e65738 100644
--- a/wcsmbs/wctob.c
+++ b/wcsmbs/wctob.c
@@ -53,7 +53,6 @@ wctob (c)
   data.__internal_use = 1;
   data.__flags = __GCONV_IS_LAST;
   data.__statep = &data.__state;
-  data.__trans = NULL;
 
   /* Make sure we start in the initial state.  */
   memset (&data.__state, '\0', sizeof (mbstate_t));

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

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On 07/21/2014 03:01 PM, Florian Weimer wrote:
> 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 attached version of this patch is suitable for backporting and 
disables external module loading, rather than attempting to fix it.  The 
tests for the internal transliteration functionality still pass.

I would like to suggest to put this in master, and include the second, 
larger patch as a clean-up after the 2.20 release.

-- 
Florian Weimer / Red Hat Product Security

[-- Attachment #2: 0001-__gconv_translit_find-Disable-function-BZ-17187.patch --]
[-- Type: text/x-patch, Size: 6123 bytes --]

2014-08-21  Florian Weimer  <fweimer@redhat.com>

	[BZ #17187]
	* iconv/gconv_trans.c (struct known_trans, search_tree, lock,
	trans_compare, open_translit, __gconv_translit_find):
	Remove module loading code.

diff --git a/NEWS b/NEWS
index 28da6e5..6d8529c 100644
--- a/NEWS
+++ b/NEWS
@@ -23,7 +23,7 @@ Version 2.20
   16966, 16967, 16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031,
   17042, 17048, 17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079,
   17084, 17086, 17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153,
-  17213, 17259, 17261, 17262, 17263.
+  17187, 17213, 17259, 17261, 17262, 17263.
 
 * Reverted change of ABI data structures for s390 and s390x:
   On s390 and s390x the size of struct ucontext and jmp_buf was increased in
@@ -108,6 +108,10 @@ Version 2.20
   handle the new instruction encodings.  This is known to affect Valgrind
   versions up through 3.9 (but will be fixed in the forthcoming 3.10
   release), and might affect other tools that do instruction emulation.
+
+* Support for loadable gconv transliteration modules has been removed
+  because it did not work at all.  Regular gconv conversion modules are
+  still supported.  (CVE-2014-5119)
 \f
 Version 2.19
 
diff --git a/iconv/gconv_trans.c b/iconv/gconv_trans.c
index 1e25854..d71c029 100644
--- a/iconv/gconv_trans.c
+++ b/iconv/gconv_trans.c
@@ -238,181 +238,11 @@ __gconv_transliterate (struct __gconv_step *step,
   return __GCONV_ILLEGAL_INPUT;
 }
 
-
-/* Structure to represent results of found (or not) transliteration
-   modules.  */
-struct known_trans
-{
-  /* This structure must remain the first member.  */
-  struct trans_struct info;
-
-  char *fname;
-  void *handle;
-  int open_count;
-};
-
-
-/* Tree with results of previous calls to __gconv_translit_find.  */
-static void *search_tree;
-
-/* We modify global data.   */
-__libc_lock_define_initialized (static, lock);
-
-
-/* Compare two transliteration entries.  */
-static int
-trans_compare (const void *p1, const void *p2)
-{
-  const struct known_trans *s1 = (const struct known_trans *) p1;
-  const struct known_trans *s2 = (const struct known_trans *) p2;
-
-  return strcmp (s1->info.name, s2->info.name);
-}
-
-
-/* Open (maybe reopen) the module named in the struct.  Get the function
-   and data structure pointers we need.  */
-static int
-open_translit (struct known_trans *trans)
-{
-  __gconv_trans_query_fct queryfct;
-
-  trans->handle = __libc_dlopen (trans->fname);
-  if (trans->handle == NULL)
-    /* Not available.  */
-    return 1;
-
-  /* Find the required symbol.  */
-  queryfct = __libc_dlsym (trans->handle, "gconv_trans_context");
-  if (queryfct == NULL)
-    {
-      /* We cannot live with that.  */
-    close_and_out:
-      __libc_dlclose (trans->handle);
-      trans->handle = NULL;
-      return 1;
-    }
-
-  /* Get the context.  */
-  if (queryfct (trans->info.name, &trans->info.csnames, &trans->info.ncsnames)
-      != 0)
-    goto close_and_out;
-
-  /* Of course we also have to have the actual function.  */
-  trans->info.trans_fct = __libc_dlsym (trans->handle, "gconv_trans");
-  if (trans->info.trans_fct == NULL)
-    goto close_and_out;
-
-  /* Now the optional functions.  */
-  trans->info.trans_init_fct =
-    __libc_dlsym (trans->handle, "gconv_trans_init");
-  trans->info.trans_context_fct =
-    __libc_dlsym (trans->handle, "gconv_trans_context");
-  trans->info.trans_end_fct =
-    __libc_dlsym (trans->handle, "gconv_trans_end");
-
-  trans->open_count = 1;
-
-  return 0;
-}
-
-
 int
 internal_function
 __gconv_translit_find (struct trans_struct *trans)
 {
-  struct known_trans **found;
-  const struct path_elem *runp;
-  int res = 1;
-
-  /* We have to have a name.  */
-  assert (trans->name != NULL);
-
-  /* Acquire the lock.  */
-  __libc_lock_lock (lock);
-
-  /* See whether we know this module already.  */
-  found = __tfind (trans, &search_tree, trans_compare);
-  if (found != NULL)
-    {
-      /* Is this module available?  */
-      if ((*found)->handle != NULL)
-	{
-	  /* Maybe we have to reopen the file.  */
-	  if ((*found)->handle != (void *) -1)
-	    /* The object is not unloaded.  */
-	    res = 0;
-	  else if (open_translit (*found) == 0)
-	    {
-	      /* Copy the data.  */
-	      *trans = (*found)->info;
-	      (*found)->open_count++;
-	      res = 0;
-	    }
-	}
-    }
-  else
-    {
-      size_t name_len = strlen (trans->name) + 1;
-      int need_so = 0;
-      struct known_trans *newp;
-
-      /* We have to continue looking for the module.  */
-      if (__gconv_path_elem == NULL)
-	__gconv_get_path ();
-
-      /* See whether we have to append .so.  */
-      if (name_len <= 4 || memcmp (&trans->name[name_len - 4], ".so", 3) != 0)
-	need_so = 1;
-
-      /* Create a new entry.  */
-      newp = (struct known_trans *) malloc (sizeof (struct known_trans)
-					    + (__gconv_max_path_elem_len
-					       + name_len + 3)
-					    + name_len);
-      if (newp != NULL)
-	{
-	  char *cp;
-
-	  /* Clear the struct.  */
-	  memset (newp, '\0', sizeof (struct known_trans));
-
-	  /* Store a copy of the module name.  */
-	  newp->info.name = cp = (char *) (newp + 1);
-	  cp = __mempcpy (cp, trans->name, name_len);
-
-	  newp->fname = cp;
-
-	  /* Search in all the directories.  */
-	  for (runp = __gconv_path_elem; runp->name != NULL; ++runp)
-	    {
-	      cp = __mempcpy (__stpcpy ((char *) newp->fname, runp->name),
-			      trans->name, name_len);
-	      if (need_so)
-		memcpy (cp, ".so", sizeof (".so"));
-
-	      if (open_translit (newp) == 0)
-		{
-		  /* We found a module.  */
-		  res = 0;
-		  break;
-		}
-	    }
-
-	  if (res)
-	    newp->fname = NULL;
-
-	  /* In any case we'll add the entry to our search tree.  */
-	  if (__tsearch (newp, &search_tree, trans_compare) == NULL)
-	    {
-	      /* Yickes, this should not happen.  Unload the object.  */
-	      res = 1;
-	      /* XXX unload here.  */
-	    }
-	}
-    }
-
-  __libc_lock_unlock (lock);
-
-  return res;
+  /* This function always fails.  Transliteration module loading is
+     not implemented.  */
+  return 1;
 }
-- 
1.9.3

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 8723 bytes --]

On Thu, Aug 21, 2014 at 07:33:03PM +0200, Florian Weimer wrote:
> On 07/21/2014 03:01 PM, Florian Weimer wrote:
> >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 attached version of this patch is suitable for backporting and disables
> external module loading, rather than attempting to fix it.  The tests for
> the internal transliteration functionality still pass.
> 
> I would like to suggest to put this in master, and include the second,
> larger patch as a clean-up after the 2.20 release.

The update here is that there is now a live privilege escalation
exploit[1] for 32-bit Fedora 20 and it may not be too hard to port the
exploit to more platforms.

I think this fix is fine (except a minor nit below), but it would be
good if another maintainer also verifies that it won't break anything.
Also, Allan needs to ack it for 2.20.  I am going to put the patch in
rawhide today anyway, so I'll report back if there are any issues; I
don't expect any though.

> 
> -- 
> Florian Weimer / Red Hat Product Security

> 2014-08-21  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #17187]
> 	* iconv/gconv_trans.c (struct known_trans, search_tree, lock,
> 	trans_compare, open_translit, __gconv_translit_find):
> 	Remove module loading code.
> 
> diff --git a/NEWS b/NEWS
> index 28da6e5..6d8529c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,7 +23,7 @@ Version 2.20
>    16966, 16967, 16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031,
>    17042, 17048, 17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079,
>    17084, 17086, 17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153,
> -  17213, 17259, 17261, 17262, 17263.
> +  17187, 17213, 17259, 17261, 17262, 17263.
>  
>  * Reverted change of ABI data structures for s390 and s390x:
>    On s390 and s390x the size of struct ucontext and jmp_buf was increased in
> @@ -108,6 +108,10 @@ Version 2.20
>    handle the new instruction encodings.  This is known to affect Valgrind
>    versions up through 3.9 (but will be fixed in the forthcoming 3.10
>    release), and might affect other tools that do instruction emulation.
> +
> +* Support for loadable gconv transliteration modules has been removed
> +  because it did not work at all.  Regular gconv conversion modules are
> +  still supported.  (CVE-2014-5119)
>  \f
>  Version 2.19
>  
> diff --git a/iconv/gconv_trans.c b/iconv/gconv_trans.c
> index 1e25854..d71c029 100644
> --- a/iconv/gconv_trans.c
> +++ b/iconv/gconv_trans.c
> @@ -238,181 +238,11 @@ __gconv_transliterate (struct __gconv_step *step,
>    return __GCONV_ILLEGAL_INPUT;
>  }
>  
> -
> -/* Structure to represent results of found (or not) transliteration
> -   modules.  */
> -struct known_trans
> -{
> -  /* This structure must remain the first member.  */
> -  struct trans_struct info;
> -
> -  char *fname;
> -  void *handle;
> -  int open_count;
> -};
> -
> -
> -/* Tree with results of previous calls to __gconv_translit_find.  */
> -static void *search_tree;
> -
> -/* We modify global data.   */
> -__libc_lock_define_initialized (static, lock);
> -
> -
> -/* Compare two transliteration entries.  */
> -static int
> -trans_compare (const void *p1, const void *p2)
> -{
> -  const struct known_trans *s1 = (const struct known_trans *) p1;
> -  const struct known_trans *s2 = (const struct known_trans *) p2;
> -
> -  return strcmp (s1->info.name, s2->info.name);
> -}
> -
> -
> -/* Open (maybe reopen) the module named in the struct.  Get the function
> -   and data structure pointers we need.  */
> -static int
> -open_translit (struct known_trans *trans)
> -{
> -  __gconv_trans_query_fct queryfct;
> -
> -  trans->handle = __libc_dlopen (trans->fname);
> -  if (trans->handle == NULL)
> -    /* Not available.  */
> -    return 1;
> -
> -  /* Find the required symbol.  */
> -  queryfct = __libc_dlsym (trans->handle, "gconv_trans_context");
> -  if (queryfct == NULL)
> -    {
> -      /* We cannot live with that.  */
> -    close_and_out:
> -      __libc_dlclose (trans->handle);
> -      trans->handle = NULL;
> -      return 1;
> -    }
> -
> -  /* Get the context.  */
> -  if (queryfct (trans->info.name, &trans->info.csnames, &trans->info.ncsnames)
> -      != 0)
> -    goto close_and_out;
> -
> -  /* Of course we also have to have the actual function.  */
> -  trans->info.trans_fct = __libc_dlsym (trans->handle, "gconv_trans");
> -  if (trans->info.trans_fct == NULL)
> -    goto close_and_out;
> -
> -  /* Now the optional functions.  */
> -  trans->info.trans_init_fct =
> -    __libc_dlsym (trans->handle, "gconv_trans_init");
> -  trans->info.trans_context_fct =
> -    __libc_dlsym (trans->handle, "gconv_trans_context");
> -  trans->info.trans_end_fct =
> -    __libc_dlsym (trans->handle, "gconv_trans_end");
> -
> -  trans->open_count = 1;
> -
> -  return 0;
> -}
> -
> -
>  int
>  internal_function
>  __gconv_translit_find (struct trans_struct *trans)
>  {
> -  struct known_trans **found;
> -  const struct path_elem *runp;
> -  int res = 1;
> -
> -  /* We have to have a name.  */
> -  assert (trans->name != NULL);
> -
> -  /* Acquire the lock.  */
> -  __libc_lock_lock (lock);
> -
> -  /* See whether we know this module already.  */
> -  found = __tfind (trans, &search_tree, trans_compare);
> -  if (found != NULL)
> -    {
> -      /* Is this module available?  */
> -      if ((*found)->handle != NULL)
> -	{
> -	  /* Maybe we have to reopen the file.  */
> -	  if ((*found)->handle != (void *) -1)
> -	    /* The object is not unloaded.  */
> -	    res = 0;
> -	  else if (open_translit (*found) == 0)
> -	    {
> -	      /* Copy the data.  */
> -	      *trans = (*found)->info;
> -	      (*found)->open_count++;
> -	      res = 0;
> -	    }
> -	}
> -    }
> -  else
> -    {
> -      size_t name_len = strlen (trans->name) + 1;
> -      int need_so = 0;
> -      struct known_trans *newp;
> -
> -      /* We have to continue looking for the module.  */
> -      if (__gconv_path_elem == NULL)
> -	__gconv_get_path ();
> -
> -      /* See whether we have to append .so.  */
> -      if (name_len <= 4 || memcmp (&trans->name[name_len - 4], ".so", 3) != 0)
> -	need_so = 1;
> -
> -      /* Create a new entry.  */
> -      newp = (struct known_trans *) malloc (sizeof (struct known_trans)
> -					    + (__gconv_max_path_elem_len
> -					       + name_len + 3)
> -					    + name_len);
> -      if (newp != NULL)
> -	{
> -	  char *cp;
> -
> -	  /* Clear the struct.  */
> -	  memset (newp, '\0', sizeof (struct known_trans));
> -
> -	  /* Store a copy of the module name.  */
> -	  newp->info.name = cp = (char *) (newp + 1);
> -	  cp = __mempcpy (cp, trans->name, name_len);
> -
> -	  newp->fname = cp;
> -
> -	  /* Search in all the directories.  */
> -	  for (runp = __gconv_path_elem; runp->name != NULL; ++runp)
> -	    {
> -	      cp = __mempcpy (__stpcpy ((char *) newp->fname, runp->name),
> -			      trans->name, name_len);
> -	      if (need_so)
> -		memcpy (cp, ".so", sizeof (".so"));
> -
> -	      if (open_translit (newp) == 0)
> -		{
> -		  /* We found a module.  */
> -		  res = 0;
> -		  break;
> -		}
> -	    }
> -
> -	  if (res)
> -	    newp->fname = NULL;
> -
> -	  /* In any case we'll add the entry to our search tree.  */
> -	  if (__tsearch (newp, &search_tree, trans_compare) == NULL)
> -	    {
> -	      /* Yickes, this should not happen.  Unload the object.  */
> -	      res = 1;
> -	      /* XXX unload here.  */
> -	    }
> -	}
> -    }
> -
> -  __libc_lock_unlock (lock);
> -
> -  return res;
> +  /* This function always fails.  Transliteration module loading is
> +     not implemented.  */

The comment should reflect the fact that there was an implementation
that we have removed.  It will be clear from the logs, but not
everybody close the code.  So maybe something like this:

/* Transliteration module loading implementation has been removed
   because it never worked and had a serious security flaw.  As a
   result, this function always fails.  */

> +  return 1;
>  }
> -- 
> 1.9.3
> 

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1119128#c9
[2] http://googleprojectzero.blogspot.in/2014/08/the-poisoned-nul-byte-2014-edition.html

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Florian Weimer]

On 08/26/2014 08:35 AM, Siddhesh Poyarekar wrote:

> I think this fix is fine (except a minor nit below), but it would be

Thanks.

> good if another maintainer also verifies that it won't break anything.
> Also, Allan needs to ack it for 2.20.  I am going to put the patch in
> rawhide today anyway, so I'll report back if there are any issues; I
> don't expect any though.

Okay, I will reword the comment.

Allan, is this okay for 2.20/current master?

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Allan McRae]

On 26/08/14 19:52, Florian Weimer wrote:
> On 08/26/2014 08:35 AM, Siddhesh Poyarekar wrote:
> 
>> I think this fix is fine (except a minor nit below), but it would be
> 
> Thanks.
> 
>> good if another maintainer also verifies that it won't break anything.
>> Also, Allan needs to ack it for 2.20.  I am going to put the patch in
>> rawhide today anyway, so I'll report back if there are any issues; I
>> don't expect any though.
> 
> Okay, I will reword the comment.
> 
> Allan, is this okay for 2.20/current master?
> 

I'd like the "if another maintainer also verifies that it won't break
anything" to be enacted.  My best guess is that it is fine and given
there is an exploit it should go in, but I am not confident enough about
lack of side effects.

Can someone else give this an ack?  Roland, Carlos, Joseph, etc?

Once that is done, it is fine to commit to master.

Allan

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> +  /* This function always fails.  Transliteration module loading is
> +     not implemented.  */
> +  return 1;

Since it always fails you can just remove the function completely.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Florian Weimer]

On 08/26/2014 02:09 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> +  /* This function always fails.  Transliteration module loading is
>> +     not implemented.  */
>> +  return 1;
>
> Since it always fails you can just remove the function completely.

If taken to the logical conclusion, this has a ripple effect and is not 
suitable for backporting:

   <https://sourceware.org/ml/libc-alpha/2014-08/msg00119.html>

So I had to stop somewhere, and I think the patch from last week is a 
reasonable compromise for backports and the 2.20 release.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Carlos O'Donell]

On 08/26/2014 07:23 AM, Allan McRae wrote:
> On 26/08/14 19:52, Florian Weimer wrote:
>> On 08/26/2014 08:35 AM, Siddhesh Poyarekar wrote:
>>
>>> I think this fix is fine (except a minor nit below), but it would be
>>
>> Thanks.
>>
>>> good if another maintainer also verifies that it won't break anything.
>>> Also, Allan needs to ack it for 2.20.  I am going to put the patch in
>>> rawhide today anyway, so I'll report back if there are any issues; I
>>> don't expect any though.
>>
>> Okay, I will reword the comment.
>>
>> Allan, is this okay for 2.20/current master?
>>
> 
> I'd like the "if another maintainer also verifies that it won't break
> anything" to be enacted.  My best guess is that it is fine and given
> there is an exploit it should go in, but I am not confident enough about
> lack of side effects.
> 
> Can someone else give this an ack?  Roland, Carlos, Joseph, etc?
> 
> Once that is done, it is fine to commit to master.

I'm reviewing.

c.

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Carlos O'Donell]

On 08/26/2014 08:25 AM, Florian Weimer wrote:
> On 08/26/2014 02:09 PM, Andreas Schwab wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>>
>>> +  /* This function always fails.  Transliteration module loading is
>>> +     not implemented.  */
>>> +  return 1;
>>
>> Since it always fails you can just remove the function completely.
> 
> If taken to the logical conclusion, this has a ripple effect and is not suitable for backporting:
> 
>   <https://sourceware.org/ml/libc-alpha/2014-08/msg00119.html>
> 
> So I had to stop somewhere, and I think the patch from last week is a reasonable compromise for backports and the 2.20 release.

Yes, and no. The ripple effect is still present in that all callers
must actually handle the function returning one instead of zero, thus
we still have to audit the callers. I tend to agree with Andreas
here, it's basically the same amount of audit work, *but* your smaller
patch will apply more easily to older branches, and reduces the
chance we have compiler/assembler/linker problems due to refactored
code.

Cheers,
Carlos.

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Carlos O'Donell]

On 08/21/2014 01:33 PM, Florian Weimer wrote:
> On 07/21/2014 03:01 PM, Florian Weimer wrote:
>> 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 attached version of this patch is suitable for backporting and
> disables external module loading, rather than attempting to fix it.
> The tests for the internal transliteration functionality still pass.
> 
> I would like to suggest to put this in master, and include the
> second, larger patch as a clean-up after the 2.20 release.

My assumption here is that we are 100% certain nobody could work around
the interface bugs (name bug, and failure to copy trans bug) and make
use of this functionality.

OK to commit from my perspective with one nit i.e. NEWS needs expanding.

> -- 
> Florian Weimer / Red Hat Product Security
> 
> 0001-__gconv_translit_find-Disable-function-BZ-17187.patch
> 
> 
> 2014-08-21  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #17187]
> 	* iconv/gconv_trans.c (struct known_trans, search_tree, lock,
> 	trans_compare, open_translit, __gconv_translit_find):
> 	Remove module loading code.
> 
> diff --git a/NEWS b/NEWS
> index 28da6e5..6d8529c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,7 +23,7 @@ Version 2.20
>    16966, 16967, 16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031,
>    17042, 17048, 17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079,
>    17084, 17086, 17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153,
> -  17213, 17259, 17261, 17262, 17263.
> +  17187, 17213, 17259, 17261, 17262, 17263.
>  
>  * Reverted change of ABI data structures for s390 and s390x:
>    On s390 and s390x the size of struct ucontext and jmp_buf was increased in
> @@ -108,6 +108,10 @@ Version 2.20
>    handle the new instruction encodings.  This is known to affect Valgrind
>    versions up through 3.9 (but will be fixed in the forthcoming 3.10
>    release), and might affect other tools that do instruction emulation.
> +
> +* Support for loadable gconv transliteration modules has been removed
> +  because it did not work at all.  Regular gconv conversion modules are
> +  still supported.  (CVE-2014-5119)

Suggest:

* Support for loadable gconv transliteration modules has been removed.
  The support for transliteration modules has been non-functional for
  over a decade, and the removal is prompted by security defects.  The
  normal gconv conversion modules are still supported.  Transliteration
  is still possible, but only to and from installed languages.
  (CVE-2014-5519)

I want to be clear that (a) for security and functionality reasons we
removed the code (b) normal gconv modules work and (c) transliteration
still works.

>  \f
>  Version 2.19
>  
> diff --git a/iconv/gconv_trans.c b/iconv/gconv_trans.c
> index 1e25854..d71c029 100644
> --- a/iconv/gconv_trans.c
> +++ b/iconv/gconv_trans.c
> @@ -238,181 +238,11 @@ __gconv_transliterate (struct __gconv_step *step,
>    return __GCONV_ILLEGAL_INPUT;
>  }
>  
> -
> -/* Structure to represent results of found (or not) transliteration
> -   modules.  */
> -struct known_trans
> -{
> -  /* This structure must remain the first member.  */
> -  struct trans_struct info;
> -
> -  char *fname;
> -  void *handle;
> -  int open_count;
> -};

OK. All uses are in this file.

> -
> -
> -/* Tree with results of previous calls to __gconv_translit_find.  */
> -static void *search_tree;

OK.

> -
> -/* We modify global data.   */
> -__libc_lock_define_initialized (static, lock);

OK. Used only by __gconv_translit_find.

> -
> -
> -/* Compare two transliteration entries.  */
> -static int
> -trans_compare (const void *p1, const void *p2)
> -{
> -  const struct known_trans *s1 = (const struct known_trans *) p1;
> -  const struct known_trans *s2 = (const struct known_trans *) p2;
> -
> -  return strcmp (s1->info.name, s2->info.name);
> -}

OK. Used only by __gconv_translit_find.

> -
> -
> -/* Open (maybe reopen) the module named in the struct.  Get the function
> -   and data structure pointers we need.  */
> -static int
> -open_translit (struct known_trans *trans)
> -{
> -  __gconv_trans_query_fct queryfct;
> -
> -  trans->handle = __libc_dlopen (trans->fname);
> -  if (trans->handle == NULL)
> -    /* Not available.  */
> -    return 1;
> -
> -  /* Find the required symbol.  */
> -  queryfct = __libc_dlsym (trans->handle, "gconv_trans_context");
> -  if (queryfct == NULL)
> -    {
> -      /* We cannot live with that.  */
> -    close_and_out:
> -      __libc_dlclose (trans->handle);
> -      trans->handle = NULL;
> -      return 1;
> -    }
> -
> -  /* Get the context.  */
> -  if (queryfct (trans->info.name, &trans->info.csnames, &trans->info.ncsnames)
> -      != 0)
> -    goto close_and_out;
> -
> -  /* Of course we also have to have the actual function.  */
> -  trans->info.trans_fct = __libc_dlsym (trans->handle, "gconv_trans");
> -  if (trans->info.trans_fct == NULL)
> -    goto close_and_out;
> -
> -  /* Now the optional functions.  */
> -  trans->info.trans_init_fct =
> -    __libc_dlsym (trans->handle, "gconv_trans_init");
> -  trans->info.trans_context_fct =
> -    __libc_dlsym (trans->handle, "gconv_trans_context");
> -  trans->info.trans_end_fct =
> -    __libc_dlsym (trans->handle, "gconv_trans_end");
> -
> -  trans->open_count = 1;
> -
> -  return 0;
> -}

OK, used only by __gconv_translit_find.

> -
> -
>  int
>  internal_function
>  __gconv_translit_find (struct trans_struct *trans)
>  {
> -  struct known_trans **found;
> -  const struct path_elem *runp;
> -  int res = 1;
> -
> -  /* We have to have a name.  */
> -  assert (trans->name != NULL);
> -
> -  /* Acquire the lock.  */
> -  __libc_lock_lock (lock);
> -
> -  /* See whether we know this module already.  */
> -  found = __tfind (trans, &search_tree, trans_compare);
> -  if (found != NULL)
> -    {
> -      /* Is this module available?  */
> -      if ((*found)->handle != NULL)
> -	{
> -	  /* Maybe we have to reopen the file.  */
> -	  if ((*found)->handle != (void *) -1)
> -	    /* The object is not unloaded.  */
> -	    res = 0;
> -	  else if (open_translit (*found) == 0)
> -	    {
> -	      /* Copy the data.  */
> -	      *trans = (*found)->info;
> -	      (*found)->open_count++;
> -	      res = 0;
> -	    }
> -	}
> -    }
> -  else
> -    {
> -      size_t name_len = strlen (trans->name) + 1;
> -      int need_so = 0;
> -      struct known_trans *newp;
> -
> -      /* We have to continue looking for the module.  */
> -      if (__gconv_path_elem == NULL)
> -	__gconv_get_path ();
> -
> -      /* See whether we have to append .so.  */
> -      if (name_len <= 4 || memcmp (&trans->name[name_len - 4], ".so", 3) != 0)
> -	need_so = 1;
> -
> -      /* Create a new entry.  */
> -      newp = (struct known_trans *) malloc (sizeof (struct known_trans)
> -					    + (__gconv_max_path_elem_len
> -					       + name_len + 3)
> -					    + name_len);
> -      if (newp != NULL)
> -	{
> -	  char *cp;
> -
> -	  /* Clear the struct.  */
> -	  memset (newp, '\0', sizeof (struct known_trans));
> -
> -	  /* Store a copy of the module name.  */
> -	  newp->info.name = cp = (char *) (newp + 1);
> -	  cp = __mempcpy (cp, trans->name, name_len);
> -
> -	  newp->fname = cp;
> -
> -	  /* Search in all the directories.  */
> -	  for (runp = __gconv_path_elem; runp->name != NULL; ++runp)
> -	    {
> -	      cp = __mempcpy (__stpcpy ((char *) newp->fname, runp->name),
> -			      trans->name, name_len);
> -	      if (need_so)
> -		memcpy (cp, ".so", sizeof (".so"));
> -
> -	      if (open_translit (newp) == 0)
> -		{
> -		  /* We found a module.  */
> -		  res = 0;
> -		  break;
> -		}
> -	    }
> -
> -	  if (res)
> -	    newp->fname = NULL;
> -
> -	  /* In any case we'll add the entry to our search tree.  */
> -	  if (__tsearch (newp, &search_tree, trans_compare) == NULL)
> -	    {
> -	      /* Yickes, this should not happen.  Unload the object.  */
> -	      res = 1;
> -	      /* XXX unload here.  */
> -	    }
> -	}
> -    }
> -
> -  __libc_lock_unlock (lock);
> -
> -  return res;
> +  /* This function always fails.  Transliteration module loading is
> +     not implemented.  */
> +  return 1;

OK. 

>  }
> -- 1.9.3

Auditing:

iconv/gconv_open.c (__gconv_open) calls __gconv_translit_find.
- Can handle return of 1 correctly? Yes.

iconv/gconv_int.h (__gconv_translit_find)
- No change to prototype? Yes.

manual/*
- I don't see any specific parts of the manual that even talk
  about transliteration modules so I do not believe there is
  anything to be changed.

Cheers,
Carlos.

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Carlos O'Donell]

On 08/26/2014 10:33 AM, Carlos O'Donell wrote:
> On 08/26/2014 07:23 AM, Allan McRae wrote:
>> On 26/08/14 19:52, Florian Weimer wrote:
>>> On 08/26/2014 08:35 AM, Siddhesh Poyarekar wrote:
>>>
>>>> I think this fix is fine (except a minor nit below), but it would be
>>>
>>> Thanks.
>>>
>>>> good if another maintainer also verifies that it won't break anything.
>>>> Also, Allan needs to ack it for 2.20.  I am going to put the patch in
>>>> rawhide today anyway, so I'll report back if there are any issues; I
>>>> don't expect any though.
>>>
>>> Okay, I will reword the comment.
>>>
>>> Allan, is this okay for 2.20/current master?
>>>
>>
>> I'd like the "if another maintainer also verifies that it won't break
>> anything" to be enacted.  My best guess is that it is fine and given
>> there is an exploit it should go in, but I am not confident enough about
>> lack of side effects.
>>
>> Can someone else give this an ack?  Roland, Carlos, Joseph, etc?
>>
>> Once that is done, it is fine to commit to master.
> 
> I'm reviewing.

Review done. Looks good to me, one nit.

c.

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Florian Weimer]

On 08/26/2014 06:45 PM, Carlos O'Donell wrote:
> On 08/21/2014 01:33 PM, Florian Weimer wrote:
>> On 07/21/2014 03:01 PM, Florian Weimer wrote:
>>> 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 attached version of this patch is suitable for backporting and
>> disables external module loading, rather than attempting to fix it.
>> The tests for the internal transliteration functionality still pass.
>>
>> I would like to suggest to put this in master, and include the
>> second, larger patch as a clean-up after the 2.20 release.
>
> My assumption here is that we are 100% certain nobody could work around
> the interface bugs (name bug, and failure to copy trans bug) and make
> use of this functionality.

I think it is impossible to work around these bugs using public 
interfaces only.

In addition, I searched for the names of the ELF symbols which must be 
provided in by any transliteration module, and neither on the public 
web, nor in ELF symbols from shipped RPM files, I could find even 
remotely relevant hits.

> OK to commit from my perspective with one nit i.e. NEWS needs expanding.

Thanks.

> Suggest:
>
> * Support for loadable gconv transliteration modules has been removed.
>    The support for transliteration modules has been non-functional for
>    over a decade, and the removal is prompted by security defects.  The
>    normal gconv conversion modules are still supported.  Transliteration
>    is still possible, but only to and from installed languages.
>    (CVE-2014-5519)
>
> I want to be clear that (a) for security and functionality reasons we
> removed the code (b) normal gconv modules work and (c) transliteration
> still works.

We do not support transliteration for the source character set, per this 
code in iconv/gconv_open.c:

   /* For the source character set we ignore the error handler 
specification.
      XXX Is this really always the best?  */
   ignore = strchr (fromset, '/');
   if (ignore != NULL && (ignore = strchr (ignore + 1, '/')) != NULL
       && *++ignore != '\0')
     {
       char *newfromset = (char *) alloca (ignore - fromset + 1);

       newfromset[ignore - fromset] = '\0';
       fromset = memcpy (newfromset, fromset, ignore - fromset);
     }

The internal transliteration code only supports TRANSLIT and IGNORE, so 
I suggest this NEWS entry:

* Support for loadable gconv transliteration modules has been removed.
   The support for transliteration modules has been non-functional for
   over a decade, and the removal is prompted by security defects.  The
   normal gconv conversion modules are still supported.  Transliteration
   with //TRANSLIT is still possible, and the //IGNORE specifier
   continues to be  supported. (CVE-2014-5519)

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Carlos O'Donell]

On 08/26/2014 12:55 PM, Florian Weimer wrote:
> On 08/26/2014 06:45 PM, Carlos O'Donell wrote:
>> On 08/21/2014 01:33 PM, Florian Weimer wrote:
>>> On 07/21/2014 03:01 PM, Florian Weimer wrote:
>>>> 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 attached version of this patch is suitable for backporting and
>>> disables external module loading, rather than attempting to fix it.
>>> The tests for the internal transliteration functionality still pass.
>>>
>>> I would like to suggest to put this in master, and include the
>>> second, larger patch as a clean-up after the 2.20 release.
>>
>> My assumption here is that we are 100% certain nobody could work around
>> the interface bugs (name bug, and failure to copy trans bug) and make
>> use of this functionality.
> 
> I think it is impossible to work around these bugs using public
> interfaces only.
> 
> In addition, I searched for the names of the ELF symbols which must
> be provided in by any transliteration module, and neither on the
> public web, nor in ELF symbols from shipped RPM files, I could find
> even remotely relevant hits.

Right, so as we suspected this is an unused feature. Which is good.
 
>> OK to commit from my perspective with one nit i.e. NEWS needs expanding.
> 
> Thanks.
> 
>> Suggest:
>>
>> * Support for loadable gconv transliteration modules has been removed.
>>    The support for transliteration modules has been non-functional for
>>    over a decade, and the removal is prompted by security defects.  The
>>    normal gconv conversion modules are still supported.  Transliteration
>>    is still possible, but only to and from installed languages.
>>    (CVE-2014-5519)
>>
>> I want to be clear that (a) for security and functionality reasons we
>> removed the code (b) normal gconv modules work and (c) transliteration
>> still works.
> 
> We do not support transliteration for the source character set, per this code in iconv/gconv_open.c:
> 
>   /* For the source character set we ignore the error handler specification.
>      XXX Is this really always the best?  */
>   ignore = strchr (fromset, '/');
>   if (ignore != NULL && (ignore = strchr (ignore + 1, '/')) != NULL
>       && *++ignore != '\0')
>     {
>       char *newfromset = (char *) alloca (ignore - fromset + 1);
> 
>       newfromset[ignore - fromset] = '\0';
>       fromset = memcpy (newfromset, fromset, ignore - fromset);
>     }
> 
> The internal transliteration code only supports TRANSLIT and IGNORE, so I suggest this NEWS entry:
> 
> * Support for loadable gconv transliteration modules has been removed.
>   The support for transliteration modules has been non-functional for
>   over a decade, and the removal is prompted by security defects.  The
>   normal gconv conversion modules are still supported.  Transliteration
>   with //TRANSLIT is still possible, and the //IGNORE specifier
>   continues to be  supported. (CVE-2014-5519)

OK, that's a more accurate description.

OK with that.

Cheers,
Carlos.

Re: [PATCH v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]

[Florian Weimer]

On 08/26/2014 06:55 PM, Florian Weimer wrote:

> The internal transliteration code only supports TRANSLIT and IGNORE, so
> I suggest this NEWS entry:
>
> * Support for loadable gconv transliteration modules has been removed.
>    The support for transliteration modules has been non-functional for
>    over a decade, and the removal is prompted by security defects.  The
>    normal gconv conversion modules are still supported.  Transliteration
>    with //TRANSLIT is still possible, and the //IGNORE specifier
>    continues to be  supported. (CVE-2014-5119)

I'm afraid I had to fix a typo in the CVE ID in a separate commit.  The 
above NEWS entry contains the corrected ID.

-- 
Florian Weimer / Red Hat Product Security