helper ynh_string_random returns empty string on length > 254

chri2 commented 2 months ago

Describe the bug

Using helper ynh_string_random with argument -l <length> and length>254 returns silently empty string.

CI advices not to use openssl rand.

To reproduce

On a yunohost:

root@yt:~# . /usr/share/yunohost/helpers.d/string 
+ . /usr/share/yunohost/helpers.d/string
root@yt:~# . /usr/share/yunohost/helpers.d/getopts 
+ . /usr/share/yunohost/helpers.d/getopts
root@yt:~# ynh_string_random -l 255 -f 'a-zA-Z0-9/+'
+ ynh_string_random -l 255 -f a-zA-Z0-9/+
+ local legacy_args=lf
+ args_array=(['l']='length=' ['f']='filter=')
+ local -A args_array
+ local length
+ local filter
+ ynh_handle_getopts_args -l 255 -f a-zA-Z0-9/+
+ set +o xtrace
+ length=255
+ filter=a-zA-Z0-9/+
+ tr --complement --delete a-zA-Z0-9/+
+ sed --quiet 's/\(.\{255\}\).*/\1/p'
+ dd if=/dev/urandom bs=1 count=1000
root@yt:~# ynh_string_random -l 254 -f 'a-zA-Z0-9/+'
+ ynh_string_random -l 254 -f a-zA-Z0-9/+
+ local legacy_args=lf
+ args_array=(['l']='length=' ['f']='filter=')
+ local -A args_array
+ local length
+ local filter
+ ynh_handle_getopts_args -l 254 -f a-zA-Z0-9/+
+ set +o xtrace
+ length=254
+ filter=a-zA-Z0-9/+
+ dd if=/dev/urandom bs=1 count=1000
+ sed --quiet 's/\(.\{254\}\).*/\1/p'
+ tr --complement --delete a-zA-Z0-9/+
Kq+Bon6aR10OODY3MvF0JnfstaehCMsbqfdst2BMChfQroTFrViTdSrxObm8M7XPoznWDgbktlhoYR5UZ+gbth3sJlXlLTWhqt/PiK0oY1ulhwSCzxHl7rz3q6PqcXCt+mf8z3IYpa407t/K7lm0x8KnVevZBpOtaUYA4Vp1le0a/QuXATz5ZdDv4y4tKFWkpjMpp7kM53UmlviYUhb64ymz6QWE4KsNwa+3L8kdcBHHvSycM7VntGhgYL1ulF

Expected behavior

ynh_string_random should return a random string as described in the docs or at least return an error if it can't.

Security implication

Any app that relied on this function when generating a random string with more than 254 characters as demonstrated above is using an empty string instead. E.g. as a super save extra long password somewhere or as a kind of secret cookie or token.

chri2 commented 2 months ago

man 7 regex

A bound is '{' followed by an unsigned decimal integer, possibly followed by ',' possibly followed by another unsigned decimal integer, always followed by '}'. The integers must lie between 0 and RE_DUP_MAX (255(!)) inclusive

chri2 commented 2 months ago

man 7 regex

A bound is '{' followed by an unsigned decimal integer, possibly followed by ',' possibly followed by another unsigned decimal integer, always followed by '}'. The integers must lie between 0 and RE_DUP_MAX (255(!)) inclusive

It turned out that sed seems to include a library not covered by the man page or that I misunderstood the meaning of the man page.

The following test shows that sed might not be the problem:

for I in `seq 200 3000`; do echo -n "$I "; printf '=%.0s' $(seq 0 $[I+10]) | sed --quiet "s/\(.\{$I\}\).*/\1/p" | wc -c; done

If sed does like expected still there is the lottery whether the length of 1000 bytes from urandom contains enough data falling in the range of --filter to fill up the asked --length.

YunoHost / issues