From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Andrew Hyatt Newsgroups: gmane.emacs.bugs Subject: bug#8427: [SECURITY] sql.el -- comint process passwords are leaked to ps(1) listing Date: Mon, 11 Nov 2019 00:31:11 -0500 Message-ID: References: <-DPnoQRPO3mztTMZP0CLEkVHEueQfRbf1NL2NMBa_alnqjzctP5kLNyD-Gd_yioQqTu-QiEXfLGzidBeSrX0jY_-tlyrBEnMU5Mo5febRng=@protonmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="121231"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (darwin) Cc: "8427@debbugs.gnu.org" <8427@debbugs.gnu.org>, Stefan Kangas To: Michael Mauger Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Nov 11 06:32:15 2019 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1iU2JC-000VOL-Cs for geb-bug-gnu-emacs@m.gmane.org; Mon, 11 Nov 2019 06:32:14 +0100 Original-Received: from localhost ([::1]:49230 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iU2JA-00039T-IY for geb-bug-gnu-emacs@m.gmane.org; Mon, 11 Nov 2019 00:32:12 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:42135) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iU2J2-00039J-Da for bug-gnu-emacs@gnu.org; Mon, 11 Nov 2019 00:32:06 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iU2J0-000838-5d for bug-gnu-emacs@gnu.org; Mon, 11 Nov 2019 00:32:04 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:44262) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iU2Iz-000832-TG for bug-gnu-emacs@gnu.org; Mon, 11 Nov 2019 00:32:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1iU2Iz-00015y-MQ for bug-gnu-emacs@gnu.org; Mon, 11 Nov 2019 00:32:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Andrew Hyatt Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 11 Nov 2019 05:32:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 8427 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: security Original-Received: via spool by 8427-submit@debbugs.gnu.org id=B8427.15734502874166 (code B ref 8427); Mon, 11 Nov 2019 05:32:01 +0000 Original-Received: (at 8427) by debbugs.gnu.org; 11 Nov 2019 05:31:27 +0000 Original-Received: from localhost ([127.0.0.1]:53083 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iU2IQ-000157-Cy for submit@debbugs.gnu.org; Mon, 11 Nov 2019 00:31:27 -0500 Original-Received: from mail-qt1-f178.google.com ([209.85.160.178]:33780) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iU2IP-00014u-27 for 8427@debbugs.gnu.org; Mon, 11 Nov 2019 00:31:25 -0500 Original-Received: by mail-qt1-f178.google.com with SMTP id y39so14492280qty.0 for <8427@debbugs.gnu.org>; Sun, 10 Nov 2019 21:31:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=eN7rhK66ZPIGfhehR59s930lmnj1bU2xLfe0vIoqYB0=; b=F55lAkoNSkvL0IyorRxdJwsVdFRSD0SvbmA5aZjTwu949JTeKS9TWH9Xi/U1pk1cZj o+qNmL6O5fdEAZV+DdugCWH4hnamcpJhrhC2hY5yfghWsqNOOcQwmJUzOmnYvJedY93o Uk7etMdCKhvcYsZ/RdPAceYliC+zF3osESqhkXwoZA9uzZa9Tc9HMJwP2KXBVNS5o3F3 e0IOrcbve6AgEQDEBnxqhTQVYWIO7UrYWwLKDLDRIP2pDQWD84T5RBw9h/o4EkeWQi6f WIyqDcb1stBos787ZEDxTg39w+PjpIvbRHnAx2Gc5RL2kHlOkB8kAXQGrnyC/nl9bh20 L8Sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=eN7rhK66ZPIGfhehR59s930lmnj1bU2xLfe0vIoqYB0=; b=NnsMJ+JTFCjlX3sWQkfy5r2djtgsRvCcUZbXCMvoUGYV20M+B8IHoenJK+hlMZTX9J XHVtOL7Li0fuSAlBU0XmJhZYOgrYMjuo66K8DHIW/2sqhnkt7xCGuOCsBAJfDDxEqdty vho3eG8+lmgfHqtBbUFfXXIEqrxbio96LHgM8m4Sif0gj5qgz0s1PvssgZheYqC+P6bY syA2iIdKUIEDkCxhBqRmNRxB6Y2mPH33pj2BV/dzPDMf1fYjBP67ASHuQkeF7CamHoG+ FA2nz1lBo59MhZ0RQ6Wni1M6+kSPzMWV83MTmNb80CWyheOvWX++A7aqOUf6byBEzApq W0gA== X-Gm-Message-State: APjAAAXx+KYn5PJHWalGaM6DZqov3Oay/w4EcEf81c0KLlOUXXAE1wGH HLCzs9msxdCydh/fHdurxOE= X-Google-Smtp-Source: APXvYqw2FbXRyR53hRfKzL0//+dSCpsX1CyFIUl6N924TgkQIrEkl79YU2Ys1QV5c5aRf9+GkR88Rw== X-Received: by 2002:ac8:7103:: with SMTP id z3mr4909553qto.387.1573450279241; Sun, 10 Nov 2019 21:31:19 -0800 (PST) Original-Received: from ahyatt-macbookpro6.roam.corp.google.com (pool-74-101-146-201.nycmny.fios.verizon.net. [74.101.146.201]) by smtp.gmail.com with ESMTPSA id a18sm7403705qkc.2.2019.11.10.21.31.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 10 Nov 2019 21:31:18 -0800 (PST) In-Reply-To: (Michael Mauger's message of "Sat, 02 Nov 2019 19:41:44 +0000") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.51.188.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:171416 Archived-At: --=-=-= Content-Type: text/plain I've simplified an implementation along the lines you suggest, and tested it via ert. I'm attaching the latest version of the patch. Please let me know what you think. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Enable-passwords-to-be-sent-in-process-when-possible.patch >From 2d0632b08350d86049c2e20c50ce67d69ad52c6d Mon Sep 17 00:00:00 2001 From: Andrew Hyatt Date: Fri, 18 Oct 2019 21:56:52 -0400 Subject: [PATCH] Enable passwords to be sent in-process when possible. * lisp/progmodes/sql.el (sql-comint, sql-comint-mysql): Add a way to handle passwords to be sent in the comint process. This is controlled by the sql-product variable :password-in-comint. When true, on the first password prompt, send argument to signal to the SQL process to read the password inside the process. This removes the slight chance that someone can spy on the password from ps or via other methods. * test/lisp/progmodes/sql-tests.el: New tests for the password interception. --- lisp/progmodes/sql.el | 60 +++++++++++++++++++++++++------ test/lisp/progmodes/sql-tests.el | 61 ++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 10 deletions(-) diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el index b17364b08f..f7cbec7130 100644 --- a/lisp/progmodes/sql.el +++ b/lisp/progmodes/sql.el @@ -160,13 +160,16 @@ ;; "Connect ti XyzDB in a comint buffer." ;; ;; ;; Do something with `sql-user', `sql-password', -;; ;; `sql-database', and `sql-server'. +;; ;; `sql-database', and `sql-server'. `sql-password' will +;; ;; be sent automatically if not sent in the command-line. +;; ;; It is recommended to avoid sending in the command-line +;; ;; if possible, since this can briefly expose passwords. ;; (let ((params ;; (append ;; (if (not (string= "" sql-user)) ;; (list "-U" sql-user)) ;; (if (not (string= "" sql-password)) -;; (list "-P" sql-password)) +;; (list "-P")) ;; (if (not (string= "" sql-database)) ;; (list "-D" sql-database)) ;; (if (not (string= "" sql-server)) @@ -458,6 +461,7 @@ file. Since that is a plaintext file, this could be dangerous." :sqli-comint-func sql-comint-mysql :list-all "SHOW TABLES;" :list-table "DESCRIBE %s;" + :password-in-comint t :prompt-regexp "^mysql> " :prompt-length 6 :prompt-cont-regexp "^ -> " @@ -624,6 +628,10 @@ may be any one of the following: not-nil it is the name of a schema whose objects should be listed. + :password-in-comint true when the password is not passed in + as a parameter, but instead requested in + the comint session itself. + :prompt-regexp regular expression string that matches the prompt issued by the product interpreter. @@ -1402,6 +1410,9 @@ You can change `sql-prompt-length' on `sql-interactive-mode-hook'.") Used by `sql-rename-buffer'.") +(defvar-local sql-password-accepted-via-comint nil + "Set to true when the password was accepted via comint.") + (defun sql-buffer-live-p (buffer &optional product connection) "Return non-nil if the process associated with buffer is live. @@ -4681,9 +4692,9 @@ the call to \\[sql-product-interactive] with (sql-generate-unique-sqli-buffer-name product nil)) ((consp new-name) (sql-generate-unique-sqli-buffer-name product - (read-string - "Buffer name (\"*SQL: XXX*\"; enter `XXX'): " - (sql-make-alternate-buffer-name product)))) + (read-string + "Buffer name (\"*SQL: XXX*\"; enter `XXX'): " + (sql-make-alternate-buffer-name product)))) ((stringp new-name) (if (or (string-prefix-p " " new-name) (string-match-p "\\`[*].*[*]\\'" new-name)) @@ -4733,12 +4744,30 @@ the call to \\[sql-product-interactive] with (get-buffer new-sqli-buffer))))) (user-error "No default SQL product defined: set `sql-product'"))) +(defun sql-watch-for-password-prompt (string) + "Intercept password prompts when we know the password. +This must also do the job of detecting password prompts. STRING +is the potential password prompt." + (if (and + sql-password + (not (string= "" sql-password)) + (not sql-password-accepted-via-comint)) + ;; In this case, we are in charge of watching for the password + ;; prompt, so let's accept or reject. If the sql-password + ;; fails, they would have to enter it manually next time. + (let ((case-fold-search t)) + (when (string-match comint-password-prompt-regexp string) + (setq sql-password-accepted-via-comint t) + (funcall comint-input-sender (get-buffer-process (current-buffer)) + sql-password))) + (comint-watch-for-password-prompt string))) + (defun sql-comint (product params &optional buf-name) "Set up a comint buffer to run the SQL processor. -PRODUCT is the SQL product. PARAMS is a list of strings which are -passed as command line arguments. BUF-NAME is the name of the new -buffer. If nil, a name is chosen for it." +PRODUCT is the SQL product. PARAMS is a list of strings which +are passed as command line arguments. BUF-NAME is the name of +the new buffer. If nil, a name is chosen for it." (let ((program (sql-get-product-feature product :sqli-program))) ;; Make sure we can find the program. `executable-find' does not @@ -4757,12 +4786,21 @@ buffer. If nil, a name is chosen for it." (setq buf-name (sql-generate-unique-sqli-buffer-name product nil))) (set-text-properties 0 (length buf-name) nil buf-name) + ;; Create the buffer first, because we want to set it up before + ;; comint starts to run. + (set-buffer (get-buffer-create buf-name)) + (when (sql-get-product-feature product :password-in-comint) + (setq sql-password-accepted-via-comint nil) + ;; Substitute our own password watcher function. + (add-hook 'comint-output-filter-functions 'sql-watch-for-password-prompt) + (remove-hook 'comint-output-filter-functions 'comint-watch-for-password-prompt)) + ;; Start the command interpreter in the buffer ;; PROC-NAME is BUF-NAME without enclosing asterisks (let ((proc-name (replace-regexp-in-string "\\`[*]\\(.*\\)[*]\\'" "\\1" buf-name))) (set-buffer (apply #'make-comint-in-buffer - proc-name buf-name program nil params))))) + proc-name (current-buffer) program nil params))))) ;;;###autoload (defun sql-oracle (&optional buffer) @@ -5188,7 +5226,9 @@ The default comes from `process-coding-system-alist' and (if (not (string= "" sql-user)) (list (concat "--user=" sql-user))) (if (not (string= "" sql-password)) - (list (concat "--password=" sql-password))) + ;; Sending --password will make MySQL prompt for the + ;; password. + (list "--password")) (if (not (= 0 sql-port)) (list (concat "--port=" (number-to-string sql-port)))) (if (not (string= "" sql-server)) diff --git a/test/lisp/progmodes/sql-tests.el b/test/lisp/progmodes/sql-tests.el index 3ac9fb10e4..278e5aba87 100644 --- a/test/lisp/progmodes/sql-tests.el +++ b/test/lisp/progmodes/sql-tests.el @@ -410,6 +410,67 @@ The ACTION will be tested after set-up of PRODUCT." (kill-buffer "*SQL: exist*"))) +(defmacro sql-watch-test-harness (expected &rest action) + "Set-up state and replace functions for SQL password test. + +EXPECTED could be: + - 'passthrough, to indicate that we expect that we pass through + to the normal comint function. + - 'both to indicate that we expected a password to be sent as well + as a prompt to passed through. + - nil, to indicate that nothing happens, including no passthrough. + - a string to indicate that the string is sent to the process + as a password. +ACTION is the body of the test." + `(let ((sent-password) + (input-called 0) + (comint-watch-called 0)) + (with-temp-buffer + (cl-letf ((comint-input-sender (lambda (_ password) (incf input-called) (setq sent-password password))) + ((symbol-function 'comint-watch-for-password-prompt) (lambda (_) (incf comint-watch-called))) + (sql-product 'sqltest) + (sql-product-alist '((sqltest + :name "SqlTest" + :sql-password-accepted-via-comint t)))) + ,@action)) + + (cond ((eq ,expected 'passthrough) + (should (= 1 comint-watch-called)) + (should (= 0 input-called))) + ((eq ,expected 'both) + (should (= 1 comint-watch-called)) + (should (= 1 input-called))) + ((null ,expected) + (should (= 0 comint-watch-called)) + (should (= 0 input-called))) + ((stringp ,expected) + (should (string-equal ,expected sent-password)) + (should (= 0 comint-watch-called)))))) + +(ert-deftest sql-tests-watch-for-password-prompt-no-password () + (sql-watch-test-harness + 'passthrough + (setq sql-password nil) + (sql-watch-for-password-prompt "Password:")) + (sql-watch-test-harness + 'passthrough + (setq sql-password "") + (sql-watch-for-password-prompt "Password:"))) + +(ert-deftest sql-tests-watch-for-password-prompt-right-prompt () + (sql-watch-test-harness + nil + (setq sql-password "password") + (sql-watch-for-password-prompt "SQL> "))) + +(ert-deftest sql-tests-watch-for-password-prompt-second-password () + ;; The harness itself makes sure we don't send the password more + ;; than once. + (sql-watch-test-harness + 'both + (setq sql-password "password") + (sql-watch-for-password-prompt "Password:") + (sql-watch-for-password-prompt "Password:"))) (provide 'sql-tests) ;;; sql-tests.el ends here -- 2.20.1 (Apple Git-117) --=-=-= Content-Type: text/plain Michael Mauger writes: > On Saturday, November 2, 2019 1:10 AM, Andrew Hyatt wrote: > >> Michael Mauger mmauger@protonmail.com writes: >> >> > On Sunday, October 20, 2019 8:56 PM, Andrew Hyatt ahyatt@gmail.com wrote: >> > >> >> Your advice is good, but following it led me to some complexity I can't >> seem to get away from. Perhaps you have some insight, so let me explain. >> The issue is that, yes, I can not advise the comint function. However, >> if I supply my own function, then I have to remove the >> comint-watch-for-password-prompt, supply my own function, then restore >> it when the user has entered their password (so it can handle subsequent >> password entries). This juggling of the normal >> comint-watch-for-password-prompt method, plus the fact that we basically >> have to reimplement part of it, gives me pause - I think it's probably >> too hacky a solution. >> >> There's a few ways out. We could introduce a variable used in >> sql-product-alist that tells SQL not to prompt for a password because >> the db will just get it via the comint password function. That would >> probably work well, but it wouldn't store the sql-password at all, that >> variable would be unused. Maybe that's OK, maybe not - I don't have a >> good sense for it. >> >> Or, we could make this auto-password-supplying per-buffer a part of >> comint itself. That would widen the scope of the fix, but it would >> probably be the best of both functionality and simplicity. >> >> What do you think? >> > > I totally understand the complexity, but I don't think it has too be too > complicated to address. > > First the sql.el only solution: If the sql-comint function decides to pass > the password via stdin then it can set a buffer-local flag indicating this > and then replace `coming-watch-for-password-prompt' on the > `comint-output-filter-functions' list with the sql version of the function. > The sql password function would be something along the lines of: > > ;; TOTALLY NOT TESTED > (defun sql-watch-for-password-prompt (string) > "blah blah ;)" > (if sql-will-prompt-for-password > ;; (based on comint-watch-for-password-prompt) vvv > (when (let ((case-fold-search t)) > (string-match (or (sql-get-product-feature sql-product 'password-prompt-regexp string) > comint-password-prompt-regexp))) > (when (string-match "^[ \n\r\t\v\f\b\a]+" string) > (setq string (replace-match "" t t string))) > (let ((comint--prompt-recursion-depth (1+ comint--prompt-recursion-depth))) > (if (> comint--prompt-recursion-depth 10) > (message "Password prompt recursion too deep") > ;;; ^^^ > ;;; automagically provide the password > (let ((proc (get-buffer-process (current-buffer)))) > (when proc > (funcall comint-input-sender proc sql-password)))))) > ;; Back to default behavior > (comint-watch-for-password-prompt string)) > ;; Make sure we don't supply again > (setq-local sql-will-prompt-password nil)) > > That should get you close without too much difficulty. Of course, it requires a > that a password-prompt-regexp feature is defined for the sql product and that the > sql-comint function defines a buffer-local flag `sql-will-prompt-for-password' in > it is deferring to stdin. > > The other solution would involve modifying comint to call a hook if set to supply > a password or nil. This would probably be a simpler change but may get more > broader attention. When the hook function is not set or returns nil then do the > default behavior of calling `comint-send-invisible' otherwise just send the password > > There are some edge cases here, but this hopefully helps. Also, obviously, test cases > are needed given that if this breaks, we break the sql interactive world! > > -- > MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer --=-=-=--