misc/169259: libfetch to use O_CLOEXEC to avoid file descriptor leaks in threaded code

[Jukka Ukkonen]

>Number:         169259
>Category:       misc
>Synopsis:       libfetch to use O_CLOEXEC to avoid file descriptor leaks in threaded code
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Wed Jun 20 07:40:08 UTC 2012
>Closed-Date:
>Last-Modified:
>Originator:     Jukka Ukkonen
>Release:        FreeBSD 9.0-STABLE
>Organization:
-----
>Environment:
FreeBSD sleipnir 9.0-STABLE FreeBSD 9.0-STABLE #0: Tue Jun 19 22:26:46 EEST 2012     root at sleipnir:/usr/obj/usr/src/sys/Sleipnir  amd64
>Description:
Libfetch has been using a call sequence ...

f = fopen (...);
fcntl (fileno(f), F_SETFD, FD_CLOEXEC);

when opening files.
This leaves a short time window which in threaded code could lead
to the newly opened file descriptor leaking to the child program,
if another thread happens to call exec().

An alternative approach avoiding the potential fd leak is

fd = open (..., O_CLOEXEC);
..
f = fdopen (fd, ...);


>How-To-Repeat:
See full description.
>Fix:
Find a patch attached.


Patch attached with submission follows:

--- lib/libfetch/file.c.orig	2012-05-26 19:34:39.000000000 +0300
+++ lib/libfetch/file.c	2012-06-19 21:28:46.000000000 +0300
@@ -44,11 +44,17 @@
 fetchXGetFile(struct url *u, struct url_stat *us, const char *flags)
 {
 	FILE *f;
+	int fd;
 
 	if (us && fetchStatFile(u, us, flags) == -1)
 		return (NULL);
 
-	f = fopen(u->doc, "r");
+	fd = open (u->doc, (O_RDONLY | O_CLOEXEC));
+
+	if (fd == -1)
+		fetch_syserr();
+
+	f = fdopen (fd, "r");
 
 	if (f == NULL)
 		fetch_syserr();
@@ -58,7 +64,6 @@
 		fetch_syserr();
 	}
 
-	fcntl(fileno(f), F_SETFD, FD_CLOEXEC);
 	return (f);
 }
 
@@ -72,11 +77,36 @@
 fetchPutFile(struct url *u, const char *flags)
 {
 	FILE *f;
+	int fd;
+	int fdflags;
+	const char *mode;
+
+	fdflags = (O_CREAT | O_CLOEXEC);
+
+	if (CHECK_FLAG('a')) {
+		/*
+		 * Match the functionality of
+		 * f = fopen(u->doc, "a");
+		 */
+		fdflags |= (O_WRONLY | O_APPEND);
+		mode = "a";
+	}
+	else {
+		/*
+		 * Match the functionality of
+		 * f = fopen(u->doc, "w+");
+		 */
+
+		fdflags = (O_RDWR | O_TRUNC);
+		mode = "w+";
+	}
+
+	fd = open (u->doc, fdflags);
+
+	if (fd == -1)
+		fetch_syserr();
 
-	if (CHECK_FLAG('a'))
-		f = fopen(u->doc, "a");
-	else
-		f = fopen(u->doc, "w+");
+	f = fdopen (fd, mode);
 
 	if (f == NULL)
 		fetch_syserr();
@@ -86,7 +116,6 @@
 		fetch_syserr();
 	}
 
-	fcntl(fileno(f), F_SETFD, FD_CLOEXEC);
 	return (f);
 }
 


>Release-Note:
>Audit-Trail:
>Unformatted: