Bug 791

Summary:	Visudo is not safe with certain args
Product:	Sudo	Reporter:	Cameron Tacklind <cameron>
Component:	Visudo	Assignee:	Todd C. Miller <Todd.Miller>
Status:	RESOLVED FIXED
Severity:	normal
Priority:	low
Version:	1.8.10
Hardware:	PC
OS:	Other

Description Cameron Tacklind 2017-07-14 19:49:52 MDT

I'm very used to editing sudoers files manually without the visudo protection. This time, I decided to try it.

In my instance, I needed to edit one of the files included by a "#includedir" directive. So, I tried to give `visudo` an argument. That was not allowed. Looking at the help/man, I saw that the "-x" argument takes a filename. Without reading further, I simply assumed this was the option that I was looking for.

Those that know the `-x` option will know that I just screwed myself. The `-x` argument writes JSON data to the named file. This put JSON data in what is effectively a sudoers file which, of course, broke sudo.

Looking into this more, it looks like visudo only checks the one file for syntax. It does not check included files. A weird related behavior is that if you add an `#include` directive while editing with visudo, if the file does not exists, visudo will create it and open an editor pointed to it. Of note, visudo does not seem to check that new file for syntax. This can lead to a broken sudoers.

I think that users of visudo expect it to be impossible to mess up their sudoers file as long as the command visudo is the only command used. This currently does not seem to be the case.

Note, my distro is on 1.8.10p3 so maybe this has already been addressed. If that is the case, my apologies.

Comment 1 Todd C. Miller 2017-07-16 19:27:17 MDT

I'm sorry that you overwrote your file by using the -x flag.  Newer versions of the visudo manual clearly name the argument to -x "output_file" instead of just "file" which should avoid such problems in the future.

Visudo does check any included files for syntax errors, though it will not edit files in a #includedir unless they contain a syntax error.

If you add a #include to an existing sudoers file, visudo will give you a chance to edit it since a missing #include would be a fatal error.  There does appear to be a bug where the new file is not checked after it is created even in the latest version.

Comment 2 Cameron Tacklind 2017-07-16 22:47:01 MDT

I'm glad that `-x` has already been made more clear in recent versions.

However, it sounds like it is still possible to use this argument to mess up the sudoers file. Since, in my observations, many distros suggest using visudo as a safe way to edit sudoers, it should, in my humble opinion, include extra protections against such mistakes.

I suppose a counter argument to this could be made. One possible logical extreme of the above would be to be safe even if a user ran something like this: `$ visudo > /etc/sudoers`. How could visudo possibly protect from this kind of mistake? I'm sure there are many other mistakes that people could make as well.

This makes me think about if there could be a way for sudo to use an old (or backup) version of the sudoers file in the event of a broken sudoers. This however opens another can of worms for security.

Comment 3 Todd C. Miller 2017-08-07 16:25:39 MDT

I'm not opposed to moving the export code out of visudo and into a standalone program.  The new program should probably absorb the sudoers2ldif script too.  Naming is always hard so any suggestions are welcome.  Names I've been considering are sudocvt, cvtsudo and convertsudoers or sudoersconvert.

Comment 4 Cameron Tacklind 2017-08-07 17:26:35 MDT

Agreed, naming is hard. But I do like the idea of making the export a separate program. I like something with the word `export` and/or a word that is relevant to the target export format. Maybe `sudoers-export` or `export-sudoers`? If there is already another `sudoers2xxx`, maybe following that format is best and keep the programs separate?

Thinking about this more, there is an alternate way that `visudo` could provide a (n extra) safety net. Since we know `visudo` will be launched with a user that has permissions to edit `/etc/sudoers`, once the new (and checked) changes are pushed to the real `/etc/sudoers` file, we could try to run `sudo`. If that fails (because maybe some bug in visudo like the `#include` issue I mentioned) we still have one thing that could be done to prevent bricking a system: launching bash. This would give the user one more (or as many as we like) chance to fix sudoers file before de-escalating privileges.

Comment 5 Todd C. Miller 2018-05-01 07:06:55 MDT

In sudo 1.8.23 the JSON conversion has been moved to the cvtsudoers utility.