[v1.1] __gconv_translit_find: Actually append ".so" to module name [BZ #17187]
Commit Message
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.
Comments
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)
>
> 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
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?
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
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.
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.
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.
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.
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.
>
> 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.
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.
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)
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.
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.
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.
@@ -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)
Version 2.19
@@ -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