Re: [PATCH 2/2] posix: Improve randomness on try_tempname_len

[Adhemerval Zanella]

On 08/01/2021 23:20, Paul Eggert wrote:
> On 1/4/21 9:03 AM, Adhemerval Zanella wrote:
>> For __GT_NOCREATE (mktemp, tempnam, tmpnam) getrandom is also used
>> on first try, otherwise randomness is obtained using the clock plus
>> a linear congruential generator.
> 
> Why not use getrandom in the first try also for __GT_DIR (mkdtemp) and 
> __GT_FILE (mkostemp, mkostemps, mkstemp, mkstemps, tmpfile)? That is what 
> Gnulib tempname.c is doing now. This not only simplifies the code, it 
> improves resistance to some (admittedly less-likely) attacks.

The idea is to always issue getrandom for __GT_DIR or __GT_FILE on first try,
as you suggested initially [1].  I followed your idea [2]:

  Here's an idea: use getrandom in the first try only for the __GT_NOCREATE 
case. 
  Although a bit more complicated, I expect this would address both your 
entropy 
  and my security concerns.

The current code should address Jakub concerns of using getrandom without 
GRND_NONBLOCK and not using on on first try (to avoid deplete the random 
entropy pool) and use getrandom only when a collision if found. I will merge
the code, close the bug, and we can work whether use getrandom only for
__GT_DIR/__GT_FILE is an improvement or not.

> 
>> Also for getrandom GRND_NONBLOCK is used to avoid blocking indefinitely
>> on some older kernels.
> 
> Thanks, I installed that part of the proposal into Gnulib by installing the 
> attached. The idea is for tempname.c to be identical after we get the 
> abovementioned issue worked out.

[1] https://sourceware.org/pipermail/libc-alpha/2020-September/117535.html
[2] https://sourceware.org/pipermail/libc-alpha/2020-September/117539.html

Re: [PATCH 1/2] posix: Sync tempname with gnulib [BZ #26648]

[Adhemerval Zanella]

On 08/01/2021 22:58, Paul Eggert wrote:
> On 1/4/21 9:03 AM, Adhemerval Zanella wrote:
> 
>> -# define __lstat64(version, file, buf) lstat (file, buf)
>> +# define __lxstat64(version, file, buf) lstat (file, buf)
> 
> That change isn't quite right for the !_LIBC case, since it doesn't define 
> the __stat64 macro and it defines __lstat64 with the wrong API.
> 
> I installed the attached patch into Gnulib, which should do the change right 
> for both Gnulib and glibc, the idea being that the Gnulib and glibc source 
> files can be identical.

Indeed, I forgot that these symbols are still provided internally.

Re: [PATCH 2/3] posix: Remove alloca usage on regex build_trtable

[Adhemerval Zanella]

On 08/01/2021 19:30, Paul Eggert wrote:
> On 1/6/21 10:17 AM, Adhemerval Zanella wrote:
>> __libc_use_alloca/alloca is replaced with malloc regardless.
> 
> These allocations are so small that they should be put on the stack instead 
> of using malloc. I did that in Gnulib by installing the attached patch. The 
> idea is that the resulting regexec.c file should be copyable unchanged into 
> glibc.
> 
> From a Gnulib point of view this code uses a 20 KiB frame (on a 64-bit host) 
> which goes past the usual 4032-byte limit for stack frames, but I think we 
> can stretch the point here.  In glibc the limit is 64 KiB so there's no 
> problem.

Right, I think we can maybe use scratch_buffer for these or even a dynamic array
with a more proper initial size as an improvement.

Re: [PATCH 1/3] posix: Remove alloca usage on regex set_regs

[Adhemerval Zanella]

On 08/01/2021 17:14, Paul Eggert wrote:
> On 1/6/21 10:17 AM, Adhemerval Zanella wrote:
>> It replaces the regmatch_t with a dynarray list.
> 
> regexec.c is shared with Gnulib, so some work needed to be done on the Gnulib 
> side for this patch since Gnulib didn't have dynarray. Dynarray is something 
> I've been meaning to add to Gnulib for some time, so I did that by installing 
> the first attached patch into Gnulib. Could you please propagate the new 
> Gnulib dynarray sources into glibc so that they stay in sync? As near as I 
> can make out, the glibc dynarray files can now be identical to the new Gnulib 
> files; if not, please let me know.

I will check and sync the differences.

> 
> 
>>   posix/regexec.c | 62 -
>>   1 file changed, 31 insertions(+), 31 deletions(-)
>> ...
>> @@ -1355,6 +1352,16 @@ pop_fail_stack (struct re_fail_stack_t *fs, Idx 
>> *pidx, Idx nregs,
>>     return fs->stack[num].node;
>>   }
>>   +
>> +#define DYNARRAY_STRUCT  regmatch_list
>> +#define DYNARRAY_ELEMENT regmatch_t
>> +#define DYNARRAY_PREFIX  regmatch_list_
>> +#include 
>> +
>> +static void update_regs (const re_dfa_t *dfa, regmatch_t *pmatch,
>> + struct regmatch_list *prev_idx_match, Idx cur_node,
>> + Idx cur_idx, Idx nmatch);
>> +
>>   /* Set the positions where the subexpressions are starts/ends to registers
>>  PMATCH.
>>  Note: We assume that pmatch[0] is already set, and
>> @@ -1370,8 +1377,8 @@ set_regs (const regex_t *preg, const 
>> re_match_context_t *mctx, size_t nmatch,
>>     re_node_set eps_via_nodes;
>>     struct re_fail_stack_t *fs;
>>     struct re_fail_stack_t fs_body = { 0, 2, NULL };
>> -  regmatch_t *prev_idx_match;
>> -  bool prev_idx_match_malloced = false;
>> +  struct regmatch_list prev_idx_match;
>> +  regmatch_list_init (&prev_idx_match);
>>       DEBUG_ASSERT (nmatch > 1);
>>     DEBUG_ASSERT (mctx->state_log != NULL);
>> @@ -1388,23 +1395,18 @@ set_regs (const regex_t *preg, const 
>> re_match_context_t *mctx, size_t nmatch,
>>     cur_node = dfa->init_node;
>>     re_node_set_init_empty (&eps_via_nodes);
>>   -  if (__libc_use_alloca (nmatch * sizeof (regmatch_t)))
>> -    prev_idx_match = (regmatch_t *) alloca (nmatch * sizeof (regmatch_t));
>> -  else
>> +  if (!regmatch_list_resize (&prev_idx_match, nmatch))
>>   {
>> -  prev_idx_match = re_malloc (regmatch_t, nmatch);
>> -  if (prev_idx_match == NULL)
>> -    {
>> -  free_fail_stack_return (fs);
>> -  return REG_ESPACE;
>> -    }
>> -  prev_idx_match_malloced = true;
>> +  regmatch_list_free (&prev_idx_match);
>> +  free_fail_stack_return (fs);
>> +  return REG_ESPACE;
>>   }
> 
> These three hunks are good, but you can omit most of the other hunks (and 
> improve performance a bit) by inserting the following line after the 3rd hunk:
> 
> +  regmatch_t *prev_idx_match = regmatch_list_begin (&prev_match);
> 
> since the dynarray doesn't grow after that and this means you don't need to 
> change the rest of the code to use prev_match rather than prev_idx_match. The 
> only other hunks you need to retain are the ones replacing re_free with 
> regmastch_list_free.
> 
> I've made this improvement to Gnulib by installing the second attached patch, 
> so you should be able to copy Gnulib regexec.c to glibc without changing it.

Ok, I will check and sync with gnulib.

Re: [PATCH 3/3] posix: Remove alloca definition from regex

[Adhemerval Zanella]

On 08/01/2021 22:20, Paul Eggert wrote:
> This patch looks good for glibc, once the previous two regex patches are 
> done. I installed it into Gnulib by applying the attached, so that 
> regex_internal.h can stay in lock-step between Gnulib and glibc.

Right, I will sync the regex code with gnulib.  Thanks for checking
on this.

Re: rcs configure hang

[Kelly Wang (kellythw)]

Hi Thien,

Thanks for follow up. Here is my reply on 11/10/2020.

Thanks for checking. Here is my system info:
% uname -a
Linux sjc-ads-7913 4.18.0-147.3.1.el8_1.x86_64 #1 SMP Wed Nov 27 01:11:44 UTC 
2019 x86_64 unknown unknown GNU/Linux
% rpm -q kernel
kernel-4.18.0-80.el8.x86_64
kernel-4.18.0-147.3.1.el8_1.x86_64

This is virtualization system.

Thanks,
Kelly
 
If you need support for DevX Tools:   http://devxsupport.cisco.com/
Specifically, for NXOS, see -
https://wiki.cisco.com/display/NEXUSPMO/ContactingNexusOpsAndTools
 

On 1/10/21, 2:07 PM, "Thien-Thi Nguyen"  wrote:


() Florian Weimer 
() Mon, 09 Nov 2020 10:14:00 +0100

   Would you be able to share details of the file system used
   (XFS or something else?) and the kernel version (uname -a,
   rpm -q kernel)?

   Do you use virtualization or containers?

   I would like to see if I can reproduce it internally.

Ping.  (Any news?)

-- 
Thien-Thi Nguyen ---
 (defun responsep (query)   ; (2021) Software Libero
   (pcase (context query)   ;   = Dissenso Etico
 (`(technical ,ml) (correctp ml))
 ...))  748E A0E8 1CB8 A748 9BFA
--- 6CE4 6703 2224 4C80 7502

Re: rcs configure hang

[Florian Weimer]

* Kelly Wang:

> Hi Florian,
>
> Thanks for checking. Here is my system info:
> % uname -a
> Linux sjc-ads-7913 4.18.0-147.3.1.el8_1.x86_64 #1 SMP Wed Nov 27 01:11:44 UTC 
> 2019 x86_64 unknown unknown GNU/Linux
> % rpm -q kernel
> kernel-4.18.0-80.el8.x86_64
> kernel-4.18.0-147.3.1.el8_1.x86_64
>
> This is virtualization system.

What's the hypervisor?

(It may also be worthwhile to try with a more recent .el8 kernel.)

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 2/2] posix: Improve randomness on try_tempname_len

[Paul Eggert]

On 1/11/21 4:29 AM, Adhemerval Zanella wrote:


The idea is to always issue getrandom for __GT_DIR or __GT_FILE on first try,
as you suggested initially [1].  I followed your idea [2]:... > [1] 

https://sourceware.org/pipermail/libc-alpha/2020-September/117535.html

[2] https://sourceware.org/pipermail/libc-alpha/2020-September/117539.html


Ah, thanks, I'd forgotten about that conversation.

I looked at the patch 
 
again. A couple of small things. First, it uses bool so needs to include 
stdbool.h. Second, the generated code's a bit smaller if we call 
random_bits only once. I added those two changes and installed the 
attached patch to Gnulib master on savannah, with the idea being that 
Gnulib's tempname.c can be identical to glibc's.
From 23c0672c281a949c254ef0c173eab987ab876e29 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Mon, 11 Jan 2021 16:46:12 -0800
Subject: [PATCH] tempname: consume less entropy

Derived from a glibc patch proposed by Adhemerval Zanella in:
https://sourceware.org/pipermail/libc-alpha/2021-January/121302.html
* lib/tempname.c: Include stdbool.h.
(random_bits): New arg use_getrandom.
(try_tempname_len): Skip getrandom on the first try,
unless __GT_NOCREATE.
* modules/tempname (Depends-on): Add stdbool.
---
 ChangeLog| 11 +++
 lib/tempname.c   | 17 ++---
 modules/tempname |  1 +
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c6f5295b4..0d0144242 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2021-01-11  Paul Eggert  
+
+	tempname: consume less entropy
+	Derived from a glibc patch proposed by Adhemerval Zanella in:
+	https://sourceware.org/pipermail/libc-alpha/2021-January/121302.html
+	* lib/tempname.c: Include stdbool.h.
+	(random_bits): New arg use_getrandom.
+	(try_tempname_len): Skip getrandom on the first try,
+	unless __GT_NOCREATE.
+	* modules/tempname (Depends-on): Add stdbool.
+
 2021-01-10  Bruno Haible  
 
 	lchmod-tests: Fix link error.
diff --git a/lib/tempname.c b/lib/tempname.c
index f199b25a7..5f804b38d 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -22,6 +22,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -77,11 +78,11 @@ typedef uint_fast64_t random_value;
 #define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
 
 static random_value
-random_bits (random_value var)
+random_bits (random_value var, bool use_getrandom)
 {
   random_value r;
   /* Without GRND_NONBLOCK it can be blocked for minutes on some systems.  */
-  if (__getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r)
+  if (use_getrandom && __getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r)
 return r;
 #if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
   /* Add entropy if getrandom did not work.  */
@@ -269,6 +270,13 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   /* How many random base-62 digits can currently be extracted from V.  */
   int vdigits = 0;
 
+  /* Whether to consume entropy when acquiring random bits.  On the
+ first try it's worth the entropy cost with __GT_NOCREATE, which
+ is inherently insecure and can use the entropy to make it a bit
+ less secure.  On the (rare) second and later attempts it might
+ help against DoS attacks.  */
+  bool use_getrandom = tryfunc == try_nocreate;
+
   /* Least unfair value for V.  If V is less than this, V can generate
  BASE_62_DIGITS digits fairly.  Otherwise it might be biased.  */
   random_value const unfair_min
@@ -292,7 +300,10 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   if (vdigits == 0)
 {
   do
-v = random_bits (v);
+{
+  v = random_bits (v, use_getrandom);
+  use_getrandom = true;
+}
   while (unfair_min <= v);
 
   vdigits = BASE_62_DIGITS;
diff --git a/modules/tempname b/modules/tempname
index 27b0d3d23..4779735d9 100644
--- a/modules/tempname
+++ b/modules/tempname
@@ -17,6 +17,7 @@ libc-config
 lstat
 mkdir
 stdalign
+stdbool
 stdint
 sys_stat
 time
-- 
2.27.0