ssh-agent does not immediately clean timeouted keys from memory

[Darren Tucker]

On Sat, Feb 24, 2007 at 10:18:38AM +1100, Darren Tucker wrote:
> On Fri, Feb 23, 2007 at 06:10:32PM +0000, openssh at p23q.org wrote:
> > during my seminar of advanced exploitation techniques (SEAT, [1]) i
> > developed some methods to crack into system via DMA (e.g. via firewire).
> > as part of this i developed a program that steals loaded ssh private
> > keys from ssh-agents. i was astonished to find that the keys are not
> > immediately removed from the agent when a timeout occurs, but only the
> > next time the agent is queried via its socket. i have written a
> > __rough__ patch that should fix the problem (a timer checks every 10
> > seconds). please take a look at it and, if you like it, incorporate it.
> 
> Overloading the sigalrm handler seems unnecessarily complex when select(2)
> has a perfectly good timeout parameter :-)

A slightly smaller patch that uses the existing loop in the reaper()
function to compute the next timeout.

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
-------------- next part --------------
Index: ssh-agent.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh/ssh-agent.c,v
retrieving revision 1.169
diff -u -p -r1.169 ssh-agent.c
--- ssh-agent.c	23 Oct 2006 17:01:16 -0000	1.169
+++ ssh-agent.c	24 Feb 2007 00:21:24 -0000
@@ -421,10 +421,10 @@ process_remove_all_identities(SocketEntr
 	buffer_put_char(&e->output, SSH_AGENT_SUCCESS);
 }
 
-static void
+static u_int
 reaper(void)
 {
-	u_int now = time(NULL);
+	u_int deadline = 0, now = time(NULL);
 	Identity *id, *nxt;
 	int version;
 	Idtab *tab;
@@ -433,13 +433,23 @@ reaper(void)
 		tab = idtab_lookup(version);
 		for (id = TAILQ_FIRST(&tab->idlist); id; id = nxt) {
 			nxt = TAILQ_NEXT(id, next);
-			if (id->death != 0 && now >= id->death) {
-				TAILQ_REMOVE(&tab->idlist, id, next);
-				free_identity(id);
-				tab->nentries--;
+			if (id->death != 0) {
+				if (now >= id->death) {
+					debug("expiring key '%s'", id->comment);
+					TAILQ_REMOVE(&tab->idlist, id, next);
+					free_identity(id);
+					tab->nentries--;
+				} else {
+					if (deadline == 0)
+						deadline = id->death;
+					else
+						deadline =
+						    MIN(deadline, id->death);
+				}
 			}
 		}
 	}
+	return deadline;
 }
 
 static void
@@ -698,9 +708,6 @@ process_message(SocketEntry *e)
 	u_int msg_len, type;
 	u_char *cp;
 
-	/* kill dead keys */
-	reaper();
-
 	if (buffer_len(&e->input) < 5)
 		return;		/* Incomplete message. */
 	cp = buffer_ptr(&e->input);
@@ -828,10 +835,11 @@ new_socket(sock_type type, int fd)
 }
 
 static int
-prepare_select(fd_set **fdrp, fd_set **fdwp, int *fdl, u_int *nallocp)
+prepare_select(fd_set **fdrp, fd_set **fdwp, int *fdl, u_int *nallocp,
+    struct timeval **tvpp)
 {
-	u_int i, sz;
-	int n = 0;
+	u_int i, sz, deadline;
+	int n = 0, timeout;
 
 	for (i = 0; i < sockets_alloc; i++) {
 		switch (sockets[i].type) {
@@ -875,6 +883,14 @@ prepare_select(fd_set **fdrp, fd_set **f
 			break;
 		}
 	}
+
+	if ((deadline = reaper()) == 0) {
+		*tvpp = NULL;
+	} else {
+		timeout = deadline - time(NULL);
+		(*tvpp)->tv_sec = timeout < 0 ? 0 : timeout;
+		(*tvpp)->tv_usec = 0;
+	}
 	return (1);
 }
 
@@ -1016,7 +1032,7 @@ int
 main(int ac, char **av)
 {
 	int c_flag = 0, d_flag = 0, k_flag = 0, s_flag = 0;
-	int sock, fd, ch;
+	int sock, fd, ch, result;
 	u_int nalloc;
 	char *shell, *format, *pidstr, *agentsocket = NULL;
 	fd_set *readsetp = NULL, *writesetp = NULL;
@@ -1029,6 +1045,7 @@ main(int ac, char **av)
 	extern char *optarg;
 	pid_t pid;
 	char pidstrbuf[1 + 3 * sizeof pid];
+	struct timeval tv, *tvp;
 
 	/* Ensure that fds 0, 1 and 2 are open or directed to /dev/null */
 	sanitise_stdfd();
@@ -1242,13 +1259,16 @@ skip:
 	nalloc = 0;
 
 	while (1) {
-		prepare_select(&readsetp, &writesetp, &max_fd, &nalloc);
-		if (select(max_fd + 1, readsetp, writesetp, NULL, NULL) < 0) {
+		tvp = &tv;
+		prepare_select(&readsetp, &writesetp, &max_fd, &nalloc, &tvp);
+		result = select(max_fd + 1, readsetp, writesetp, NULL, tvp);
+		reaper();	/* remove expired keys */
+		if (result < 0) {
 			if (errno == EINTR)
 				continue;
 			fatal("select: %s", strerror(errno));
-		}
-		after_select(readsetp, writesetp);
+		} else if (result > 0)
+			after_select(readsetp, writesetp);
 	}
 	/* NOTREACHED */
 }