Patrick Williams | f1e5d69 | 2016-03-30 15:21:19 -0500 | [diff] [blame] | 1 | From ed4ce82dbfa8a3a3c8ea6fa0db113c71e234416c Mon Sep 17 00:00:00 2001 |
| 2 | From: "djm@openbsd.org" <djm@openbsd.org> |
| 3 | Date: Wed, 13 Jan 2016 23:04:47 +0000 |
| 4 | Subject: [PATCH] upstream commit |
| 5 | |
| 6 | eliminate fallback from untrusted X11 forwarding to trusted |
| 7 | forwarding when the X server disables the SECURITY extension; Reported by |
| 8 | Thomas Hoger; ok deraadt@ |
| 9 | |
| 10 | Upstream-ID: f76195bd2064615a63ef9674a0e4096b0713f938 |
| 11 | Upstream-Status: Backport |
| 12 | CVE: CVE-2016-1907 |
| 13 | |
| 14 | [YOCTO #8935] |
| 15 | |
| 16 | Signed-off-by: Armin Kuster <akuster@mvista.com> |
| 17 | |
| 18 | --- |
| 19 | clientloop.c | 114 ++++++++++++++++++++++++++++++++++++----------------------- |
| 20 | clientloop.h | 4 +-- |
| 21 | mux.c | 22 ++++++------ |
| 22 | ssh.c | 23 +++++------- |
| 23 | 4 files changed, 93 insertions(+), 70 deletions(-) |
| 24 | |
| 25 | Index: openssh-7.1p2/clientloop.c |
| 26 | =================================================================== |
| 27 | --- openssh-7.1p2.orig/clientloop.c |
| 28 | +++ openssh-7.1p2/clientloop.c |
| 29 | @@ -1,4 +1,4 @@ |
| 30 | -/* $OpenBSD: clientloop.c,v 1.276 2015/10/20 03:36:35 mmcc Exp $ */ |
| 31 | +/* $OpenBSD: clientloop.c,v 1.279 2016/01/13 23:04:47 djm Exp $ */ |
| 32 | /* |
| 33 | * Author: Tatu Ylonen <ylo@cs.hut.fi> |
| 34 | * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland |
| 35 | @@ -288,6 +288,9 @@ client_x11_display_valid(const char *dis |
| 36 | { |
| 37 | size_t i, dlen; |
| 38 | |
| 39 | + if (display == NULL) |
| 40 | + return 0; |
| 41 | + |
| 42 | dlen = strlen(display); |
| 43 | for (i = 0; i < dlen; i++) { |
| 44 | if (!isalnum((u_char)display[i]) && |
| 45 | @@ -301,34 +304,33 @@ client_x11_display_valid(const char *dis |
| 46 | |
| 47 | #define SSH_X11_PROTO "MIT-MAGIC-COOKIE-1" |
| 48 | #define X11_TIMEOUT_SLACK 60 |
| 49 | -void |
| 50 | +int |
| 51 | client_x11_get_proto(const char *display, const char *xauth_path, |
| 52 | u_int trusted, u_int timeout, char **_proto, char **_data) |
| 53 | { |
| 54 | - char cmd[1024]; |
| 55 | - char line[512]; |
| 56 | - char xdisplay[512]; |
| 57 | + char cmd[1024], line[512], xdisplay[512]; |
| 58 | + char xauthfile[PATH_MAX], xauthdir[PATH_MAX]; |
| 59 | static char proto[512], data[512]; |
| 60 | FILE *f; |
| 61 | - int got_data = 0, generated = 0, do_unlink = 0, i; |
| 62 | - char xauthdir[PATH_MAX] = "", xauthfile[PATH_MAX] = ""; |
| 63 | + int got_data = 0, generated = 0, do_unlink = 0, i, r; |
| 64 | struct stat st; |
| 65 | u_int now, x11_timeout_real; |
| 66 | |
| 67 | *_proto = proto; |
| 68 | *_data = data; |
| 69 | - proto[0] = data[0] = '\0'; |
| 70 | + proto[0] = data[0] = xauthfile[0] = xauthdir[0] = '\0'; |
| 71 | |
| 72 | - if (xauth_path == NULL ||(stat(xauth_path, &st) == -1)) { |
| 73 | - debug("No xauth program."); |
| 74 | - } else if (!client_x11_display_valid(display)) { |
| 75 | - logit("DISPLAY '%s' invalid, falling back to fake xauth data", |
| 76 | + if (!client_x11_display_valid(display)) { |
| 77 | + logit("DISPLAY \"%s\" invalid; disabling X11 forwarding", |
| 78 | display); |
| 79 | - } else { |
| 80 | - if (display == NULL) { |
| 81 | - debug("x11_get_proto: DISPLAY not set"); |
| 82 | - return; |
| 83 | - } |
| 84 | + return -1; |
| 85 | + } |
| 86 | + if (xauth_path != NULL && stat(xauth_path, &st) == -1) { |
| 87 | + debug("No xauth program."); |
| 88 | + xauth_path = NULL; |
| 89 | + } |
| 90 | + |
| 91 | + if (xauth_path != NULL) { |
| 92 | /* |
| 93 | * Handle FamilyLocal case where $DISPLAY does |
| 94 | * not match an authorization entry. For this we |
| 95 | @@ -337,43 +339,60 @@ client_x11_get_proto(const char *display |
| 96 | * is not perfect. |
| 97 | */ |
| 98 | if (strncmp(display, "localhost:", 10) == 0) { |
| 99 | - snprintf(xdisplay, sizeof(xdisplay), "unix:%s", |
| 100 | - display + 10); |
| 101 | + if ((r = snprintf(xdisplay, sizeof(xdisplay), "unix:%s", |
| 102 | + display + 10)) < 0 || |
| 103 | + (size_t)r >= sizeof(xdisplay)) { |
| 104 | + error("%s: display name too long", __func__); |
| 105 | + return -1; |
| 106 | + } |
| 107 | display = xdisplay; |
| 108 | } |
| 109 | if (trusted == 0) { |
| 110 | - mktemp_proto(xauthdir, PATH_MAX); |
| 111 | /* |
| 112 | + * Generate an untrusted X11 auth cookie. |
| 113 | + * |
| 114 | * The authentication cookie should briefly outlive |
| 115 | * ssh's willingness to forward X11 connections to |
| 116 | * avoid nasty fail-open behaviour in the X server. |
| 117 | */ |
| 118 | + mktemp_proto(xauthdir, sizeof(xauthdir)); |
| 119 | + if (mkdtemp(xauthdir) == NULL) { |
| 120 | + error("%s: mkdtemp: %s", |
| 121 | + __func__, strerror(errno)); |
| 122 | + return -1; |
| 123 | + } |
| 124 | + do_unlink = 1; |
| 125 | + if ((r = snprintf(xauthfile, sizeof(xauthfile), |
| 126 | + "%s/xauthfile", xauthdir)) < 0 || |
| 127 | + (size_t)r >= sizeof(xauthfile)) { |
| 128 | + error("%s: xauthfile path too long", __func__); |
| 129 | + unlink(xauthfile); |
| 130 | + rmdir(xauthdir); |
| 131 | + return -1; |
| 132 | + } |
| 133 | + |
| 134 | if (timeout >= UINT_MAX - X11_TIMEOUT_SLACK) |
| 135 | x11_timeout_real = UINT_MAX; |
| 136 | else |
| 137 | x11_timeout_real = timeout + X11_TIMEOUT_SLACK; |
| 138 | - if (mkdtemp(xauthdir) != NULL) { |
| 139 | - do_unlink = 1; |
| 140 | - snprintf(xauthfile, PATH_MAX, "%s/xauthfile", |
| 141 | - xauthdir); |
| 142 | - snprintf(cmd, sizeof(cmd), |
| 143 | - "%s -f %s generate %s " SSH_X11_PROTO |
| 144 | - " untrusted timeout %u 2>" _PATH_DEVNULL, |
| 145 | - xauth_path, xauthfile, display, |
| 146 | - x11_timeout_real); |
| 147 | - debug2("x11_get_proto: %s", cmd); |
| 148 | - if (x11_refuse_time == 0) { |
| 149 | - now = monotime() + 1; |
| 150 | - if (UINT_MAX - timeout < now) |
| 151 | - x11_refuse_time = UINT_MAX; |
| 152 | - else |
| 153 | - x11_refuse_time = now + timeout; |
| 154 | - channel_set_x11_refuse_time( |
| 155 | - x11_refuse_time); |
| 156 | - } |
| 157 | - if (system(cmd) == 0) |
| 158 | - generated = 1; |
| 159 | + if ((r = snprintf(cmd, sizeof(cmd), |
| 160 | + "%s -f %s generate %s " SSH_X11_PROTO |
| 161 | + " untrusted timeout %u 2>" _PATH_DEVNULL, |
| 162 | + xauth_path, xauthfile, display, |
| 163 | + x11_timeout_real)) < 0 || |
| 164 | + (size_t)r >= sizeof(cmd)) |
| 165 | + fatal("%s: cmd too long", __func__); |
| 166 | + debug2("%s: %s", __func__, cmd); |
| 167 | + if (x11_refuse_time == 0) { |
| 168 | + now = monotime() + 1; |
| 169 | + if (UINT_MAX - timeout < now) |
| 170 | + x11_refuse_time = UINT_MAX; |
| 171 | + else |
| 172 | + x11_refuse_time = now + timeout; |
| 173 | + channel_set_x11_refuse_time(x11_refuse_time); |
| 174 | } |
| 175 | + if (system(cmd) == 0) |
| 176 | + generated = 1; |
| 177 | } |
| 178 | |
| 179 | /* |
| 180 | @@ -395,9 +414,7 @@ client_x11_get_proto(const char *display |
| 181 | got_data = 1; |
| 182 | if (f) |
| 183 | pclose(f); |
| 184 | - } else |
| 185 | - error("Warning: untrusted X11 forwarding setup failed: " |
| 186 | - "xauth key data not generated"); |
| 187 | + } |
| 188 | } |
| 189 | |
| 190 | if (do_unlink) { |
| 191 | @@ -405,6 +422,13 @@ client_x11_get_proto(const char *display |
| 192 | rmdir(xauthdir); |
| 193 | } |
| 194 | |
| 195 | + /* Don't fall back to fake X11 data for untrusted forwarding */ |
| 196 | + if (!trusted && !got_data) { |
| 197 | + error("Warning: untrusted X11 forwarding setup failed: " |
| 198 | + "xauth key data not generated"); |
| 199 | + return -1; |
| 200 | + } |
| 201 | + |
| 202 | /* |
| 203 | * If we didn't get authentication data, just make up some |
| 204 | * data. The forwarding code will check the validity of the |
| 205 | @@ -427,6 +451,8 @@ client_x11_get_proto(const char *display |
| 206 | rnd >>= 8; |
| 207 | } |
| 208 | } |
| 209 | + |
| 210 | + return 0; |
| 211 | } |
| 212 | |
| 213 | /* |
| 214 | Index: openssh-7.1p2/clientloop.h |
| 215 | =================================================================== |
| 216 | --- openssh-7.1p2.orig/clientloop.h |
| 217 | +++ openssh-7.1p2/clientloop.h |
| 218 | @@ -1,4 +1,4 @@ |
| 219 | -/* $OpenBSD: clientloop.h,v 1.31 2013/06/02 23:36:29 dtucker Exp $ */ |
| 220 | +/* $OpenBSD: clientloop.h,v 1.32 2016/01/13 23:04:47 djm Exp $ */ |
| 221 | |
| 222 | /* |
| 223 | * Author: Tatu Ylonen <ylo@cs.hut.fi> |
| 224 | @@ -39,7 +39,7 @@ |
| 225 | |
| 226 | /* Client side main loop for the interactive session. */ |
| 227 | int client_loop(int, int, int); |
| 228 | -void client_x11_get_proto(const char *, const char *, u_int, u_int, |
| 229 | +int client_x11_get_proto(const char *, const char *, u_int, u_int, |
| 230 | char **, char **); |
| 231 | void client_global_request_reply_fwd(int, u_int32_t, void *); |
| 232 | void client_session2_setup(int, int, int, const char *, struct termios *, |
| 233 | Index: openssh-7.1p2/mux.c |
| 234 | =================================================================== |
| 235 | --- openssh-7.1p2.orig/mux.c |
| 236 | +++ openssh-7.1p2/mux.c |
| 237 | @@ -1,4 +1,4 @@ |
| 238 | -/* $OpenBSD: mux.c,v 1.54 2015/08/19 23:18:26 djm Exp $ */ |
| 239 | +/* $OpenBSD: mux.c,v 1.58 2016/01/13 23:04:47 djm Exp $ */ |
| 240 | /* |
| 241 | * Copyright (c) 2002-2008 Damien Miller <djm@openbsd.org> |
| 242 | * |
| 243 | @@ -1354,16 +1354,18 @@ mux_session_confirm(int id, int success, |
| 244 | char *proto, *data; |
| 245 | |
| 246 | /* Get reasonable local authentication information. */ |
| 247 | - client_x11_get_proto(display, options.xauth_location, |
| 248 | + if (client_x11_get_proto(display, options.xauth_location, |
| 249 | options.forward_x11_trusted, options.forward_x11_timeout, |
| 250 | - &proto, &data); |
| 251 | - /* Request forwarding with authentication spoofing. */ |
| 252 | - debug("Requesting X11 forwarding with authentication " |
| 253 | - "spoofing."); |
| 254 | - x11_request_forwarding_with_spoofing(id, display, proto, |
| 255 | - data, 1); |
| 256 | - client_expect_confirm(id, "X11 forwarding", CONFIRM_WARN); |
| 257 | - /* XXX exit_on_forward_failure */ |
| 258 | + &proto, &data) == 0) { |
| 259 | + /* Request forwarding with authentication spoofing. */ |
| 260 | + debug("Requesting X11 forwarding with authentication " |
| 261 | + "spoofing."); |
| 262 | + x11_request_forwarding_with_spoofing(id, display, proto, |
| 263 | + data, 1); |
| 264 | + /* XXX exit_on_forward_failure */ |
| 265 | + client_expect_confirm(id, "X11 forwarding", |
| 266 | + CONFIRM_WARN); |
| 267 | + } |
| 268 | } |
| 269 | |
| 270 | if (cctx->want_agent_fwd && options.forward_agent) { |
| 271 | Index: openssh-7.1p2/ssh.c |
| 272 | =================================================================== |
| 273 | --- openssh-7.1p2.orig/ssh.c |
| 274 | +++ openssh-7.1p2/ssh.c |
| 275 | @@ -1,4 +1,4 @@ |
| 276 | -/* $OpenBSD: ssh.c,v 1.420 2015/07/30 00:01:34 djm Exp $ */ |
| 277 | +/* $OpenBSD: ssh.c,v 1.433 2016/01/13 23:04:47 djm Exp $ */ |
| 278 | /* |
| 279 | * Author: Tatu Ylonen <ylo@cs.hut.fi> |
| 280 | * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland |
| 281 | @@ -1604,6 +1604,7 @@ ssh_session(void) |
| 282 | struct winsize ws; |
| 283 | char *cp; |
| 284 | const char *display; |
| 285 | + char *proto = NULL, *data = NULL; |
| 286 | |
| 287 | /* Enable compression if requested. */ |
| 288 | if (options.compression) { |
| 289 | @@ -1674,13 +1675,9 @@ ssh_session(void) |
| 290 | display = getenv("DISPLAY"); |
| 291 | if (display == NULL && options.forward_x11) |
| 292 | debug("X11 forwarding requested but DISPLAY not set"); |
| 293 | - if (options.forward_x11 && display != NULL) { |
| 294 | - char *proto, *data; |
| 295 | - /* Get reasonable local authentication information. */ |
| 296 | - client_x11_get_proto(display, options.xauth_location, |
| 297 | - options.forward_x11_trusted, |
| 298 | - options.forward_x11_timeout, |
| 299 | - &proto, &data); |
| 300 | + if (options.forward_x11 && client_x11_get_proto(display, |
| 301 | + options.xauth_location, options.forward_x11_trusted, |
| 302 | + options.forward_x11_timeout, &proto, &data) == 0) { |
| 303 | /* Request forwarding with authentication spoofing. */ |
| 304 | debug("Requesting X11 forwarding with authentication " |
| 305 | "spoofing."); |
| 306 | @@ -1770,6 +1767,7 @@ ssh_session2_setup(int id, int success, |
| 307 | extern char **environ; |
| 308 | const char *display; |
| 309 | int interactive = tty_flag; |
| 310 | + char *proto = NULL, *data = NULL; |
| 311 | |
| 312 | if (!success) |
| 313 | return; /* No need for error message, channels code sens one */ |
| 314 | @@ -1777,12 +1775,9 @@ ssh_session2_setup(int id, int success, |
| 315 | display = getenv("DISPLAY"); |
| 316 | if (display == NULL && options.forward_x11) |
| 317 | debug("X11 forwarding requested but DISPLAY not set"); |
| 318 | - if (options.forward_x11 && display != NULL) { |
| 319 | - char *proto, *data; |
| 320 | - /* Get reasonable local authentication information. */ |
| 321 | - client_x11_get_proto(display, options.xauth_location, |
| 322 | - options.forward_x11_trusted, |
| 323 | - options.forward_x11_timeout, &proto, &data); |
| 324 | + if (options.forward_x11 && client_x11_get_proto(display, |
| 325 | + options.xauth_location, options.forward_x11_trusted, |
| 326 | + options.forward_x11_timeout, &proto, &data) == 0) { |
| 327 | /* Request forwarding with authentication spoofing. */ |
| 328 | debug("Requesting X11 forwarding with authentication " |
| 329 | "spoofing."); |