[PATCH] debug: Reject %n format strings in RELRO of static main program (bug 33183)

[Adhemerval Zanella Netto]

On 23/07/25 18:36, Florian Weimer wrote:
> Fortification should trap on %n strings in the RELRO area of a
> statically linked main program because RELRO may not be in effect
> with static linking (whether or not the program header exists).
> 
> While at itm fix the return type of check_relro in
> elf/dl-readonly-area.c.  It happened to work correctly because
> only two enum constants with index 0 and 1 were used before.
> 
> ---
>  debug/Makefile                     |  2 ++
>  debug/tst-sprintf-fortify-rdonly.c | 27 +++++++++++++++++++++++----
>  elf/dl-readonly-area.c             |  9 ++++++++-
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/debug/Makefile b/debug/Makefile
> index 919bf91c42..412917d6ca 100644
> --- a/debug/Makefile
> +++ b/debug/Makefile
> @@ -343,6 +343,8 @@ tests-static = \
>    tst-sprintf-fortify-rdonly-static \
>    # tests-static
>  
> +CFLAGS-tst-sprintf-fortify-rdonly-static.c += -DSTATIC_TEST
> +
>  tests-time64 += \
>    $(tests-all-time64-chk) \
>    # tests-time64
> diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c
> index fafc8340ea..969af820ed 100644
> --- a/debug/tst-sprintf-fortify-rdonly.c
> +++ b/debug/tst-sprintf-fortify-rdonly.c
> @@ -31,7 +31,8 @@
>  
>  static sigjmp_buf chk_fail_buf;
>  static volatile int ret;
> -static bool chk_fail_ok;
> +static volatile int chk_fail_line;
> +static volatile bool chk_fail_ok;
>  
>  static void
>  handler (int sig)
> @@ -42,12 +43,17 @@ handler (int sig)
>        longjmp (chk_fail_buf, 1);
>      }
>    else
> -    _exit (127);
> +    {
> +      printf ("error: fortify failure in block starting on line %d\n",
> +	      chk_fail_line);
> +      _exit (127);
> +    }
>  }
>  
>  #define FORTIFY_FAIL \
>    do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0)
>  #define CHK_FAIL_START \
> +  chk_fail_line = __LINE__;			\
>    chk_fail_ok = true;				\
>    if (! sigsetjmp (chk_fail_buf, 1))		\
>      {
> @@ -143,23 +149,36 @@ do_test (void)
>      CHK_FAIL_END
>    }
>  
> -  /* relro_format is at a readonly part of .bss segment, so '%n' in format input
> -     should not trigger a fortify failure.  */
> +  /* relro_format is at a readonly part of .bss segment, so '%n' in
> +     format input should not trigger a fortify failure.  However, for
> +     static builds, RELRO may not be in effect.  */
>    {
>      char buf[128];
>      int n1;
>      int n2;
> +#ifdef STATIC_TEST
> +    CHK_FAIL_START
> +#endif
>      if (sprintf (buf, relro_format, str2, &n1, str2, &n2) != 2
>  	|| n1 != 1 || n2 != 2)
>        FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2);
> +#ifdef STATIC_TEST
> +    CHK_FAIL_END
> +#endif
>    }
>  
>    /* Same as before, but from an library.  */
>    {
>      int n1;
>      int n2;
> +#ifdef STATIC_TEST
> +    CHK_FAIL_START
> +#endif
>      if (sprintf_relro (&n1, &n2) != 2 || n1 != 1 || n2 != 2)
>        FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2);
> +#ifdef STATIC_TEST
> +    CHK_FAIL_END
> +#endif
>    }
>  
>    {
> diff --git a/elf/dl-readonly-area.c b/elf/dl-readonly-area.c
> index 22769ec9d9..471f1cd4bc 100644
> --- a/elf/dl-readonly-area.c
> +++ b/elf/dl-readonly-area.c
> @@ -18,9 +18,16 @@
>  
>  #include <ldsodefs.h>
>  
> -static bool
> +static enum dl_readonly_area_error_type
>  check_relro (const struct link_map *l, uintptr_t start, uintptr_t end)
>  {
> +#ifndef SHARED
> +  /* Statically linked binaries may not use relocation, so RELRO could
> +     remain writable.  For consistency, apply this for static PIE, too
> +     (where there is relocation processing).  */
> +  if (l == GL(dl_ns)[LM_ID_BASE]._ns_loaded)
> +    return dl_readonly_area_writable;

Is this correct? Because if PT_GNU_RELRO exists, either it will be applied
or the process status will be aborted (_dl_protect_relro).  The check_relro
already returns dl_readonly_area_writable on hppa (where relro is not
applied), so I am not sure about adding this snipped for static only.

And what do you mean by relocations here?

> +#endif
>    if (l->l_relro_addr != 0)
>      {
>        uintptr_t relro_start = ALIGN_DOWN (l->l_addr + l->l_relro_addr,
> 
> base-commit: 1944817240eeea5c044995e45a771a5a5193ecf1
>