[PATCH v3 04/27] adduser: replace BB_EXECLP call with BB_EXECVP

[Nadav Tasher]

So, in summary, I need one of the following:
1. Guarentee that anything BB_EXECVPE does is not going to affect argv[],
since some of them are defined as string literals, and those can be in .rodata.
2. Change all of the argv initializations to result in char ** without casts.

I think it would be easier to implement (1), since making a copy of argv[] is
quite easy, but I must agree that implementing (2) is the right way to go.

What do you suggest I do?
Should I use xstrdup to duplicate literals?

Nadav
 
On Wed, Jan 29, 2025 at 12:32:29AM +0200, Nadav Tasher wrote:
> I actually prefer to duplicate argv[] before calling run_noexec_applet_and_exit(),
> because changing the signature of applets implies that future applets would need
> more work done to be ported to busybox, as their main() function would need to
> be adapted.
> 
> This copy will only happen before using run_noexec_applet_and_exit().
> Every other flow will not duplicate the argv[].
> 
> On Tue, Jan 28, 2025 at 06:03:40PM +1000, d+busybox at adaptive-enterprises.com wrote:
> > 
> > 
> > On Tue, 28 Jan 2025, Kang-Che Sung wrote:
> > > > > Perhaps what you really intended is this?
> > > > > 
> > > > > const char *passwd_argv[] = { "passwd", "--", login_name, NULL };
> > > > 
> > > > That would be much more intuitive, readable, safer and so on, but then
> > > > you'll need a cast when pass it to the pre-const functions,
> > > > eg (char **)passwd_argv.
> > > 
> > > From the surrounding code I've read, you are attempting to pass a
> > > ".rodata" string into an "argv" character pointer array.
> > > 
> > > But "argv" is supposed to be mutable by the callee (i.e. the new
> > > program's main function). (I think it's required by the C standard.)
> > > 
> > > Unless there's a guarantee that the callee never modifies the "argv"
> > > string, passing ".rodata" strings with de-const casts would be unsafe.
> > > That's why when I initially saw the patch and code I was nervous. (A
> > > (char *) de-const cast without an in-code explanation of why is a red
> > > flag to me.)
> > 
> > Ah, you're right. I was focused on the fact that the consumer of the
> > char** argv was
> > 
> > 	int execvp(const char *file, char *const argv[]);
> > 
> > where I hoped, as a syscall, it would copy-out the argv[] in a read-only
> > way which makes the cast safe.
> > 
> > But, as you point out, in this patchset run_applet_no_and_exit() will
> > be passing a (casted) rodata argv straight to an applet's main().
> > 
> > To avoid unnecessary heap allocations and copying on tiny targets,
> > (by str*dup'ing argv[] every time) maybe we can add const as another
> > restriction required by NOEXEC. I mean we might find we're lucky that most
> > of the ~167 NOEXEC applets could already change their main signature from
> > 
> > 	(int argc, char **argv)
> > 
> > to
> > 
> > 	(int argc, const char *const *argv)
> > 
> > and for those that can't, ie those that write to argv[], we may
> > find they can be easily fixed to use a local copy instead.