File openssh-7.6p1-audit_race_condition.patch of Package openssh

Overview Repositories Revisions Requests Users Attributes Meta

File openssh-7.6p1-audit_race_condition.patch of Package openssh (Revision 2c0911c871c5443d9e6bbd061cb12532)

Currently displaying revision 2c0911c871c5443d9e6bbd061cb12532 , Show latest

diff --git a/monitor_wrap.c b/monitor_wrap.c
index 6d900a8..bfc8e8e 100644
--- a/monitor_wrap.c
+++ b/monitor_wrap.c
@@ -1187,4 +1187,51 @@ mm_audit_destroy_sensitive_data(struct ssh *ssh, const char *fp, pid_t pid, uid_
 	mm_request_send(pmonitor->m_recvfd, MONITOR_REQ_AUDIT_SERVER_KEY_FREE, m);
 	sshbuf_free(m);
 }
+
+int mm_forward_audit_messages(int fdin)
+{
+	u_char buf[4];
+	u_int blen, msg_len;
+	struct sshbuf *m;
+	int ret = 0;
+
+	debug3("%s: entering", __func__);
+	m = sshbuf_new();
+	do {
+		int r;
+
+		blen = atomicio(read, fdin, buf, sizeof(buf));
+		if (blen == 0) /* closed pipe */
+			break;
+		if (blen != sizeof(buf)) {
+			error("%s: Failed to read the buffer from child", __func__);
+			ret = -1;
+			break;
+		}
+
+		msg_len = get_u32(buf);
+		if (msg_len > 256 * 1024)
+			fatal("%s: read: bad msg_len %d", __func__, msg_len);
+		sshbuf_reset(m);
+		if ((r = sshbuf_reserve(m, msg_len, NULL)) != 0)
+			fatal("%s: buffer error: %s", __func__, ssh_err(r));
+		if (atomicio(read, fdin, sshbuf_mutable_ptr(m), msg_len) != msg_len) {
+			error("%s: Failed to read the the buffer conent from the child", __func__);
+			ret = -1;
+			break;
+		}
+		if (atomicio(vwrite, pmonitor->m_recvfd, buf, blen) != blen || 
+		    atomicio(vwrite, pmonitor->m_recvfd, sshbuf_mutable_ptr(m), msg_len) != msg_len) {
+			error("%s: Failed to write the messag to the monitor", __func__);
+			ret = -1;
+			break;
+		}
+	} while (1);
+	sshbuf_free(m);
+	return ret;
+}
+void mm_set_monitor_pipe(int fd)
+{
+	pmonitor->m_recvfd = fd;
+}
 #endif /* SSH_AUDIT_EVENTS */
diff --git a/monitor_wrap.h b/monitor_wrap.h
index 1ebc51d..b3882f3 100644
--- a/monitor_wrap.h
+++ b/monitor_wrap.h
@@ -91,6 +91,8 @@ void mm_audit_unsupported_body(struct ssh *, int);
 void mm_audit_kex_body(struct ssh *, int, char *, char *, char *, char *, pid_t, uid_t);
 void mm_audit_session_key_free_body(struct ssh *, int, pid_t, uid_t);
 void mm_audit_destroy_sensitive_data(struct ssh *, const char *, pid_t, uid_t);
+int mm_forward_audit_messages(int);
+void mm_set_monitor_pipe(int);
 #endif
 
 struct Session;
diff --git a/packet.c b/packet.c
index dfd1f85..30e31f9 100644
--- a/packet.c
+++ b/packet.c
@@ -599,7 +599,7 @@ ssh_packet_rdomain_in(struct ssh *ssh)
 /* Closes the connection and clears and frees internal data structures. */
 
 static void
-ssh_packet_close_internal(struct ssh *ssh, int do_close)
+ssh_packet_close_internal(struct ssh *ssh, int do_close, int do_audit)
 {
 	struct session_state *state = ssh->state;
 	u_int mode;
@@ -651,7 +651,7 @@ ssh_packet_close_internal(struct ssh *ssh, int do_close)
 #endif	/* WITH_ZLIB */
 	cipher_free(state->send_context);
 	cipher_free(state->receive_context);
-	if (had_keys && state->server_side) {
+	if (do_audit && had_keys && state->server_side) {
 		/* Assuming this is called only from privsep child */
 		audit_session_key_free(ssh, MODE_MAX);
 	}
@@ -677,13 +677,19 @@ ssh_packet_close_internal(struct ssh *ssh, int do_close)
 void
 ssh_packet_close(struct ssh *ssh)
 {
-	ssh_packet_close_internal(ssh, 1);
+	ssh_packet_close_internal(ssh, 1, 1);
 }
 
 void
 ssh_packet_clear_keys(struct ssh *ssh)
 {
-	ssh_packet_close_internal(ssh, 0);
+	ssh_packet_close_internal(ssh, 0, 1);
+}
+
+void
+ssh_packet_clear_keys_noaudit(struct ssh *ssh)
+{
+	ssh_packet_close_internal(ssh, 0, 0);
 }
 
 /* Sets remote side protocol flags. */
diff --git a/packet.h b/packet.h
index 239b391..341e33c 100644
--- a/packet.h
+++ b/packet.h
@@ -102,6 +102,7 @@ int      ssh_packet_get_connection_out(struct ssh *);
 void     ssh_packet_close(struct ssh *);
 void	 ssh_packet_set_input_hook(struct ssh *, ssh_packet_hook_fn *, void *);
 void	 ssh_packet_clear_keys(struct ssh *);
+void     ssh_packet_clear_keys_noaudit(struct ssh *);
 void	 ssh_clear_newkeys(struct ssh *, int);
 
 int	 ssh_packet_is_rekeying(struct ssh *);
diff --git a/session.c b/session.c
index 3162089..a130703 100644
--- a/session.c
+++ b/session.c
@@ -158,6 +158,10 @@ static Session *sessions = NULL;
 login_cap_t *lc;
 #endif
 
+#ifdef SSH_AUDIT_EVENTS
+int paudit[2];
+#endif
+
 static int is_child = 0;
 static int in_chroot = 0;
 
@@ -732,6 +736,8 @@ do_exec(struct ssh *ssh, Session *s, const char *command)
 	}
 	if (s->command != NULL && s->ptyfd == -1)
 		s->command_handle = PRIVSEP(audit_run_command(ssh, s->command));
+	if (pipe(paudit) < 0)
+		fatal("pipe: %s", strerror(errno));
 #endif
 	if (s->ttyfd != -1)
 		ret = do_exec_pty(ssh, s, command);
@@ -747,6 +753,20 @@ do_exec(struct ssh *ssh, Session *s, const char *command)
 	 */
 	sshbuf_reset(loginmsg);
 
+#ifdef SSH_AUDIT_EVENTS
+	close(paudit[1]);
+	if (use_privsep && ret == 0) {
+		/*
+		 * Read the audit messages from forked child and send them
+		 * back to monitor. We don't want to communicate directly,
+		 * because the messages might get mixed up.
+		 * Continue after the pipe gets closed (all messages sent).
+		 */
+		ret = mm_forward_audit_messages(paudit[0]);
+	}
+	close(paudit[0]);
+#endif /* SSH_AUDIT_EVENTS */
+
 	return ret;
 }
 
@@ -1551,15 +1571,31 @@ do_child(struct ssh *ssh, Session *s, const char *command)
 	int env_size;
 	int r = 0;
 
+#ifdef SSH_AUDIT_EVENTS
+	int pparent = paudit[1];
+	close(paudit[0]);
+	/* Hack the monitor pipe to avoid race condition with parent */
+	if (use_privsep)
+		mm_set_monitor_pipe(pparent);
+#endif
+
 	sshpkt_fmt_connection_id(ssh, remote_id, sizeof(remote_id));
 
 	/* remove hostkey from the child's memory */
-	destroy_sensitive_data(ssh, 1);
-	ssh_packet_clear_keys(ssh);
-	/* Don't audit this - both us and the parent would be talking to the
-	   monitor over a single socket, with no synchronization. */
+	destroy_sensitive_data(ssh, use_privsep);
+	ssh_packet_clear_keys_noaudit(ssh);
+	/*
+	 * We can audit this, because we hacked the pipe to direct the
+	 * messages over postauth child. But this message requires an answer
+	 * which we can't do using a one-way pipe.
+	 */
 	packet_destroy_all(ssh, 0, 1);
 
+#ifdef SSH_AUDIT_EVENTS
+	/* Notify parent that we are done */
+	close(pparent);
+#endif
+
 	/* Force a password change */
 	if (s->authctxt->force_pwchange) {
 		do_setusercontext(pw);

Places

File openssh-7.6p1-audit_race_condition.patch of Package openssh (Revision 2c0911c871c5443d9e6bbd061cb12532)

Places