Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
Hello Jason,
Am Fri, Apr 29, 2022 at 07:04:47PM +0200 schrieb Jason A. Donenfeld:
> Rather than having getrandom() be called in a loop that handles EINTR --
> which would require more code bloat -- we just limit the maximum seed
> size to 256 bytes, which the kernel guarantees won't be interrupted.
> Additionally document the flock() and fsync() usage so that somebody
> doesn't remove it. Apparently busybox developers like to remove things
> they don't understand with no regards to security implications, so Denys
> suggested I leave some comments here.
>
> Cc: Denys Vlasenko
> Cc: Bernhard Reutner-Fischer
> Signed-off-by: Jason A. Donenfeld
> ---
> util-linux/seedrng.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/util-linux/seedrng.c b/util-linux/seedrng.c
> index c42274759..5ee6197e3 100644
> --- a/util-linux/seedrng.c
> +++ b/util-linux/seedrng.c
> @@ -56,7 +56,7 @@
>
> enum {
> MIN_SEED_LEN = SHA256_OUTSIZE,
> - MAX_SEED_LEN = 512
> + MAX_SEED_LEN = 256 /* Maximum size of getrandom() call without EINTR. */
> };
>
> static size_t determine_optimal_seed_len(void)
> @@ -142,6 +142,8 @@ static int seed_from_file_if_exists(const char *filename,
> int dfd, bool credit,
> bb_perror_msg("can't%s seed", " read");
> return -1;
> }
> + /* The fsync() here is necessary for safety here, so that power being
> pulled
> + * at the wrong moment doesn't result in the seed being used twice by
> accident. */
nit: "here" is used twice in the same sentence.
Greets
Alex
> if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
> bb_perror_msg("can't%s seed", " remove");
> return -1;
> @@ -190,6 +192,8 @@ int seedrng_main(int argc UNUSED_PARAM, char *argv[])
> if (mkdir(seed_dir, 0700) < 0 && errno != EEXIST)
> bb_perror_msg_and_die("can't %s seed directory", "create");
> dfd = open(seed_dir, O_DIRECTORY | O_RDONLY);
> + /* The flock() here is absolutely necessary, as the consistency of this
> + * program breaks down with concurrent uses. */
> if (dfd < 0 || flock(dfd, LOCK_EX) < 0)
> bb_perror_msg_and_die("can't %s seed directory", "lock");
> xfchdir(dfd);
> @@ -222,6 +226,8 @@ int seedrng_main(int argc UNUSED_PARAM, char *argv[])
>
> printf("Saving %u bits of %screditable seed for next boot\n",
> (unsigned)new_seed_len * 8, new_seed_creditable ? "" : "non-");
> fd = open(NON_CREDITABLE_SEED_NAME, O_WRONLY | O_CREAT | O_TRUNC, 0400);
> + /* The fsync() here is necessary to ensure the data is written to disk
> before
> + * we attempt to make it creditable. */
> if (fd < 0 || full_write(fd, new_seed, new_seed_len) !=
> (ssize_t)new_seed_len || fsync(fd) < 0) {
> bb_perror_msg("can't%s seed", " write");
> return program_ret | (1 << 4);
> --
> 2.35.1
>
> ___
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
Hello Denys, Am Sat, Apr 30, 2022 at 03:12:11PM +0200 schrieb Denys Vlasenko: > Do you often pull power cords from machines you use for > somewhat important crypto operations, such as generating keys? > What are the chances that you also do it on a machine without RTC > clock (which would otherwise supply randomness > from the current time)? FWIW … busybox is often used in embedded systems, and from my personal 10y+ experience in that field, I can safely assure you: customers and users pull power chords all the time and at absolutely unpredictable times. Don't know if that's a real threat from a security point of view, but only looking at what developers do is not giving you the full picture. Greets Alex ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Let users access their home directories via ftp
I use a similar patch, I added option -h to enable the functionality.
On Sun, May 1, 2022 at 2:17 PM Aleksander Mazur wrote:
>
> Hi,
>
> AFAIU ftpd just shares current working directory (unless given a path), no
> matter who logs in.
> I find it useful to let ftpd chroot or cd to the home directory of a
> (non-root) user who logs in.
> Please consider attached patch. I hope it won't ruin anybody's setup.
>
> --
> Aleksander Mazur
> ___
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
diff -Nru busybox-1.33.1.orig/networking/ftpd.c busybox-1.33.1/networking/ftpd.c
--- busybox-1.33.1.orig/networking/ftpd.c 2021-01-01 02:52:27.0 -0800
+++ busybox-1.33.1/networking/ftpd.c 2021-08-23 22:12:23.582753732 -0700
@@ -1164,9 +1164,10 @@
#endif
BIT_A =(!BB_MMU) * 2,
OPT_A = (1 << (BIT_A + 0)),
- OPT_v = (1 << (BIT_A + 1)),
- OPT_S = (1 << (BIT_A + 2)),
- OPT_w = (1 << (BIT_A + 3)) * ENABLE_FEATURE_FTPD_WRITE,
+ OPT_h = (1 << (BIT_A + 1)),
+ OPT_v = (1 << (BIT_A + 2)),
+ OPT_S = (1 << (BIT_A + 3)),
+ OPT_w = (1 << (BIT_A + 4)) * ENABLE_FEATURE_FTPD_WRITE,
};
int ftpd_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
@@ -1186,7 +1187,7 @@
verbose_S = 0;
G.timeout = 2 * 60;
#if BB_MMU
- opts = getopt32(argv, "^" "AvS" IF_FEATURE_FTPD_WRITE("w")
+ opts = getopt32(argv, "^" "AhvS" IF_FEATURE_FTPD_WRITE("w")
"t:+T:+" IF_FEATURE_FTPD_AUTHENTICATION("a:")
"\0" "vv:SS",
, _timeout, IF_FEATURE_FTPD_AUTHENTICATION(_opt,)
@@ -1295,7 +1296,7 @@
G.root_fd = -1;
#endif
argv += optind;
- if (argv[0]) {
+ if (argv[0] && (pw == NULL || !(opts & OPT_h))) {
const char *basedir = argv[0];
#if !BB_MMU
G.root_fd = xopen("/", O_RDONLY | O_DIRECTORY);
@@ -1319,8 +1320,14 @@
}
#if ENABLE_FEATURE_FTPD_AUTHENTICATION
- if (pw)
+ if (pw) {
+ const char *basedir = pw->pw_dir;
+ if ((opts & OPT_h) && chroot(pw->pw_dir) == 0)
+ basedir = "/";
change_identity(pw);
+ if (opts & OPT_h)
+ xchdir(basedir);
+ }
/* else: -A is in effect */
#endif
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
On Sun, May 1, 2022 at 6:35 PM Emmanuel Deloget wrote: > > > - RNG is seeded and credited using file A. > > > - File A is unlinked but not fsync()d. > > > - TLS connection does something and a nonce is generated. > > > - System loses power and reboots. > > > - RNG is seeded and credited using same file A. > > > - TLS connection does something and the same nonce is generated, > > > resulting in catastrophic cryptographic failure. > > > > > > This is a big deal. Crediting seeds is not to be taken lightly. > > > Crediting file-based seeds is _only_ safe when they're never used > > > twice. > > > > Using the same file twice is better than having nothing at all. > > I beg to differ, and especially on some embedded systems where the RNG > might be quite controllable by an attacker from the outside (mostly because > it lacks a lot of entropy crediting inputs, which is exactly the reason why we > need seedrng in the first place). This may lead to catastrophic cryptography > failures down the road. Did you personally encounter such a situation? I'm not implying it's not really happening, I'm interested to hear from people who met this situation in real world. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Let users access their home directories via ftp
Hi,
AFAIU ftpd just shares current working directory (unless given a path), no
matter who logs in.
I find it useful to let ftpd chroot or cd to the home directory of a (non-root)
user who logs in.
Please consider attached patch. I hope it won't ruin anybody's setup.
--
Aleksander Mazur
--- a/networking/ftpd.c
+++ b/networking/ftpd.c
@@ -1295,8 +1295,14 @@
G.root_fd = -1;
#endif
argv += optind;
- if (argv[0]) {
- const char *basedir = argv[0];
+ const char *basedir = argv[0];
+#if ENABLE_FEATURE_FTPD_AUTHENTICATION
+ if (!basedir && pw && pw->pw_uid) {
+ /* Non-root user logged in */
+ basedir = pw->pw_dir;
+ }
+#endif
+ if (basedir) {
#if !BB_MMU
G.root_fd = xopen("/", O_RDONLY | O_DIRECTORY);
close_on_exec_on(G.root_fd);
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v4] tree: new applet
On Mon, 2 May 2022 01:40:39 +0800 Kang-Che Sung wrote: > Hi Roger, > > May I suggest you add an option to draw the tree lines using ASCII > characters only, and make the Unicode support optional at build time? > > I just feel uncomfortable when I see source code containing embedded UTF-8 > characters and the strings have no ASCII alternative. > > The DOS tree command has the "/a" option. You know what I mean. > > Thank you. Hi, something like ?: #ifdef CONFIG_TREE_ASCII_ONLY #define PREFIX_CHILD "|- " #define PREFIX_LAST_CHILD "|_ " #define PREFIX_GRAND_CHILD "| " #define PREFIX_LAST_GRAND_CHILD " " #else #define PREFIX_CHILD "├── " #define PREFIX_LAST_CHILD "└── " #define PREFIX_GRAND_CHILD "│ " #define PREFIX_LAST_GRAND_CHILD "" #endif Ciao, Tito > On Sunday, May 1, 2022, Roger Knecht wrote: > > Add new applet which resembles the MS-DOS tree program to list > directories and files in a tree structure. > > > > function old new delta > > tree_print - 388+388 > > .rodata95678 95767 +89 > > tree_main - 73 +73 > > tree_print_prefix - 28 +28 > > packed_usage 34417 34429 +12 > > globals- 8 +8 > > applet_main 31923200 +8 > > applet_names27472752 +5 > > > -- > > (add/remove: 5/0 grow/shrink: 4/0 up/down: 611/0) Total: 611 > bytes > > > > Signed-off-by: Roger Knecht > > --- > > Changelog: > > > > V4: > > - Rephrase commit message > > - Updated bloatcheck to latest master > > > > V3: > > - Fixed symlink handling > > - Handle multiple directories in command line arguments > > - Extended tests for symlink and multiple directories > > - Reduced size by using libbb functions > > > > V2: > > - Fixed tree help text > > - Reduced size by 644 bytes > > ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v4] tree: new applet
Hi Roger, May I suggest you add an option to draw the tree lines using ASCII characters only, and make the Unicode support optional at build time? I just feel uncomfortable when I see source code containing embedded UTF-8 characters and the strings have no ASCII alternative. The DOS tree command has the "/a" option. You know what I mean. Thank you. On Sunday, May 1, 2022, Roger Knecht wrote: > Add new applet which resembles the MS-DOS tree program to list directories and files in a tree structure. > > function old new delta > tree_print - 388+388 > .rodata95678 95767 +89 > tree_main - 73 +73 > tree_print_prefix - 28 +28 > packed_usage 34417 34429 +12 > globals- 8 +8 > applet_main 31923200 +8 > applet_names27472752 +5 > -- > (add/remove: 5/0 grow/shrink: 4/0 up/down: 611/0) Total: 611 bytes > > Signed-off-by: Roger Knecht > --- > Changelog: > > V4: > - Rephrase commit message > - Updated bloatcheck to latest master > > V3: > - Fixed symlink handling > - Handle multiple directories in command line arguments > - Extended tests for symlink and multiple directories > - Reduced size by using libbb functions > > V2: > - Fixed tree help text > - Reduced size by 644 bytes > ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v4] tree: new applet
On Sun, 01 May 2022 12:51:16 +
Roger Knecht wrote:
> Add new applet which resembles the MS-DOS tree program to list directories
> and files in a tree structure.
>
> function old new delta
> tree_print - 388+388
> .rodata95678 95767 +89
> tree_main - 73 +73
> tree_print_prefix - 28 +28
> packed_usage 34417 34429 +12
> globals- 8 +8
> applet_main 31923200 +8
> applet_names27472752 +5
> --
> (add/remove: 5/0 grow/shrink: 4/0 up/down: 611/0) Total: 611 bytes
>
> Signed-off-by: Roger Knecht
> ---
> Changelog:
>
> V4:
> - Rephrase commit message
> - Updated bloatcheck to latest master
>
> V3:
> - Fixed symlink handling
> - Handle multiple directories in command line arguments
> - Extended tests for symlink and multiple directories
> - Reduced size by using libbb functions
>
> V2:
> - Fixed tree help text
> - Reduced size by 644 bytes
>
> AUTHORS | 3 +
> miscutils/tree.c | 135 +++
> testsuite/tree.tests | 97 +++
> 3 files changed, 235 insertions(+)
> create mode 100644 miscutils/tree.c
> create mode 100755 testsuite/tree.tests
>
> diff --git a/AUTHORS b/AUTHORS
> index 5c9a634c9..9ec0e2ee4 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -181,3 +181,6 @@ Jie Zhang
>
> Maxime Coste
> paste implementation
> +
> +Roger Knecht
> +tree
> diff --git a/miscutils/tree.c b/miscutils/tree.c
> new file mode 100644
> index 0..e053e8483
> --- /dev/null
> +++ b/miscutils/tree.c
> @@ -0,0 +1,135 @@
> +/* vi: set sw=4 ts=4: */
> +/*
> + * Copyright (C) 2022 Roger Knecht
> + *
> + * Licensed under GPLv2, see file LICENSE in this source tree.
> + */
> +//config:config TREE
> +//config:bool "tree (0.6 kb)"
> +//config:default n
> +//config:help
> +//config:List files and directories in a tree structure.
> +//config:
> +
> +//applet:IF_TREE(APPLET(tree, BB_DIR_USR_BIN, BB_SUID_DROP))
> +
> +//kbuild:lib-$(CONFIG_TREE) += tree.o
> +
> +//usage:#define tree_trivial_usage NOUSAGE_STR
> +//usage:#define tree_full_usage ""
> +
> +#include "libbb.h"
> +
> +#define PREFIX_CHILD "├── "
> +#define PREFIX_LAST_CHILD "└── "
> +#define PREFIX_GRAND_CHILD "│ "
> +#define PREFIX_LAST_GRAND_CHILD ""
> +#define DEFAULT_PATH "."
> +
> +struct directory {
> + struct directory* parent;
> + const char* prefix;
> +};
> +
> +static struct globals {
> + int directories;
> + int files;
> +} globals;
> +
> +static void tree_print_prefix(struct directory* directory) {
> + if (directory) {
> + tree_print_prefix(directory->parent);
> + fputs_stdout(directory->prefix);
> + }
> +}
> +
> +static void tree_print(const char* directory_name, struct directory*
> directory) {
> + struct dirent **entries, *dirent;
> + struct directory child_directory;
> + char* symlink_path;
> + int index, size;
//> + bool is_not_last, is_file;
> +
> + // read directory entries
> + size = scandir(directory_name, , NULL, alphasort);
> +
> + if (size < 0) {
> + fputs_stdout(directory_name);
> + puts(" [error opening dir]");
> + return;
> + }
> +
> + // print directory name
> + puts(directory_name);
> +
> + // switch to sub directory
> + xchdir(directory_name);
> +
> + child_directory.parent = directory;
> +
> + // print all directory entries
> + for (index = 0; index < size; index++) {
> + dirent = entries[index];
> +
> + // filter hidden files and directories
Hi,
if (dirent->d_name[0] != '.') {
> + if (strncmp(dirent->d_name, ".", 1) != 0) {
twisted logic?
> + is_file = !is_directory(dirent->d_name, 1);
is_not_last is used only once
> + is_not_last = (index + 1) < size;
> + symlink_path = xmalloc_readlink(dirent->d_name);
> +
> + // print tree line prefix
> + tree_print_prefix(directory);
> +
if ((index + 1) < size) {
> + if (is_not_last) {
> + child_directory.prefix = PREFIX_GRAND_CHILD;
> + fputs_stdout(PREFIX_CHILD);
> + } else {
> + child_directory.prefix =
> PREFIX_LAST_GRAND_CHILD;
> +
Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
I'm not sure I have any right to jump into the conversation but here I am.
Le dim. 1 mai 2022 à 15:07, David Laight a écrit :
>
> From: Jason A. Donenfeld
> > Sent: 30 April 2022 14:48
> >
> > Hi Denys,
> >
> > On Sat, Apr 30, 2022 at 3:12 PM Denys Vlasenko
> > wrote:
> > > > + /* The fsync() here is necessary for safety here, so that power
> > > > being pulled
> > > > +* at the wrong moment doesn't result in the seed being used
> > > > twice by accident. */
> > > > if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
> > > > bb_perror_msg("can't%s seed", " remove");
> > > > return -1;
> > >
> > > I don't understand the scenario mentioned here.
> > > Let's say we removed fsync() above.
> > > We read /var/lib/seedrng/seed.credit contents.
> > > We unlink() it, but this change does not go to disk yet.
> > > We seed the RNG.
> > > We recreate /var/lib/seedrng/seed.credit with new contents,
> > > but this change does not go to disk yet too.
> > > We exit.
> > > Only now, after we are done, RNG can be used for e.g. key generation.
> > >
> > > And only if the power gets pulled AFTER this key generation,
> > > and the write cached data is still not on the disk
> > > (for example, if you use ext4 or xfs, this won't happen since
> > > they synchronously wait for both O_TRUNC truncations
> > > and renames), the previous /var/lib/seedrng/seed.credit might
> > > still exist and might get reused on the next boot,
> > > and a new key can be generated from the same seed.
> > >
> > > Do you often pull power cords from machines you use for
> > > somewhat important crypto operations, such as generating keys?
> > > What are the chances that you also do it on a machine without RTC
> > > clock (which would otherwise supply randomness
> > > from the current time)?
> > >
> > > IOW: To me, this does not seem to be a realistic threat.
> > > It's a theoretical one: you can concoct a scenario where it might happen,
> > > but triggering it for real, even on purpose, would be difficult.
> >
> > Again, stop charging steadfastly toward creating vulnerabilities
> > because you don't understand things. The scenario is:
> >
> > - RNG is seeded and credited using file A.
> > - File A is unlinked but not fsync()d.
> > - TLS connection does something and a nonce is generated.
> > - System loses power and reboots.
> > - RNG is seeded and credited using same file A.
> > - TLS connection does something and the same nonce is generated,
> > resulting in catastrophic cryptographic failure.
> >
> > This is a big deal. Crediting seeds is not to be taken lightly.
> > Crediting file-based seeds is _only_ safe when they're never used
> > twice.
>
> Using the same file twice is better than having nothing at all.
I beg to differ, and especially on some embedded systems where the RNG
might be quite controllable by an attacker from the outside (mostly because
it lacks a lot of entropy crediting inputs, which is exactly the reason why we
need seedrng in the first place). This may lead to catastrophic cryptography
failures down the road.
> At least different systems use different values.
> Unless you have a remote 'dos' attack that can crash the system
> at exactly the right point in the boot sequence this is an
> entirely 'academic' error.
Having such a distinction is less helpful than one might think. First of all,
even an 'academic' vulnerability is a vulnerability. It means that basing any
embedded product on busybox's seedrng precludes said product from
being acceptable for any kind of security certification program. It doesn't
matter whether the vuln is exploitable or not at the time of certification - the
reasoning is that it's a known vuln, and it _might_ be exploitable in
the future.
Official bodies might be paranoïd here, but they have the last word.
> What is much more likely is that the file where the entropy
> is saved is just a memory overlay on top of a read-only image.
>
> That is much more likely for an embedded system than any of
> the 'failure' cases you've considered.
Given the right tools and how to use them, we developers have a duty
to use them correctly. I understand that it's not always the case, but this
is our responsibility - not the responsibility of the tool maker. If I design
a product without (at least) one way to permanently store a seed file
then I shall provide another way to seed the RNG.
Fortunately, a lot of recent SoC have a hardware RNG which will
participate to /dev/random early on.
> I also wonder how sane it is to do 'new_key = f(old_key)'.
> That doesn't seem significantly better than using the same key.
f() being a hash function with a salt + a (maybe not enough)
random value, I would think that new_key is going to be widely
different than old_key and more or less unpredictable, which is
what we want for a seed.
> For a really embedded system the only persistent storage
> could easily be a small serial EEPROM with a very limited
> number
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Sun, May 1, 2022 at 10:05 AM tito wrote: > > On Fri, 29 Apr 2022 18:16:41 +0200 > Denys Vlasenko wrote: > > > On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote: > > > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote: > > > > Can we replace all [s]size_t's with ints/unsigneds? We do not expect > > > > random pools anywhere near 4 gigabytes... > > > > > > Probably that's fine. Is the advantage to tossing out consistent types > > > worth it though? Does this actually save space? Since [s]size_t is > > > usually the word size, won't codegen not really change much? > > > > For example, on x86-64, 32-bit insns are often shorter. > > ___ > > busybox mailing list > > busybox@busybox.net > > http://lists.busybox.net/mailman/listinfo/busybox > > Hi, > shouldn't that be "was not" exceeded? > > -* genuine entropy: make sure disk is not full, quota was't > esceeded, etc: > +* genuine entropy: make sure disk is not full, quota was't > exceeded, etc: > Yes, a typo. Fixing... ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
From: Jason A. Donenfeld
> Sent: 30 April 2022 14:48
>
> Hi Denys,
>
> On Sat, Apr 30, 2022 at 3:12 PM Denys Vlasenko
> wrote:
> > > + /* The fsync() here is necessary for safety here, so that power
> > > being pulled
> > > +* at the wrong moment doesn't result in the seed being used
> > > twice by accident. */
> > > if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
> > > bb_perror_msg("can't%s seed", " remove");
> > > return -1;
> >
> > I don't understand the scenario mentioned here.
> > Let's say we removed fsync() above.
> > We read /var/lib/seedrng/seed.credit contents.
> > We unlink() it, but this change does not go to disk yet.
> > We seed the RNG.
> > We recreate /var/lib/seedrng/seed.credit with new contents,
> > but this change does not go to disk yet too.
> > We exit.
> > Only now, after we are done, RNG can be used for e.g. key generation.
> >
> > And only if the power gets pulled AFTER this key generation,
> > and the write cached data is still not on the disk
> > (for example, if you use ext4 or xfs, this won't happen since
> > they synchronously wait for both O_TRUNC truncations
> > and renames), the previous /var/lib/seedrng/seed.credit might
> > still exist and might get reused on the next boot,
> > and a new key can be generated from the same seed.
> >
> > Do you often pull power cords from machines you use for
> > somewhat important crypto operations, such as generating keys?
> > What are the chances that you also do it on a machine without RTC
> > clock (which would otherwise supply randomness
> > from the current time)?
> >
> > IOW: To me, this does not seem to be a realistic threat.
> > It's a theoretical one: you can concoct a scenario where it might happen,
> > but triggering it for real, even on purpose, would be difficult.
>
> Again, stop charging steadfastly toward creating vulnerabilities
> because you don't understand things. The scenario is:
>
> - RNG is seeded and credited using file A.
> - File A is unlinked but not fsync()d.
> - TLS connection does something and a nonce is generated.
> - System loses power and reboots.
> - RNG is seeded and credited using same file A.
> - TLS connection does something and the same nonce is generated,
> resulting in catastrophic cryptographic failure.
>
> This is a big deal. Crediting seeds is not to be taken lightly.
> Crediting file-based seeds is _only_ safe when they're never used
> twice.
Using the same file twice is better than having nothing at all.
At least different systems use different values.
Unless you have a remote 'dos' attack that can crash the system
at exactly the right point in the boot sequence this is an
entirely 'academic' error.
What is much more likely is that the file where the entropy
is saved is just a memory overlay on top of a read-only image.
That is much more likely for an embedded system than any of
the 'failure' cases you've considered.
I also wonder how sane it is to do 'new_key = f(old_key)'.
That doesn't seem significantly better than using the same key.
For a really embedded system the only persistent storage
could easily be a small serial EEPROM with a very limited
number of write cycles.
This requires special code to read/write and care to avoid
hitting the write cycle count on a small number of memory cells.
No amount of faffing about with filesystem accesses will
help here at all.
There is also the case (that on my systems at least) udev
initialisation reads from /dev/[u]random well before the S20
script loads any saved entropy.
I've not tried to find out what the value is used for.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
On Sat, Apr 30, 2022 at 11:48 PM Jason A. Donenfeld wrote: > > On Sat, Apr 30, 2022 at 11:19 PM Denys Vlasenko > wrote: > > Thank you for the explanation. I re-adding the fsync > > and adding a comment. Please take a look at current git. > > Oh god, what have you done? You have butchered seedrng into garbage. I > do not agree with the changes you made. You've removed important error > handling in ways that make certain intended use cases absolutely > impossible. Please revert your changes, which you made mid-discussion > here with no agreement reached. Then you can interact on the mailing > list by sending patches and discussing them. If not -- if you want to > keep tumbling down this monstrous route that you're on -- my > participation here ends entirely, and my advice will be to avoid > busybox because its maintainer is a wreckless cowboy. > > Just from a cursory look: > > - You removed the return value check on fsync(dfd), which means the > check is worthless and introduces a security vulnerability. > - You haven't responded to my messages regarding the importance of > returning proper error codes and appear to have removed them entirely > now? > - Your comment about reads from /dev/urandom depleting the entropy > pool isn't correct. (Plus you used an inconsistent type of comment > with bad indentation. Did you even check your work?) > - You completely ignored the `MAX_SEED_LEN = 256` change from the > patch that this thread is actually about, which means there's no > resolution for that issue. Plus you didn't respond to my email where I > discussed various solutions for that matter. Did you read the patch I > sent? In the 12 hours since I sent this to you, not only have you completely failed to address any of those issues (especially the first and fourth), let alone respond to my email, but you've been busy adding another security regression. This time it takes the form of removing consistent encoding of the hash contents. A cryptographic change without any prior mailing list discussion? You truly are a monster. Please just remove seedrng from busybox. I regret ever coming anywhere near this project. You clearly will not be a responsible steward of security-related code. This is only going to lead to bad things for users down the road. Just get rid of the mess you've made, and we can part ways. Thanks, Jason ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] libbb: make '--help' handling more consistent
Running an applet with '--help' as its only argument is treated
as a special case. If additional arguments follow '--help' the
behaviour is inconsistent:
- applets which call single_argv() print help and do nothing else;
- applets which call getopt() report "unrecognized option '--help'"
and print help anyway;
- expr says "expr: syntax error" and doesn't print help;
- printenv silently ignores '--help', prints any other variables
and doesn't print help;
- realpath says "--help: No such file or directory", prints the path
of any other files and doesn't print help.
If the first argument is '--help' ignore any other arguments and print
help. This is more consistent and most likely what the user wanted.
See also commit 6bdfbc4cb (libbb: fix '--help' handling in
FEATURE_SH_NOFORK=y).
function old new delta
show_usage_if_dash_dash_help 75 69 -6
--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-6) Total: -6 bytes
Signed-off-by: Ron Yorston
---
libbb/appletlib.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/libbb/appletlib.c b/libbb/appletlib.c
index 841b3b873..d56b5b409 100644
--- a/libbb/appletlib.c
+++ b/libbb/appletlib.c
@@ -258,7 +258,6 @@ void lbb_prepare(const char *applet
/* Redundant for busybox (run_applet_and_exit covers that case)
* but needed for "individual applet" mode */
if (argv[1]
-&& !argv[2]
&& strcmp(argv[1], "--help") == 0
&& !is_prefixed_with(applet, "busybox")
) {
@@ -940,8 +939,8 @@ void FAST_FUNC show_usage_if_dash_dash_help(int applet_no,
char **argv)
&& applet_no != APPLET_NO_echo
# endif
) {
- if (argv[1] && !argv[2] && strcmp(argv[1], "--help") == 0) {
- /* Make "foo --help" exit with 0: */
+ if (argv[1] && strcmp(argv[1], "--help") == 0) {
+ /* Make "foo --help [...]" exit with 0: */
xfunc_error_retval = 0;
bb_show_usage();
}
--
2.35.1
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
On Fri, 29 Apr 2022 18:16:41 +0200 Denys Vlasenko wrote: > On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld wrote: > > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote: > > > Can we replace all [s]size_t's with ints/unsigneds? We do not expect > > > random pools anywhere near 4 gigabytes... > > > > Probably that's fine. Is the advantage to tossing out consistent types > > worth it though? Does this actually save space? Since [s]size_t is > > usually the word size, won't codegen not really change much? > > For example, on x86-64, 32-bit insns are often shorter. > ___ > busybox mailing list > busybox@busybox.net > http://lists.busybox.net/mailman/listinfo/busybox Hi, shouldn't that be "was not" exceeded? -* genuine entropy: make sure disk is not full, quota was't esceeded, etc: +* genuine entropy: make sure disk is not full, quota was't exceeded, etc: Ciao, Tito ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v4 2/2] less: replace most uses of NORMAL escape with UNHIGHLIGHT
Emmitting NORMAL resets the entire SGR state of the terminal, which
includes colors. We don't want that now, since those sequences can
come between a colored line, and this would cut the coloring short.
UNHIGHLIGHT is a dedicated code to just flip the HIGHLIGHT (invert)
bit to off, and is a better complement anywhere after HIGHLIGHT.
NORMAL is still used wherever appropriate, e.g. at the end of lines
to reset color state.
This cannot handle the case when there is a HIGHLIGHT sequence in
the input itself, and any such highlighting can be cut short by
less-emmitted highlight sequences. Avoiding that probably requires
looking at the codes and keeping state. Testing shows that even
greenwood less doesn't bother to do that.
---
miscutils/less.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/miscutils/less.c b/miscutils/less.c
index 1e2c4c5cf..2f618ba34 100644
--- a/miscutils/less.c
+++ b/miscutils/less.c
@@ -155,6 +155,7 @@
#define ESC "\033"
/* The escape codes for highlighted and normal text */
#define HIGHLIGHT ESC"[7m"
+#define UNHIGHLIGHT ESC"[27m"
#define NORMAL ESC"[m"
/* The escape code to home and clear to the end of screen */
#define CLEAR ESC"[H"ESC"[J"
@@ -312,13 +313,13 @@ static void clear_line(void)
static void print_hilite(const char *str)
{
- printf(HIGHLIGHT"%s"NORMAL, str);
+ printf(HIGHLIGHT"%s"UNHIGHLIGHT, str);
}
static void print_statusline(const char *str)
{
clear_line();
- printf(HIGHLIGHT"%.*s"NORMAL, width - 1, str);
+ printf(HIGHLIGHT"%.*s"UNHIGHLIGHT, width - 1, str);
}
/* Exit the program gracefully */
@@ -710,7 +711,7 @@ static void m_status_print(void)
percent = (100 * last + num_lines/2) / num_lines;
printf(" %i%%", percent <= 100 ? percent : 100);
}
- printf(NORMAL);
+ printf(UNHIGHLIGHT);
}
#endif
@@ -740,7 +741,7 @@ static void status_print(void)
if (!cur_fline)
p = filename;
if (num_files > 1) {
- printf(HIGHLIGHT"%s (file %i of %i)"NORMAL,
+ printf(HIGHLIGHT"%s (file %i of %i)"UNHIGHLIGHT,
p, current_file, num_files);
return;
}
@@ -807,7 +808,7 @@ static void print_found(const char *line)
/* buf[] holds quarantined version of str */
/* Each part of the line that matches has the HIGHLIGHT
-* and NORMAL escape sequences placed around it.
+* and UNHIGHLIGHT escape sequences placed around it.
* NB: we regex against line, but insert text
* from quarantined copy (buf[]) */
str = buf;
@@ -816,7 +817,7 @@ static void print_found(const char *line)
goto start;
while (match_status == 0) {
- char *new = xasprintf("%s%.*s"HIGHLIGHT"%.*s"NORMAL,
+ char *new = xasprintf("%s%.*s"HIGHLIGHT"%.*s"UNHIGHLIGHT,
growline ? growline : "",
(int)match_structs.rm_so, str,
(int)(match_structs.rm_eo -
match_structs.rm_so),
@@ -1551,7 +1552,7 @@ static void show_flag_status(void)
}
clear_line();
- printf(HIGHLIGHT"The status of the flag is: %u"NORMAL, flag_val != 0);
+ printf(HIGHLIGHT"The status of the flag is: %u"UNHIGHLIGHT, flag_val !=
0);
}
#endif
--
2.36.0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v4 1/2] less: fully implement -R and print color escapes
The earlier implementation used to just strip color escapes. This makes them
output 'raw', coloring the screen. Like less, it assumes that the codes don't
move the cursor. Emits NORMAL at newlines, like less, to deal with malformed
input.
count_colctrl() counts the number of chars in the next ANSI-SGR [1] escape,
essentially matching for "\033\[[0-9;]*m".
Doesn't work in regex find mode, but neither does the normal escape
highlighting.
[1]
https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_(Select_Graphic_Rendition)_parameters
---
miscutils/less.c | 54 +++-
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/miscutils/less.c b/miscutils/less.c
index 8a0525cb7..1e2c4c5cf 100644
--- a/miscutils/less.c
+++ b/miscutils/less.c
@@ -139,7 +139,7 @@
//usage: "\n -S Truncate long lines"
//usage: )
//usage: IF_FEATURE_LESS_RAW(
-//usage: "\n -R Remove color escape codes in input"
+//usage: "\n -R Output 'raw' color escape codes"
//usage: )
//usage: "\n -~ Suppress ~s displayed past EOF"
@@ -229,9 +229,6 @@ struct globals {
regex_t pattern;
smallint pattern_valid;
#endif
-#if ENABLE_FEATURE_LESS_RAW
- smallint in_escape;
-#endif
#if ENABLE_FEATURE_LESS_ASK_TERMINAL
smallint winsize_err;
#endif
@@ -541,26 +538,6 @@ static void read_lines(void)
*--p = '\0';
continue;
}
-#if ENABLE_FEATURE_LESS_RAW
- if (option_mask32 & FLAG_R) {
- if (c == '\033')
- goto discard;
- if (G.in_escape) {
- if (isdigit(c)
-|| c == '['
-|| c == ';'
-|| c == 'm'
- ) {
- discard:
- G.in_escape = (c != 'm');
- readpos++;
- continue;
- }
- /* Hmm, unexpected end of "ESC [ N ; N
m" sequence */
- G.in_escape = 0;
- }
- }
-#endif
{
size_t new_last_line_pos = last_line_pos + 1;
if (c == '\t') {
@@ -864,13 +841,34 @@ static void print_found(const char *line)
void print_found(const char *line);
#endif
+static size_t count_colctrl(const char *str)
+{
+size_t n;
+if (str[1] == '['
+ && (n = strspn(str+2, "0123456789;")) >= 0 // always true
+ && str[n+2] == 'm')
+return n+3;
+return 0;
+}
+
static void print_ascii(const char *str)
{
char buf[width+1];
char *p;
size_t n;
+#if ENABLE_FEATURE_LESS_RAW
+ size_t esc = 0;
+#endif
while (*str) {
+#if ENABLE_FEATURE_LESS_RAW
+ if (esc) {
+ printf("%.*s", (int) esc, str);
+ str += esc;
+ esc = 0;
+ continue;
+ }
+#endif
n = strcspn(str, controls);
if (n) {
if (!str[n]) break;
@@ -886,6 +884,13 @@ static void print_ascii(const char *str)
/* VT100's CSI, aka Meta-ESC. Who's inventor? */
/* I want to know who committed this sin */
*p++ = '{';
+#if ENABLE_FEATURE_LESS_RAW
+ else if ((option_mask32 & FLAG_R)
+&& *str == '\033'
+&& (esc = count_colctrl(str))) {
+ break; // flush collected control chars
+ }
+#endif
else
*p++ = ctrlconv[(unsigned char)*str];
str++;
@@ -894,6 +899,7 @@ static void print_ascii(const char *str)
print_hilite(buf);
}
puts(str);
+ printf(NORMAL);
}
/* Print the buffer */
--
2.36.0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v4 0/2] less: fully implement -R
Reposting without major changes because it hasn't recieved any responses in 2 weeks. The switch was half-implemented earlier and it only used to trim escape sequences. This patch series implements the feature fully i.e. makes it emit color (SGR) sequences raw, and fixes other behaviour around it. Changes from v1: - count_colctrl does better validation for SGR sequences Changes from v2: - fixed corrupted patches (accidentally edited before sending) - fix code style Changes from v3: - fixed regex in commit message ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
