Re: Backporting CVE-2016-10739

[Carlos O'Donell <carlos@redhat.com>]

On 2/4/19 9:53 AM, Florian Weimer wrote:
> Here is the patch I'm testing to restore ABI after the regular
> backports.  This looks fairly reasonable to me as far as such things
> go.
> 
> I've put all patches on the fw/bug20018-backport branch on sourceware as
> well.
> 
> Thanks,
> Florian
> 
> Restore GLIBC_PRIVATE ABI after CVE-2016-10739 fix [BZ #20018]

Shouldn't this say "Remove GLIBC_PRIVATE ABI" ?

I admit this confused me for a while since the patch clearly removes
the ABI from resolv/Versions.

> This commit avoids adding the __inet_aton_exact@GLIBC_PRIVATE
> symbol.  In master, the separately-compiled getaddrinfo
> implementation in nscd needs it, however such an internal ABI change
> is not desirable on a release branch if it can be avoided easily.
> 
> 2019-02-04  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #20018]
> 	Restore GLIBC_PRIVATE ABI after CVE-2016-10739 fix.

s/Restore/Remove/g

> 	* include/arpa/inet.h (__inet_aton_exact): Declare as hidden.
> 	* resolv/inet_addr.c (__inet_aton_exact): Remove libc_hidden_def.
> 	* resolv/Versions (GLIBC_PRIVATE): Do not export
> 	__inet_aton_exact.
> 	* nscd/nscd-inet_addr.c: New file.  Build resolv/inet_addr.c for
> 	nscd, without public symbols.
> 	* nscd/Makefile (nscd-modules): Add it.
> 
> diff --git a/include/arpa/inet.h b/include/arpa/inet.h
> index 19aec74275..dce60b4909 100644
> --- a/include/arpa/inet.h
> +++ b/include/arpa/inet.h
> @@ -2,8 +2,8 @@
>  
>  #ifndef _ISOMAC
>  /* Variant of inet_aton which rejects trailing garbage.  */
> -extern int __inet_aton_exact (const char *__cp, struct in_addr *__inp);
> -libc_hidden_proto (__inet_aton_exact)
> +extern int __inet_aton_exact (const char *__cp, struct in_addr *__inp)
> +  attribute_hidden;

Why do you mark this attribute_hidden?

You should remove this from the public header, and move it to a private
header that we already have.

Again, this might be a balance question, between too many changes in the
release branch with respect to master.

Is your goal here to minimize the changes if we need future backports?

I'm OK with this, but please add a comment here saying this. We should
use comments to help reviewers and mergers undrestand why this is different.

e.g.

/* CVE-2016-10739: Hide this interface on the stable release branch.  */

>  
>  libc_hidden_proto (inet_ntop)
>  libc_hidden_proto (inet_pton)
> diff --git a/nscd/Makefile b/nscd/Makefile
> index b713a84c49..eb23c01a39 100644
> --- a/nscd/Makefile
> +++ b/nscd/Makefile
> @@ -36,7 +36,7 @@ nscd-modules := nscd connections pwdcache getpwnam_r getpwuid_r grpcache \
>  		getsrvbynm_r getsrvbypt_r servicescache \
>  		dbg_log nscd_conf nscd_stat cache mem nscd_setup_thread \
>  		xmalloc xstrdup aicache initgrcache gai res_hconf \
> -		netgroupcache
> +		netgroupcache nscd-inet_addr

OK, add a new object file to the nscd build.

>  
>  ifeq ($(build-nscd)$(have-thread-library),yesyes)
>  
> diff --git a/nscd/nscd-inet_addr.c b/nscd/nscd-inet_addr.c
> new file mode 100644
> index 0000000000..cfa4ac7462
> --- /dev/null
> +++ b/nscd/nscd-inet_addr.c
> @@ -0,0 +1,24 @@
> +/* Legacy IPv4 text-to-address functions.  Version for nscd.
> +   Copyright (C) 2019 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Do not provide definitions of the public symbols exported from
> +   libc.  */
> +#undef weak_alias
> +#define weak_alias(from, to)

Why do we have to play with the weak aliases?
Because they create visible symbols at the new name?

> +
> +#include <resolv/inet_addr.c>

OK, recompile inet_addr* as nscd-inet_addr*

> diff --git a/resolv/Versions b/resolv/Versions
> index 9a82704af7..b05778d965 100644
> --- a/resolv/Versions
> +++ b/resolv/Versions
> @@ -27,7 +27,6 @@ libc {
>      __h_errno; __resp;
>  
>      __res_iclose;
> -    __inet_aton_exact;

OK, remove from libc's GLIBC_PRIVATE ABI.

>      __inet_pton_length;
>      __resolv_context_get;
>      __resolv_context_get_preinit;
> diff --git a/resolv/inet_addr.c b/resolv/inet_addr.c
> index 41b6166a5b..1bc4a2c4d6 100644
> --- a/resolv/inet_addr.c
> +++ b/resolv/inet_addr.c
> @@ -192,7 +192,6 @@ __inet_aton_exact (const char *cp, struct in_addr *addr)
>    else
>      return 0;
>  }
> -libc_hidden_def (__inet_aton_exact)

Why remove this? Why not redefine it like you did for weak alias?

If we are going to modify inet_addr.c, we can modify it
such that we avoid hacking weak_alias *or* hack both?

Like -DNSCD_INET_ADDR=0 from the Makefile for the normal build
and then #define NSCD_INET_ADDR 1 in the nscd-inet_addr.c sources?

Something cleaner that makes it obvious there is an interface
here between nscd/libc at the source level and that we are employing
source-level reuse. I know it makes the change a bit bigger and that
we need to balance a minimal change on a release branch, but it seems
like we could be a little bit more explicit with our changes.

Likewise if we're going to remove things, I think there is value
in comments explaining why this is missing. That way if you get
a merge failure you have a pointer to the reason, rather than nothing.
It documents intent.

>  
>  /* inet_aton ignores trailing garbage.  */
>  int
> 


-- 
Cheers,
Carlos.