diff --git a/docs/graphics-protocol.rst b/docs/graphics-protocol.rst index 65a3f4814..05ea619ac 100644 --- a/docs/graphics-protocol.rst +++ b/docs/graphics-protocol.rst @@ -284,7 +284,7 @@ and can take the values: Value of `t` Meaning ================== ============ ``d`` Direct (the data is transmitted within the escape code itself) -``f`` A simple file (regular files only, not named pipes or similar) +``f`` A simple file (regular files only, not named pipes, device files, etc.) ``t`` A temporary file, the terminal emulator will delete the file after reading the pixel data. For security reasons the terminal emulator should only delete the file if it is in a known temporary directory, such as :file:`/tmp`, @@ -302,7 +302,11 @@ Value of `t` Meaning When opening files, the terminal emulator must follow symlinks. In case of symlink loops or too many symlinks, it should fail and respond with an error, -similar to reporting any other kind of I/O error. +similar to reporting any other kind of I/O error. Since the file paths come +from potentially untrusted sources, terminal emulators **must** refuse to read +any device/socket/etc. special files. Only regular files are allowed. +Additionally, terminal emulators may refuse to read files in *sensitive* +parts of the filesystem, such as :file:`/proc`, :file:`/sys`, :file:`/dev/`, etc. Local client ^^^^^^^^^^^^^^ diff --git a/kitty/boss.py b/kitty/boss.py index 866a90728..ee1f479bb 100644 --- a/kitty/boss.py +++ b/kitty/boss.py @@ -131,6 +131,7 @@ from .utils import ( func_name, get_editor, get_new_os_window_size, + is_ok_to_read_image_file, is_path_in_temp_dir, less_version, log_error, @@ -2441,6 +2442,9 @@ class Boss: with suppress(FileNotFoundError): os.remove(path) + def is_ok_to_read_image_file(self, path: str, fd: int) -> bool: + return is_ok_to_read_image_file(path, fd) + def set_update_check_process(self, process: Optional['PopenType[bytes]'] = None) -> None: if self.update_check_process is not None: with suppress(Exception): diff --git a/kitty/graphics.c b/kitty/graphics.c index 3fe289b8e..4b7ddb520 100644 --- a/kitty/graphics.c +++ b/kitty/graphics.c @@ -421,8 +421,19 @@ load_image_data(GraphicsManager *self, Image *img, const GraphicsCommand *g, con if (g->payload_sz > 2048) ABRT("EINVAL", "Filename too long"); snprintf(fname, sizeof(fname)/sizeof(fname[0]), "%.*s", (int)g->payload_sz, payload); if (transmission_type == 's') fd = safe_shm_open(fname, O_RDONLY, 0); - else fd = safe_open(fname, O_CLOEXEC | O_RDONLY, 0); + else fd = safe_open(fname, O_CLOEXEC | O_RDONLY | O_NONBLOCK, 0); // O_NONBLOCK so that opening a FIFO pipe does not block if (fd == -1) ABRT("EBADF", "Failed to open file for graphics transmission with error: [%d] %s", errno, strerror(errno)); + if (global_state.boss) { + DECREF_AFTER_FUNCTION PyObject *cret_ = PyObject_CallMethod(global_state.boss, "is_ok_to_read_image_file", "si", fname, fd); + if (cret_ == NULL) { + PyErr_Print(); + ABRT("EBADF", "Failed to check file for read permission"); + } + if (cret_ != Py_True) { + log_error("Refusing to read image file as permission was denied"); + ABRT("EPERM", "Permission denied to read image file"); + } + } load_data->loading_completed_successfully = mmap_img_file(self, fd, g->data_sz, g->data_offset); safe_close(fd, __FILE__, __LINE__); if (transmission_type == 't' && strstr(fname, "tty-graphics-protocol") != NULL) { diff --git a/kitty/utils.py b/kitty/utils.py index 8b1e4a580..326fe356c 100644 --- a/kitty/utils.py +++ b/kitty/utils.py @@ -712,6 +712,26 @@ def is_path_in_temp_dir(path: str) -> bool: return False +def is_ok_to_read_image_file(path: str, fd: int) -> bool: + import stat + path = os.path.abspath(os.path.realpath(path)) + try: + path_stat = os.stat(path, follow_symlinks=True) + fd_stat = os.fstat(fd) + except OSError: + return False + if not os.path.samestat(path_stat, fd_stat): + return False + parts = path.split(os.sep)[1:] + if len(parts) < 1: + return False + if parts[0] in ('sys', 'proc', 'dev'): + if parts[0] == 'dev': + return len(parts) > 2 and parts[1] == 'shm' + return False + return stat.S_ISREG(fd_stat.st_mode) + + def resolve_abs_or_config_path(path: str, env: Optional[Mapping[str, str]] = None, conf_dir: Optional[str] = None) -> str: path = os.path.expanduser(path) path = expandvars(path, env or {}) diff --git a/kitty_tests/datatypes.py b/kitty_tests/datatypes.py index e5f13933b..ecd026747 100644 --- a/kitty_tests/datatypes.py +++ b/kitty_tests/datatypes.py @@ -20,7 +20,7 @@ from kitty.fast_data_types import ( ) from kitty.fast_data_types import Cursor as C from kitty.rgb import to_color -from kitty.utils import is_path_in_temp_dir, sanitize_title, sanitize_url_for_dispay_to_user +from kitty.utils import is_ok_to_read_image_file, is_path_in_temp_dir, sanitize_title, sanitize_url_for_dispay_to_user from . import BaseTest, filled_cursor, filled_history_buf, filled_line_buf @@ -444,6 +444,21 @@ class TestDataTypes(BaseTest): self.assertTrue(is_path_in_temp_dir(os.path.join(prefix, path))) for path in ('/home/xy/d.png', '/tmp/../home/x.jpg'): self.assertFalse(is_path_in_temp_dir(os.path.join(path))) + for path in '/proc/self/cmdline /dev/tty'.split(): + if os.path.exists(path): + with open(path) as pf: + self.assertFalse(is_ok_to_read_image_file(path, pf.fileno()), path) + fifo = os.path.join(tempfile.gettempdir(), 'test-kitty-fifo') + os.mkfifo(fifo) + fifo_fd = os.open(fifo, os.O_RDONLY | os.O_NONBLOCK) + try: + self.assertFalse(is_ok_to_read_image_file(fifo, fifo_fd), fifo) + finally: + os.close(fifo_fd) + os.remove(fifo) + if os.path.isdir('/dev/shm'): + with tempfile.NamedTemporaryFile(dir='/dev/shm') as tf: + self.assertTrue(is_ok_to_read_image_file(tf.name, tf.fileno()), fifo) self.ae(sanitize_url_for_dispay_to_user( 'h://a\u0430b.com/El%20Ni%C3%B1o/'), 'h://xn--ab-7kc.com/El Niño/')