Program to let any user run a command

Question

I have a script which can only be run by one specific user on a system. I need to let all users on the system have access to run that script and see the output. But there must never be more than one instance of the script running simultaneously.

I came up with this program to be run as the specific user who can run the script. This program will be left running listening on a TCP port. Any user who want to run the script then connects to the TCP port and the script will run.

#include <arpa/inet.h>
#include <errno.h>
#include <error.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
int setup_socket()
{
 const int one = 1;
 struct sockaddr_in6 bind_addr = {
 .sin6_addr = IN6ADDR_LOOPBACK_INIT,
 .sin6_family = AF_INET6,
 .sin6_port = htons(65454),
 .sin6_flowinfo = 0,
 .sin6_scope_id = 0,
 };
 int fd = socket(PF_INET6, SOCK_STREAM, 0);
 setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one));
 if (bind(fd, (struct sockaddr*)&bind_addr, sizeof(bind_addr)))
 error(EXIT_FAILURE, errno, "bind() failed");
 if (listen(fd, 5))
 error(EXIT_FAILURE, errno, "listen() failed");
 return fd;
}
int main(int argc, char ** argv)
{
 int fd;
 if (argc < 2) {
 fprintf(stderr, "Usage: %s <command>\n", argv[0]);
 return EXIT_FAILURE;
 }
 fd = setup_socket();
 while (1) {
 int child_fd = accept(fd, NULL, NULL);
 pid_t child = fork();
 switch (child) {
 case -1:
 error(0, errno, "fork() failed");
 close(child_fd);
 break;
 case 0:
 if ((dup2(child_fd, 1) != 1) ||
 (dup2(child_fd, 2) != 2))
 error(EXIT_FAILURE, errno, "dup2() failed");
 execvp(argv[1], argv + 1);
 error(EXIT_FAILURE, errno, "excvp(%s) failed", argv[1]);
 default:
 close(child_fd);
 waitpid(child, NULL, 0);
 }
 }
}

Do you see any flaws in this program? I am mostly concerned with security problems, but also interested in hearing of any other problems you may see in the program.

• 200_success: Couldn't you set the SUID bit on the program? Or use sudo? Those would be more conventional solutions to this kind of problem.
• kasperd: @200_success I could do that. In that case I'd have to find another way to guard against concurrent invocations. I guess combining a suid program with a lock file could work. My initial feeling was that it was easier to avoid security flaws in my current approach (since the listening process doesn't take any input from the socket), and simultaneously that approach took care of avoiding concurrent invocations. But the SUID approach might be simpler after all.

Answer 1

• This looks a bit unclear:

  const int one = 1;
  

  It's primarily used as the option_value argument of setsockopt(). Consider renaming this to something more clear so that it doesn't leave readers confused.

• In addition to your other error-checking, you can also check the return value of socket() and print its error in case it fails.

• A little nit-picky, but you can just initialize fd after the command line check instead of declaring it first and then assigning to it:

  int fd = setup_socket();
  

• kasperd: The lack of checking the return value of socket was clearly an oversight, so that I am going to fix. I don't see how the name one could be made much more clear as it already tells exactly what that constant is. As for moving the declaration of fd down, I believe that would break with older compilers. Maybe I should get rid of that habit, as there might be very little gain from trying to support such old compilers.
• Jamal: @kasperd: It's up to you, which is also why I mentioned it's a nit-pick. Then again, it is common to list variables in C, but I'm not used to doing things that way.

Answer 2

There are some detailed comments below, but the way you have implemented the idea allows any user to send any command to the socket and have it execute. That seems a little dangerous. At a minimum you need to tie it down to running just the script intended (i.e. no argv passing).

Detail:

• setup_socket() should be static and have a void parameter list.

• check the return value from socket, setsockpt and accept

• use perror to print error messages, as in:

  if (bind(fd, (struct sockaddr*)&bind_addr, sizeof(bind_addr))) {
   perror("bind() failed");
   exit(EXIT_FAILURE);
  }
  

  I guess you could call perror from within error and it seems unlikely that error would ever need to be passed anything but EXIT_FAILURE unless it only exits if it sees that error code value, which seems like a bad idea. So maybe you could just do:

  if (bind(fd, (struct sockaddr*)&bind_addr, sizeof(bind_addr))) {
   error_exit("bind() failed");
  }
  

  All the same I think I prefer the 2-line version above.

• Note that many people insist on braces being used even from on-line conditions. This is pig-ugly but it does make sense as it entirely removes a class of errors.

• I'd move one to just before it is used and change its name, maybe to reuseaddr as it is the value associated with the SO_REUSEADDR option.

• if this is to run as a traditional daemon, it should probably detach itself from the shell and use syslog (or some such) for error messages.

Finally, I just noticed that error.h is included using the <> instead of "", which is wrong unless it is a system header.

• kasperd: The line for inclusion of error.h is taken straight from the ERROR(3) man page. The lack of checking the setsockpt call was a deliberate decision because in most cases the program will work without the setsockpt call anyway. Should setsockpt fail the program will either work anyway or fail on bind. Your comment about sending data over the socket would only apply if any code in the script is reading from stdout or stderr, which it shouldn't be. But I could use the shutdown call to provide an extra layer of protection in case such a bug exists.
• kasperd: The only reason the const one exists in the first place is because I cannot use &1 as argument for setsockopt. I named it one because I would use the same constant for any setsockopt call where I need 1 as optval. The constant could easily be needed for multiple optname in the same program. For that reason I don't think it makes sense to name the constant according to which socket option it is currently being used for.
• kasperd: The lack of checking return value of socket and accept were oversights on my part. The calling conventions of error I am not going to change, because error is part of glibc, and I prefer not to modify glibc unless I absolutely have to.
• William Morris: I wasn't aware that error() was in glibc, so strike that comment. My comments, like all reviews on CR, are offered in good faith in the hope of being useful. You can choose whether to accept or ignore them as you please, but some thanks are normal. Instead your responses seem rather defensive - I'll not comment further.
• kasperd: Then you also learned something new today :-) I have only known about the error function for a few weeks myself. I learned about it from another SE post. Before that almost every program I wrote would contain a function that just called perror and exit.