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

[Adhemerval Zanella Netto]

On 24/07/25 10:08, Adhemerval Zanella Netto wrote:
> 
> 
> 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.

I think the correct fix is indeed make the test rely on PT_GNU_RELRO,
it is supported on all architectures even though not default.

> 
> And what do you mean by relocations here?