diff options
author | wm4 <wm4@nowhere> | 2019-12-18 07:12:53 +0100 |
---|---|---|
committer | wm4 <wm4@nowhere> | 2019-12-18 07:12:53 +0100 |
commit | 11d35b72a6fdd58f59ea7e593a4a5267f54b2e54 (patch) | |
tree | b38758cebddaa3cdfca83ec0446239110572b33d | |
parent | 5f74ed58286a1339412554932f31844ec1b64280 (diff) | |
download | mpv-11d35b72a6fdd58f59ea7e593a4a5267f54b2e54.tar.bz2 mpv-11d35b72a6fdd58f59ea7e593a4a5267f54b2e54.tar.xz |
x11: fix X property out of bounds memory reads
The size overflow check was inverted: instead of allowing reading only
the first dst_size bytes of the property, it allowed copying past the
property buffer (as returned by xlib). xlib doesn't return the size of
the buffer in bytes, so it has to be computed and checked manually.
Wouldn't it be great if C allowed me to write the overflow check in a
readable way, so it doesn't trick me into writing dumb security bugs?
Relying on X security is even dumber than creating a X security bug,
though, so this was not a real problem. But I found that one specific
call tried to read more than what the property provided, so reduce that.
Also, len*ib obviously can't overflow, so there's an additional layer of
dumb to this whole thing.
While we're at dumb things, why the hell does xlib use "long" for 32 bit
types. It's a god damn pain.
-rw-r--r-- | video/out/x11_common.c | 4 |
1 files changed, 2 insertions, 2 deletions
diff --git a/video/out/x11_common.c b/video/out/x11_common.c index 92dbfc2aa3..8c37074e0b 100644 --- a/video/out/x11_common.c +++ b/video/out/x11_common.c @@ -207,7 +207,7 @@ static bool x11_get_property_copy(struct vo_x11_state *x11, Window w, void *ptr = x11_get_property(x11, w, property, type, format, &len); if (ptr) { size_t ib = format == 32 ? sizeof(long) : format / 8; - if (dst_size / ib >= len) { + if (dst_size <= len * ib) { memcpy(dst, ptr, dst_size); ret = true; } @@ -1079,7 +1079,7 @@ static void vo_x11_check_net_wm_desktop_change(struct vo *vo) if (x11->parent) return; - long params[5] = {0}; + long params[1] = {0}; if (x11_get_property_copy(x11, x11->window, XA(x11, _NET_WM_DESKTOP), XA_CARDINAL, 32, params, sizeof(params))) { |