bug#24703: Store references in 8-byte chunks in compiled code

[Mark H Weaver <mhw@netris.org>]

ludo@gnu.org (Ludovic Courtès) writes:

> Mark H Weaver <mhw@netris.org> skribis:
>
>> Unfortunately, it is too widespread.  As I just pointed out in
>>
>>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24712#13
>>
>> Among the many packages that include these obfuscated store references,
>> one is 'glibc-final'.  My attempt to graft 'bash' in 'master' to fix
>> CVE-2016-0634 and CVE-2016-7543 has resulted in a system where I cannot
>> build *any* derivation, because 'guile-final' crashes during boot while
>> its 'glibc-final' tries to find its 'gconv' modules in the ungrafted
>> 'glibc-final', which is not available in the build environment.
>
> Out of curiosity, Guile crashes while loading iconv modules, right?
>
> This is surprising because the guile-for-build is always the ungrafted
> ‘guile-final’ (see ‘gnu-build’ in (guix build-system gnu)).

Indeed.  The derivations that crashed were using a grafted 'guile'.
These were not 'gnu-build' derivations, but simpler derivations such as
'module-import-compiled' derivations and some others involved with
building a 'system'.

>> So, if our approach is to use -fno-builtin-strcpy, then we will have to
>> apply it system-wide, and rebuild all of 'core-updates' from scratch.
>
> Another approach would be to patch GCC, specifically ‘expand_movstr’ in
> gcc/builtins.c, which is the part responsible for this optimization,
> along these lines (untested):
>
> --- gcc-5.3.0/gcc/builtins.c.orig	2016-10-18 10:45:35.042826368 +0200
> +++ gcc-5.3.0/gcc/builtins.c	2016-10-18 10:50:46.080616285 +0200
> @@ -3470,6 +3470,19 @@ expand_builtin_mempcpy_args (tree dest,
>  # define CODE_FOR_movstr CODE_FOR_nothing
>  #endif
>  
> +/* Return true if STR is a string denoting a "/gnu/store" file name.  */
> +
> +static bool
> +store_reference_p (tree str)
> +{
> +  const char *store;
> +
> +  store = getenv ("NIX_STORE") ?: "/gnu/store";
> +
> +  return (TREE_STRING_LENGTH (str) > strlen (store)
> +	  && strncmp (TREE_STRING_POINTER (str), store, strlen (store)));
> +}

[...]

> WDYT?

I think it's not sufficient to apply this workaround only for string
literals that _begin_ with the store directory.  In some cases, the
store name may appear only in the middle of a string.

> In the meantime, we need a workaround.  The only option I can think of
> is to retain a reference to the ungrafted item by adding a symlink to
> it, like:

I consider it a potentially serious security problem that ungrafted
outputs are being used.  Papering over the problem by preventing this
buggy software from being deleted is, in my opinion, not acceptable.

I would suggest instead that we'll need to add grafts for all packages
that include these chunked references.  However, due to bug 24832, it
may be that we'll need to rebuild all of 'core-updates' from scratch
anyway.

>> I've been investigating another approach: to enhance our scanner and
>> grafter to handle these 8-byte-chunked references.  I believe it is
>> feasible, but only if we abandon the ability to change the file names of
>> grafts outside of the hash.  The reason is that the hash portion of
>> store references are surrounded by enough other known characters on both
>> sides that the hash portion is almost always contained entirely within
>> 8-byte chunks.
>
> I think that would add complexity, would make grafting slower, and
> abandoning the ability to change file names would be a regression.
>
> So I’m more in favor of a GCC patch like above, or another compilation
> tweak.
>
> WDYT?

The GCC approach is okay with me in the short term, but I'll likely want
to revisit this issue in the future.

     Thanks,
       Mark