[sudo-workers] dead condition

[Radovan Sroka]

Hi,

I'm resolving some issues and I encountered a dead condition in plugins/sudoers/visudo.c.

Output of static analysis:

Error: DEADCODE (CWE-561): [#def67]
sudo-1.8.14p3/plugins/sudoers/visudo.c:444: cond_at_most: Condition "(nread = read(sp->fd, buf, 4096UL)) > 0L", taking false branch. Now the value of "nread" is at most 0.
sudo-1.8.14p3/plugins/sudoers/visudo.c:449: at_most: At condition "nread > 0L", the value of "nread" must be at most 0.
sudo-1.8.14p3/plugins/sudoers/visudo.c:449: dead_error_condition: The condition "nread > 0L" cannot be true.
sudo-1.8.14p3/plugins/sudoers/visudo.c:449: dead_error_line: Execution cannot reach the expression "buf[nread - 1L] != 10" inside this statement: "if (nread > 0L && buf[nread...".
#  447|   
#  448|   	    /* Add missing newline at EOF if needed. */
#  449|-> 	    if (nread > 0 && buf[nread - 1] != '\n') {
#  450|   		buf[0] = '\n';
#  451|   		if (write(tfd, buf, 1) != 1)


Code:

 if (sp->tpath == NULL) {
	if (asprintf(&sp->tpath, "%s.tmp", sp->path) == -1)
	    sudo_fatalx(U_("%s: %s"), __func__, U_("unable to allocate memory"));
	tfd = open(sp->tpath, O_WRONLY | O_CREAT | O_TRUNC, 0600);
	if (tfd < 0)
	    sudo_fatal("%s", sp->tpath);

	/* Copy sp->path -> sp->tpath and reset the mtime. */
	if (orig_size != 0) {
	    (void) lseek(sp->fd, (off_t)0, SEEK_SET);
	    while ((nread = read(sp->fd, buf, sizeof(buf))) > 0)
		if (write(tfd, buf, nread) != nread)
		    sudo_fatal(U_("write error"));

	    /* Add missing newline at EOF if needed. */
	    if (nread > 0 && buf[nread - 1] != '\n') {
		buf[0] = '\n';
		if (write(tfd, buf, 1) != 1)
		    sudo_fatal(U_("write error"));
	    }
	}
	(void) close(tfd);
}

I think when while ends then nread will be 0 or -1 and next "if" condition never execute.

Here is my patch:

  1 diff -up ./plugins/sudoers/visudo.c.fix ./plugins/sudoers/visudo.c
  2 --- ./plugins/sudoers/visudo.c.fix      2015-08-17 14:56:33.048310054 +0200
  3 +++ ./plugins/sudoers/visudo.c  2015-08-17 15:09:29.471611089 +0200
  4 @@ -440,13 +440,16 @@ edit_sudoers(struct sudoersfile *sp, cha
  5 
  6         /* Copy sp->path -> sp->tpath and reset the mtime. */
  7         if (orig_size != 0) {
  8 +           ssize_t last_valid_nread = 0;
  9             (void) lseek(sp->fd, (off_t)0, SEEK_SET);
 10 -           while ((nread = read(sp->fd, buf, sizeof(buf))) > 0)
 11 +           while ((nread = read(sp->fd, buf, sizeof(buf))) > 0) {
 12                 if (write(tfd, buf, nread) != nread)
 13                     sudo_fatal(U_("write error"));
 14 +               last_valid_nread = nread;
 15 +           }
 16 
 17             /* Add missing newline at EOF if needed. */
 18 -           if (nread > 0 && buf[nread - 1] != '\n') {
 19 +           if (last_valid_nread > 0 && buf[last_valid_nread - 1] != '\n') {
 20                 buf[0] = '\n';
 21                 if (write(tfd, buf, 1) != 1)
 22                     sudo_fatal(U_("write error"));


It's OK? or I missed something ?



Radovan Sroka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sudo-1.8.14p3-deadcode_visudo_c.patch
Type: text/x-patch
Size: 929 bytes
Desc: not available
URL: <http://www.sudo.ws/pipermail/sudo-workers/attachments/20150818/8ee110ea/attachment.bin>