rcolebaugh / rpms / openssh

Forked from rpms/openssh 2 years ago
Clone

Blame openssh-7.1p2-audit-race-condition.patch

Petr Šabata 81d24c
diff -up openssh-7.4p1/monitor_wrap.c.audit-race openssh-7.4p1/monitor_wrap.c
Petr Šabata 81d24c
--- openssh-7.4p1/monitor_wrap.c.audit-race	2016-12-23 16:35:52.694685771 +0100
Petr Šabata 81d24c
+++ openssh-7.4p1/monitor_wrap.c	2016-12-23 16:35:52.697685772 +0100
Petr Šabata 81d24c
@@ -1107,4 +1107,50 @@ mm_audit_destroy_sensitive_data(const ch
Petr Šabata 81d24c
 	mm_request_send(pmonitor->m_recvfd, MONITOR_REQ_AUDIT_SERVER_KEY_FREE, m);
Petr Šabata 81d24c
 	sshbuf_free(m);
Petr Šabata 81d24c
 }
Petr Šabata 81d24c
+
Petr Šabata 81d24c
+int mm_forward_audit_messages(int fdin)
Petr Šabata 81d24c
+{
Petr Šabata 81d24c
+	u_char buf[4];
Petr Šabata 81d24c
+	u_int blen, msg_len;
Petr Šabata 81d24c
+	struct sshbuf *m;
Petr Šabata 81d24c
+	int r, ret = 0;
Petr Šabata 81d24c
+
DistroBaker d029bb
+	debug3_f("entering");
Petr Šabata 81d24c
+	if ((m = sshbuf_new()) == NULL)
DistroBaker d029bb
+ 		fatal_f("sshbuf_new failed");
Petr Šabata 81d24c
+	do {
Petr Šabata 81d24c
+		blen = atomicio(read, fdin, buf, sizeof(buf));
Petr Šabata 81d24c
+		if (blen == 0) /* closed pipe */
Petr Šabata 81d24c
+			break;
Petr Šabata 81d24c
+		if (blen != sizeof(buf)) {
DistroBaker d029bb
+			error_f("Failed to read the buffer from child");
Petr Šabata 81d24c
+			ret = -1;
Petr Šabata 81d24c
+			break;
Petr Šabata 81d24c
+		}
Petr Šabata 81d24c
+
Petr Šabata 81d24c
+		msg_len = get_u32(buf);
Petr Šabata 81d24c
+		if (msg_len > 256 * 1024)
DistroBaker d029bb
+			fatal_f("read: bad msg_len %d", msg_len);
Petr Šabata 81d24c
+		sshbuf_reset(m);
Petr Šabata 81d24c
+		if ((r = sshbuf_reserve(m, msg_len, NULL)) != 0)
DistroBaker d029bb
+			fatal_fr(r, "buffer error");
Petr Šabata 81d24c
+		if (atomicio(read, fdin, sshbuf_mutable_ptr(m), msg_len) != msg_len) {
DistroBaker d029bb
+			error_f("Failed to read the the buffer content from the child");
Petr Šabata 81d24c
+			ret = -1;
Petr Šabata 81d24c
+			break;
Petr Šabata 81d24c
+		}
Petr Šabata 81d24c
+		if (atomicio(vwrite, pmonitor->m_recvfd, buf, blen) != blen || 
Petr Šabata 81d24c
+		    atomicio(vwrite, pmonitor->m_recvfd, sshbuf_mutable_ptr(m), msg_len) != msg_len) {
DistroBaker d029bb
+			error_f("Failed to write the message to the monitor");
Petr Šabata 81d24c
+			ret = -1;
Petr Šabata 81d24c
+			break;
Petr Šabata 81d24c
+		}
Petr Šabata 81d24c
+	} while (1);
Petr Šabata 81d24c
+	sshbuf_free(m);
Petr Šabata 81d24c
+	return ret;
Petr Šabata 81d24c
+}
Petr Šabata 81d24c
+void mm_set_monitor_pipe(int fd)
Petr Šabata 81d24c
+{
Petr Šabata 81d24c
+	pmonitor->m_recvfd = fd;
Petr Šabata 81d24c
+}
Petr Šabata 81d24c
 #endif /* SSH_AUDIT_EVENTS */
Petr Šabata 81d24c
diff -up openssh-7.4p1/monitor_wrap.h.audit-race openssh-7.4p1/monitor_wrap.h
Petr Šabata 81d24c
--- openssh-7.4p1/monitor_wrap.h.audit-race	2016-12-23 16:35:52.694685771 +0100
Petr Šabata 81d24c
+++ openssh-7.4p1/monitor_wrap.h	2016-12-23 16:35:52.698685772 +0100
Petr Šabata 81d24c
@@ -83,6 +83,8 @@ void mm_audit_unsupported_body(int);
Petr Šabata 81d24c
 void mm_audit_kex_body(struct ssh *, int, char *, char *, char *, char *, pid_t, uid_t);
Petr Šabata 81d24c
 void mm_audit_session_key_free_body(struct ssh *, int, pid_t, uid_t);
Petr Šabata 81d24c
 void mm_audit_destroy_sensitive_data(struct ssh *, const char *, pid_t, uid_t);
Petr Šabata 81d24c
+int mm_forward_audit_messages(int);
Petr Šabata 81d24c
+void mm_set_monitor_pipe(int);
Petr Šabata 81d24c
 #endif
Petr Šabata 81d24c
 
Petr Šabata 81d24c
 struct Session;
Petr Šabata 81d24c
diff -up openssh-7.4p1/session.c.audit-race openssh-7.4p1/session.c
Petr Šabata 81d24c
--- openssh-7.4p1/session.c.audit-race	2016-12-23 16:35:52.695685771 +0100
Petr Šabata 81d24c
+++ openssh-7.4p1/session.c	2016-12-23 16:37:26.339730596 +0100
Petr Šabata 81d24c
@@ -162,6 +162,10 @@ static Session *sessions = NULL;
Petr Šabata 81d24c
 login_cap_t *lc;
Petr Šabata 81d24c
 #endif
Petr Šabata 81d24c
 
Petr Šabata 81d24c
+#ifdef SSH_AUDIT_EVENTS
Petr Šabata 81d24c
+int paudit[2];
Petr Šabata 81d24c
+#endif
Petr Šabata 81d24c
+
Petr Šabata 81d24c
 static int is_child = 0;
Petr Šabata 81d24c
 static int in_chroot = 0;
Petr Šabata 81d24c
 static int have_dev_log = 1;
Petr Šabata 81d24c
@@ -289,6 +293,8 @@ xauth_valid_string(const char *s)
Petr Šabata 81d24c
 	return 1;
Petr Šabata 81d24c
 }
Petr Šabata 81d24c
 
Petr Šabata 81d24c
+void child_destory_sensitive_data(struct ssh *ssh);
Petr Šabata 81d24c
+
Petr Šabata 81d24c
 #define USE_PIPES 1
Petr Šabata 81d24c
 /*
Petr Šabata 81d24c
  * This is called to fork and execute a command when we have no tty.  This
Petr Šabata 81d24c
@@ -424,6 +430,8 @@ do_exec_no_pty(Session *s, const char *c
Petr Šabata 81d24c
 		close(err[0]);
Petr Šabata 81d24c
 #endif
Petr Šabata 81d24c
 
Petr Šabata 81d24c
+		child_destory_sensitive_data(ssh);
Petr Šabata 81d24c
+
Petr Šabata 81d24c
 		/* Do processing for the child (exec command etc). */
Petr Šabata 81d24c
 		do_child(ssh, s, command);
Petr Šabata 81d24c
 		/* NOTREACHED */
Petr Šabata 81d24c
@@ -547,6 +555,9 @@ do_exec_pty(Session *s, const char *comm
Petr Šabata 81d24c
 		/* Close the extra descriptor for the pseudo tty. */
Petr Šabata 81d24c
 		close(ttyfd);
Petr Šabata 81d24c
 
Petr Šabata 81d24c
+		/* Do this early, so we will not block large MOTDs */
Petr Šabata 81d24c
+		child_destory_sensitive_data(ssh);
Petr Šabata 81d24c
+
Petr Šabata 81d24c
 		/* record login, etc. similar to login(1) */
Petr Šabata 81d24c
 #ifndef HAVE_OSF_SIA
Petr Šabata 81d24c
 		do_login(ssh, s, command);
Petr Šabata 81d24c
@@ -717,6 +728,8 @@ do_exec(Session *s, const char *command)
Petr Šabata 81d24c
 	}
Petr Šabata 81d24c
 	if (s->command != NULL && s->ptyfd == -1)
Petr Šabata 81d24c
 		s->command_handle = PRIVSEP(audit_run_command(ssh, s->command));
Petr Šabata 81d24c
+	if (pipe(paudit) < 0)
Petr Šabata 81d24c
+		fatal("pipe: %s", strerror(errno));
Petr Šabata 81d24c
 #endif
Petr Šabata 81d24c
 	if (s->ttyfd != -1)
Petr Šabata 81d24c
 		ret = do_exec_pty(ssh, s, command);
Petr Šabata 81d24c
@@ -732,6 +745,20 @@ do_exec(Session *s, const char *command)
Petr Šabata 81d24c
 	 */
Petr Šabata 81d24c
 	sshbuf_reset(loginmsg);
Petr Šabata 81d24c
 
Petr Šabata 81d24c
+#ifdef SSH_AUDIT_EVENTS
Petr Šabata 81d24c
+	close(paudit[1]);
Petr Šabata 81d24c
+	if (use_privsep && ret == 0) {
Petr Šabata 81d24c
+		/*
Petr Šabata 81d24c
+		 * Read the audit messages from forked child and send them
Petr Šabata 81d24c
+		 * back to monitor. We don't want to communicate directly,
Petr Šabata 81d24c
+		 * because the messages might get mixed up.
Petr Šabata 81d24c
+		 * Continue after the pipe gets closed (all messages sent).
Petr Šabata 81d24c
+		 */
Petr Šabata 81d24c
+		ret = mm_forward_audit_messages(paudit[0]);
Petr Šabata 81d24c
+	}
Petr Šabata 81d24c
+	close(paudit[0]);
Petr Šabata 81d24c
+#endif /* SSH_AUDIT_EVENTS */
Petr Šabata 81d24c
+
Petr Šabata 81d24c
 	return ret;
Petr Šabata 81d24c
 }
Petr Šabata 81d24c
 
Petr Šabata 81d24c
@@ -1538,6 +1565,34 @@ child_close_fds(void)
Petr Šabata 81d24c
 	log_redirect_stderr_to(NULL);
Petr Šabata 81d24c
 }
Petr Šabata 81d24c
 
Petr Šabata 81d24c
+void
Petr Šabata 81d24c
+child_destory_sensitive_data(struct ssh *ssh)
Petr Šabata 81d24c
+{
Petr Šabata 81d24c
+#ifdef SSH_AUDIT_EVENTS
Petr Šabata 81d24c
+	int pparent = paudit[1];
Petr Šabata 81d24c
+	close(paudit[0]);
Petr Šabata 81d24c
+	/* Hack the monitor pipe to avoid race condition with parent */
Petr Šabata 81d24c
+	if (use_privsep)
Petr Šabata 81d24c
+		mm_set_monitor_pipe(pparent);
Petr Šabata 81d24c
+#endif
Petr Šabata 81d24c
+
Petr Šabata 81d24c
+	/* remove hostkey from the child's memory */
Petr Šabata 81d24c
+	destroy_sensitive_data(ssh, use_privsep);
Petr Šabata 81d24c
+	/*
Petr Šabata 81d24c
+	 * We can audit this, because we hacked the pipe to direct the
Petr Šabata 81d24c
+	 * messages over postauth child. But this message requires answer
Petr Šabata 81d24c
+	 * which we can't do using one-way pipe.
Petr Šabata 81d24c
+	 */
Petr Šabata 81d24c
+	packet_destroy_all(ssh, 0, 1);
Petr Šabata 81d24c
+	/* XXX this will clean the rest but should not audit anymore */
Petr Šabata 81d24c
+	/* packet_clear_keys(ssh); */
Petr Šabata 81d24c
+
Petr Šabata 81d24c
+#ifdef SSH_AUDIT_EVENTS
Petr Šabata 81d24c
+	/* Notify parent that we are done */
Petr Šabata 81d24c
+	close(pparent);
Petr Šabata 81d24c
+#endif
Petr Šabata 81d24c
+}
Petr Šabata 81d24c
+
Petr Šabata 81d24c
 /*
Petr Šabata 81d24c
  * Performs common processing for the child, such as setting up the
Petr Šabata 81d24c
  * environment, closing extra file descriptors, setting the user and group
Petr Šabata 81d24c
@@ -1554,13 +1608,6 @@ do_child(Session *s, const char *command
Petr Šabata 81d24c
 
Petr Šabata 81d24c
 	sshpkt_fmt_connection_id(ssh, remote_id, sizeof(remote_id));
Petr Šabata 81d24c
 
Petr Šabata 81d24c
-	/* remove hostkey from the child's memory */
Petr Šabata 81d24c
-	destroy_sensitive_data(ssh, 1);
Petr Šabata 81d24c
-	ssh_packet_clear_keys(ssh);
Petr Šabata 81d24c
-	/* Don't audit this - both us and the parent would be talking to the
Petr Šabata 81d24c
-	   monitor over a single socket, with no synchronization. */
Petr Šabata 81d24c
-	packet_destroy_all(ssh, 0, 1);
Petr Šabata 81d24c
-
Petr Šabata 81d24c
 	/* Force a password change */
Petr Šabata 81d24c
 	if (s->authctxt->force_pwchange) {
Petr Šabata 81d24c
 		do_setusercontext(pw);