sudoedit doesn't work if $SUDO_EDITOR has arguments

[aidanharris]

Describe the bug
Sometimes I like to open files in KDE's Kate using sudoedit so I have an alias like the following:

$ which sukate
sukate: aliased to env SUDO_EDITOR='kate -b -n' sudoedit 

Expected behavior
This works in the original sudo but not with sudo-rs. I suspect that it doesn't split the whitespace when searching if it's a valid editor, i.e it's probably checking for /usr/bin/kate\ \-b\ \-n which of course doesn't exist.

If this is intentional, then fair enough, a wrapper script can be used instead but if not then sudo-rs should probably do whatever the original sudo does. I haven't checked but it's probably something equivalent to the following pseudocode:

editor="$1"
shift
# check if $editor is safe/trusted
# if it is
exec "$editor" "$@"

[squell]

Thanks for the report; quick question is also, how is this documented in ogsudo?

(Also with respect to safe editor settings, etc.)

[aidanharris]

I couldn't find any mention of the argument splitting in any of the documentation but the source code mentions this:

    /*
     * Split editor into an argument vector, including files to edit.
     * The EDITOR and VISUAL environment variables may contain command
     * line args so look for those and alloc space for them too.
     */
    cp = wordsplit(ed, edend, &ep);

https://github.com/sudo-project/sudo/blob/87810321402faeb446d3304577cc80ed338ec8dc/plugins/sudoers/editor.c#L132-L137

[squell]

I think that might explain the behaviour; typically we implement user-visible features based on available documentation (only consulting source if something needs clarification).

If using visudo with the env_editor option enabled (by default it is) and kate in the allowed editor list, are the arguments also preserved?

[aidanharris]

If using visudo with the env_editor option enabled (by default it is) and kate in the allowed editor list, are the arguments also preserved?

A simplified test is to do:

sudo env SUDO_EDITOR='cat -n' visudo

With the original sudo that will dump the sudoers file (passing it through cat and display line numbers because of the -n argument).

In sudo-rs that fails to parse the editor and falls back to vim.

[squell]

Thanks, feels like should be a simple fix; I propose we add a simple regression test in that case for sudoedit and visudo that editor-argument are preserved.

Would you also feel up to trying a stab at a fix? This might be "good first issue" territory.

[aidanharris]

Would you also feel up to trying a stab at a fix? This might be "good first issue" territory.

I could try. I'm not the best at Rust though. I suppose I would start by first writing the failing test?

[squell]

Alright. That would already save us a bit of work; you can probably copy one of the tests in test-framework. If you open a Draft PR with that failing test I could then commit the actual fix over that (and maybe add some minor fixes to the test case).

Only if you can spare the time and like doing it, of course! (the only reward is that you'll end up in the contributors list).

[aidanharris]

One thing to be mindful of with arguments is to make sure -- is skipped (ogsudo bails out with an exit code of 1) if found in any of the editor variables.

env SUDO_EDITOR='echo --' visudo
visudo: ignoring editor: echo --
visudo: editor arguments may not contain "--"

    * plugins/sudoers/editor.c, plugins/sudoers/sudoers.c,
    plugins/sudoers/visudo.c:
    sudoedit: do not permit editor arguments to include "--"
    (CVE-2023-22809) We use "--" to separate the editor and arguments
    from the files to edit. If the editor arguments include "--", sudo
    can be tricked into allowing the user to edit a file not permitted
    by the security policy. Thanks to Matthieu Barjole and Victor
    Cutillas of Synacktiv (https://synacktiv.com) for finding this bug.
    [0274a4f3b]

[squell]

We can in principle allow -- in arguments. Ogsudo doesn't allow that as a fix for https://www.sudo.ws/security/advisories/sudoedit_any/ which is essentially caused by the sudo frontend and sudoers plugin having a particular internal "stringly typed" interface. Our interface is different (we will have copied the correct files before even looking at what the editor is; also we don't "copy them back").