bug#45162: [PATCH] Fixup Re: bug#45162: 28.0.50; Messages scrambled by socks-filter

["J.P." <jp@neverwas.me>]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Drop-me-add-test-for-SOCKS5-auth-protocol.patch --]
[-- Type: text/x-patch, Size: 4713 bytes --]

From 9145f6044a75b45807a1c2b9e1703da73d5ddf59 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Clear-socks-protocol-scratch-after-authentication.patch --]
[-- Type: text/x-patch, Size: 943 bytes --]

From 81b83735f97383b4ff863a051ab695a321d3fd1c Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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