From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: "J.P." Newsgroups: gmane.emacs.bugs Subject: bug#45162: [PATCH] Fixup Re: bug#45162: 28.0.50; Messages scrambled by socks-filter Date: Thu, 07 Jan 2021 02:35:19 -0800 Message-ID: <87pn2ht4pk.fsf_-_@neverwas.me> References: <87r1nxu840.fsf@neverwas.me> <87sg8c8ic5.fsf@gnus.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="26611"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) Cc: 45162@debbugs.gnu.org To: Lars Ingebrigtsen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Jan 07 11:36:12 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kxSeJ-0006lK-OP for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 07 Jan 2021 11:36:11 +0100 Original-Received: from localhost ([::1]:52718 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kxSeI-0000xe-Mi for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 07 Jan 2021 05:36:10 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:43860) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kxSeA-0000xL-Mk for bug-gnu-emacs@gnu.org; Thu, 07 Jan 2021 05:36:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:35022) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kxSeA-00041E-F4 for bug-gnu-emacs@gnu.org; Thu, 07 Jan 2021 05:36:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kxSeA-0006IU-Cb for bug-gnu-emacs@gnu.org; Thu, 07 Jan 2021 05:36:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: "J.P." Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 07 Jan 2021 10:36:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 45162 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch fixed Original-Received: via spool by 45162-submit@debbugs.gnu.org id=B45162.161001573424170 (code B ref 45162); Thu, 07 Jan 2021 10:36:02 +0000 Original-Received: (at 45162) by debbugs.gnu.org; 7 Jan 2021 10:35:34 +0000 Original-Received: from localhost ([127.0.0.1]:46568 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kxSdh-0006Hm-O2 for submit@debbugs.gnu.org; Thu, 07 Jan 2021 05:35:34 -0500 Original-Received: from qrelay153.mxroute.com ([172.82.139.153]:40443) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kxSdf-0006HX-HO for 45162@debbugs.gnu.org; Thu, 07 Jan 2021 05:35:32 -0500 Original-Received: from filter004.mxroute.com ([149.28.56.236] 149.28.56.236.vultr.com) (Authenticated sender: mN4UYu2MZsgR) by qrelay153.mxroute.com (ZoneMTA) with ESMTPA id 176dc6a48aa000a57c.001 for <45162@debbugs.gnu.org>; Thu, 07 Jan 2021 10:35:21 +0000 X-Zone-Loop: 1287e24e96dfe2faee1b67202dc168756fa158afde48 X-Originating-IP: [149.28.56.236] Original-Received: from eagle.mxlogin.com (23-92-74-70.static.hvvc.us [23.92.74.70]) by filter004.mxroute.com (Postfix) with ESMTPS id B5FE73EDB5; Thu, 7 Jan 2021 10:35:20 +0000 (UTC) In-Reply-To: <87sg8c8ic5.fsf@gnus.org> (Lars Ingebrigtsen's message of "Fri, 11 Dec 2020 16:37:14 +0100") X-AuthUser: masked@neverwas.me X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list 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-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:197468 Archived-At: --=-=-= Content-Type: text/plain Lars and Emacs, My sincerest apologies. I'm embarrassed/ashamed to admit that my previous patch, Commit: abc8d6b9465fecb989170426756c7ee4b133fd40 "Append incremental message segments in socks-filter" was not only incomplete but effectively *broke* socks.el for anyone who's tried using it since! In a boneheaded oversight, I neglected to reset the "scratch" work area after functions performing the SOCKS5 authentication subprotocol are through with it. This is a one-line follow-up that hopefully corrects that. Please see the included test, which passes after this tweak is applied. Below is a long-winded attempt to explain the awkward placement of this change for anyone who cares, present or future. Thanks (and again, so sorry!), J.P. P.S. Please let me know if I need to open a new bug report. Summary of [1] using concrete values for SOCKS 5 and USER/PASS method [2]: 0502 0002 c: auth offer, socks-ver len-methods methods ... 0502 s: auth choice, socks-version method 0103 666f 6f03 6261 72 c: attempt, version len-user "foo" len-pass "bar" 0100 s: verdict, version status 1. (BG) In socks-filter, the WAITING-FOR-AUTH case stashes the latest chunk by extending the existing stash rightward. In practice, this is the first chunk, which contains the server's selection of one of the auth methods offered by the client (Emacs) in the initial request. It then transitions to SUBMETHOD-NEGOTIATION. 2. (FG) Once socks-open-connection has detected that transition, it calls the handler corresponding to the server's chosen auth method to handle the subnegotiation. 3. (FG) That handler, most likely socks-username/password-auth, swaps out the primary process filter, socks-filter, for a temporary one. 4. (BG) That temp filter, socks-username/password-auth-filter, parses out the status code and transitions to AUTHENTICATED (not sure why here, exactly) 5. (FG) The auth handler (3), having detected that transition, returns control (and a verdict) to the opener (2), which, on success, ensures the state is AUTHENTICATED and restores the primary filter (socks-filter). 6. (FG) socks-send-command transitions to WAITING before sending the main request (method call) as per the protocol, which is similar to SOCKS 4. >From the point of view of the socks-filter, the AUTHENTICATED case is effectively invisible because it's preempted explicitly as described above (as opposed to, e.g., left as a no-op to avoid a race). Thus, we can't run anything here, however opportune it may seem. Instead, I tacked the correction onto the end of the opener (which see). Ironically, the original approach of prepending chunks on the left papered over any need to zero out the scratch buffer after these opening negotiations (for the CONNECT method, anyway). [1]: https://tools.ietf.org/html/rfc1928#section-3 [2]: https://tools.ietf.org/html/rfc1929 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-Drop-me-add-test-for-SOCKS5-auth-protocol.patch >From 9145f6044a75b45807a1c2b9e1703da73d5ddf59 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" Date: Thu, 7 Jan 2021 01:10:49 -0800 Subject: [PATCH 1/2] Drop me: add test for SOCKS5 auth protocol --- test/lisp/net/socks-tests.el | 87 ++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 test/lisp/net/socks-tests.el diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el new file mode 100644 index 00000000000..94ba49ab487 --- /dev/null +++ b/test/lisp/net/socks-tests.el @@ -0,0 +1,87 @@ +;;; socks-tests.el --- tests for SOCKS -*- coding: utf-8; lexical-binding: t; -*- + +;;; Commentary: + +;; | (let ((default-directory (expand-file-name "../.."))) +;; | (compile "make lisp/net/socks-tests.log")) + +;;; Code: + +(require 'socks) +(require 'url-http) + +;; This ran successfully against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com +(defun socks-tests-fake-server () + (let* ((lines '(([5 2 0 2] . [5 2]) ; Emacs + ([5 3 0 1 2] . [5 2]) ; curl (+ gssapi) + ([1 3 #x66 #x6f #x6f 3 #x62 #x61 #x72] . [1 0]) + ([5 1 0 3 11 ?e ?x ?a ?m ?p ?l ?e ?. ?c ?o ?m 0 80] + . [5 0 0 1 0 0 0 0 0 0]))) + (sentinel '(lambda (proc event) + (message "[%s]: %s" proc event) + (unless (string-match-p "^open" event) + (message "Quitting") + (kill-emacs 0)))) + (content (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain; charset=UTF-8\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n")) + (filter `(lambda (proc line) + (let* ((content ,content) + (resp (or (assoc-default (vconcat line) test-lines) + (when (string-prefix-p "GET /" line) + content)))) + (message "-> %s" line) + (message "<- %s" resp) + (process-send-string proc (concat resp))))) + (program `(progn + (defvar test-lines ',lines) + (let ((proc (make-network-process :server 1 + :buffer (current-buffer) + :filter ,filter + :sentinel ,sentinel + :name "testnet" + :family 'ipv4 + :coding 'raw-text + :host "127.0.0.1" + :service 10080))) + (message "[%s] Listening on 127.0.0.1:10080" proc) + (while (process-live-p proc) + (sleep-for 1))))) + (buffer (generate-new-buffer "*fake-socks-server*")) + (server (start-process "fake-socks-server" buffer + (concat invocation-directory invocation-name) + "-Q" "--batch" "--eval" (format "%S" program)))) + (set-process-query-on-exit-flag server nil) + (with-current-buffer buffer + (while (not (string-match-p "Listening" (buffer-string))) + (sleep-for 0.1))) + server)) + +(ert-deftest socks-tests-auth-filter-canned-e2e () + ;; 0.31 secs on my machine but marked as expensive because involves IO + :tags '(:expensive-test) + (let* ((socks-server `("fake-server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-gateway-method 'socks) + (url (url-generic-parse-url "http://example.com")) + (server (socks-tests-fake-server)) + ;; + done + ;; + (cb (lambda (&rest _r) + (goto-char (point-min)) + (should (search-forward "Hello World" nil t)) + (message "Found: %S" (buffer-substring (line-beginning-position) + (line-end-position))) + (setq done t)))) + (ignore url-gateway-method) + (ert-info ("Connect to HTTP endpoint over SOCKS5 with USER/PASS method") + (url-http url cb '(nil)) + (while (not done) (sleep-for 0.1))) + (delete-process server) + (kill-buffer (process-buffer server)))) + +;;; socks-tests.el ends here -- 2.29.2 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0002-Clear-socks-protocol-scratch-after-authentication.patch >From 81b83735f97383b4ff863a051ab695a321d3fd1c Mon Sep 17 00:00:00 2001 From: "F. Jason Park" Date: Thu, 7 Jan 2021 01:13:19 -0800 Subject: [PATCH 2/2] Clear socks protocol scratch after authentication * lisp/net/socks.el (socks-open-connection): Fix incomplete patch titled "Append incremental message segments in socks-filter," which addressed chunk ordering but neglected to zero out the work area following successful username/password authentication (bug#45162). Copyright-paperwork-exempt: yes --- lisp/net/socks.el | 1 + 1 file changed, 1 insertion(+) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 0d48fd7e05a..96fafc826b8 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -385,6 +385,7 @@ socks-open-connection ) ) (process-put proc 'socks-state socks-state-authenticated) + (process-put proc 'socks-scratch "") (set-process-filter proc #'socks-filter))) proc))) -- 2.29.2 --=-=-=--