[-- Attachment #1: Type: text/plain, Size: 3958 bytes --] Severity: minor Tags: patch Hi, If this is indeed a bug, I'm guessing it's a pretty minor one because in most cases, the bulk of a SOCKS reply doesn't seem to matter, so long as the status code portion is correctly interpreted. But in rarer cases, for example when using Tor's RESOLVE command, which is part of a nonstandard extension suite, a response may need to be preserved intact. Rather than try to explain, I've included a throwaway before/after test showing current and expected behavior, as well as a small patch that seems to fix the issue. If this is all nonsense, please forgive me. I am no expert, just a grateful end user. Thanks, J.P. In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.23, cairo version 1.16.0) of 2020-12-09 built on pc Repository revision: 4f352ad6f1759ae6dcff6ba43847273491bf9c30 Repository branch: socks Windowing system distributor 'Fedora Project', version 11.0.12010000 System Description: Fedora 33 (Workstation Edition) Configured using: 'configure --with-dbus --with-gif --with-jpeg --with-png --with-rsvg --with-tiff --with-xft --with-xpm --with-x-toolkit=gtk3 --with-gpm=no --with-xwidgets --with-modules --with-harfbuzz --with-cairo --with-json CC=gcc 'CFLAGS=-DMAIL_USE_LOCKF -O0 -g3 -Wall' LDFLAGS=-Wl,-z,relro PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig' Configured features: XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS XWIDGETS LIBSYSTEMD JSON PDUMPER LCMS2 Important settings: value of $LANG: en_US.UTF-8 value of $XMODIFIERS: @im=ibus locale-coding-system: utf-8-unix Major mode: Lisp Interaction Minor modes in effect: tooltip-mode: t global-eldoc-mode: t eldoc-mode: t electric-indent-mode: t mouse-wheel-mode: t tool-bar-mode: t menu-bar-mode: t file-name-shadow-mode: t global-font-lock-mode: t font-lock-mode: t blink-cursor-mode: t auto-composition-mode: t auto-encryption-mode: t auto-compression-mode: t line-number-mode: t transient-mark-mode: t Load-path shadows: None found. Features: (shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs rfc822 mml easymenu mml-sec epa derived epg epg-config gnus-util rmail rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs password-cache json map text-property-search time-date subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock syntax facemenu font-core term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european ethiopic indian cyrillic chinese composite charscript charprop case-table epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button loaddefs faces cus-face macroexp files window text-properties overlay sha1 md5 base64 format env code-pages mule custom widget hashtable-print-readable backquote threads xwidget-internal dbusbind inotify lcms2 dynamic-setting system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit x multi-tty make-network-process emacs) Memory information: ((conses 16 51710 8248) (symbols 48 6739 1) (strings 32 18249 1932) (string-bytes 1 610727) (vectors 16 12311) (vector-slots 8 170737 6932) (floats 8 21 45) (intervals 56 219 0) (buffers 984 10)) [-- Attachment #2: 0001-DROP-ME-add-test-explaining-existing-bug.patch --] [-- Type: text/x-patch, Size: 5345 bytes --] From da49cec6391f282ff998e5ace022c74805dabcf8 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Thu, 10 Dec 2020 03:04:48 -0800 Subject: [PATCH 1/3] DROP ME: add test explaining existing bug This is file is meant to demonstrate existing behavior. It is not meant for inclusion in the test suite. --- test/lisp/net/socks-tests.el | 85 ++++++++++++++++++++++++++++++++++++ 1 file changed, 85 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..601da494a26 --- /dev/null +++ b/test/lisp/net/socks-tests.el @@ -0,0 +1,85 @@ +;;; socks-tests.el --- tests for SOCKS -*- coding: utf-8; lexical-binding: t; -*- + +;;; Commentary: + +;; This file is supposed to describe (what's believed to be) an existing bug. +;; +;; | (let ((default-directory (expand-file-name "../.."))) +;; | (compile "make lisp/net/socks-tests.log")) + +(require 'socks) + +(ert-deftest socks-filter-test-existing-behavior-v4 () + (let* ((buf (generate-new-buffer "*test-socks-filter*")) + (proc (start-process "test-socks-filter" buf "sleep" "1"))) + (process-put proc 'socks t) + (process-put proc 'socks-state socks-state-waiting) + (process-put proc 'socks-server-protocol 4) + (ert-info ("Receive initial incomplete segment") + (should (equal (string-to-vector "\0Z\0\0]¸Ø") [0 90 0 0 93 184 216])) + ;; An IPv4 address from example.com OK status ^ ^ msg start + (socks-filter proc "\0Z\0\0]¸Ø") + (ert-info ("State still set to waiting") + (should (eq (process-get proc 'socks-state) socks-state-waiting))) + (ert-info ("Response field is nil because processing incomplete") + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch field holds stashed partial payload") + (should (string= "\0Z\0\0]¸Ø" (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (proto length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Response field holds last scratch with new segment prepended") + (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-response)))) + (ert-info ("Scratch remains populated, matches response field") + (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-scratch))))) + (should (equal (string-to-vector "\"\0Z\0\0]¸Ø") [34 0 90 0 0 93 184 216])) + (delete-process proc) + (set-process-query-on-exit-flag proc nil) + (kill-buffer buf))) + +(ert-deftest socks-filter-test-existing-behavior-v5 () + (let* ((buf (generate-new-buffer "*test-socks-filter*")) + (proc (start-process "test-socks-filter" buf "sleep" "1"))) + (process-put proc 'socks t) + (process-put proc 'socks-state socks-state-waiting) + (process-put proc 'socks-server-protocol 5) + (ert-info ("Receive initial incomplete segment") + ;; An Ipv6 addr for fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + (should (equal (string-to-vector (concat + "\5\0\0\4" + "\x26\x05\xbc\x80\x30\x10\x00\x60" + "\xde\xad\xbe\xef\xca\xfe\xfe\xd9" + "\0\0")) + (vconcat + [5 0 0 4] ; version status (OK) noop addr-type (4 -> 6!) + [#x26 #x05 #xbc #x80 #x30 #x10 #x00 #x60] + [#xde #xad #xbe #xef #xca #xfe #xfe #xd9] + [0 0]))) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") + (ert-info ("State still set to waiting") + (should (eq (process-get proc 'socks-state) socks-state-waiting))) + (ert-info ("Response field is nil because processing incomplete") + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch field holds partial payload of pending msg") + (should (string= "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60" + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (ert-info ("Error raised because parsing logic expects appended bytes") + (let ((err (should-error (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9")))) + (should (equal err '(wrong-type-argument number-or-marker-p nil))))) + (ert-info ("State untouched (still waiting)") + (should (eq (process-get proc 'socks-state) socks-state-waiting))) + (ert-info ("Response field untouched") + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival prepended") + (should (string= (concat "\xde\xad\xbe\xef\xca\xfe\xfe\xd9" + "\5\0\0\4" + "\x26\x05\xbc\x80\x30\x10\x00\x60") + (process-get proc 'socks-scratch))))) + (delete-process proc) + (set-process-query-on-exit-flag proc nil) + (kill-buffer buf))) + + +;;; socks-tests.el ends here -- 2.28.0 [-- Attachment #3: 0002-DROP-ME-update-test-to-reflect-expected-behavior.patch --] [-- Type: text/x-patch, Size: 4096 bytes --] From b8467e34275f371e8e6381b929498fac7864f4a7 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Thu, 10 Dec 2020 03:11:38 -0800 Subject: [PATCH 2/3] DROP ME: update test to reflect expected behavior The tests now show SOCKS v4 and v5 messages successfully received. --- test/lisp/net/socks-tests.el | 37 +++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index 601da494a26..af310b8e6e9 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -29,11 +29,11 @@ socks-filter-test-existing-behavior-v4 (socks-filter proc "\42") ; ?\" 34 (ert-info ("State transitions to complete (proto length check passes)") (should (eq (process-get proc 'socks-state) socks-state-connected))) - (ert-info ("Response field holds last scratch with new segment prepended") - (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-response)))) + (ert-info ("Response field holds last scratch with new segment appended") + (should (string= "\0Z\0\0]¸Ø\"" (process-get proc 'socks-response)))) (ert-info ("Scratch remains populated, matches response field") - (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-scratch))))) - (should (equal (string-to-vector "\"\0Z\0\0]¸Ø") [34 0 90 0 0 93 184 216])) + (should (string= "\0Z\0\0]¸Ø\"" (process-get proc 'socks-scratch))))) + (should (equal (string-to-vector "\0Z\0\0]¸Ø\"") [0 90 0 0 93 184 216 34])) (delete-process proc) (set-process-query-on-exit-flag proc nil) (kill-buffer buf))) @@ -64,19 +64,30 @@ socks-filter-test-existing-behavior-v5 (ert-info ("Scratch field holds partial payload of pending msg") (should (string= "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60" (process-get proc 'socks-scratch))))) - (ert-info ("Last part arrives") - (ert-info ("Error raised because parsing logic expects appended bytes") - (let ((err (should-error (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9")))) - (should (equal err '(wrong-type-argument number-or-marker-p nil))))) - (ert-info ("State untouched (still waiting)") + (ert-info ("Second part arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State untouched") (should (eq (process-get proc 'socks-state) socks-state-waiting))) (ert-info ("Response field untouched") (should-not (process-get proc 'socks-response))) - (ert-info ("Scratch contains new arrival prepended") - (should (string= (concat "\xde\xad\xbe\xef\xca\xfe\xfe\xd9" - "\5\0\0\4" - "\x26\x05\xbc\x80\x30\x10\x00\x60") + (ert-info ("Scratch contains new arrival appended") + (should (string= (concat "\5\0\0\4" + "\x26\x05\xbc\x80\x30\x10\x00\x60" + "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") (process-get proc 'socks-scratch))))) + (ert-info ("Final part arrives (port number)") + (socks-filter proc "\0\0") + (ert-info ("State transitions to complete") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch contains final part appended") + (should (string= (concat "\5\0\0\4" + "\x26\x05\xbc\x80\x30\x10\x00\x60" + "\xde\xad\xbe\xef\xca\xfe\xfe\xd9" + "\0\0") + (process-get proc 'socks-scratch)))) + (ert-info ("Response field updated with final message") + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) (delete-process proc) (set-process-query-on-exit-flag proc nil) (kill-buffer buf))) -- 2.28.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #4: 0003-Append-incremental-message-segments-in-socks-filter.patch --] [-- Type: text/x-patch, Size: 1881 bytes --] From ae5b7b4c19ebdf53f84e0bf4750a734ae1fe908d Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Thu, 10 Dec 2020 03:12:49 -0800 Subject: [PATCH 3/3] Append incremental message segments in socks-filter Prior to this, arriving segments were prepended, resulting in a scrambled message being stored in the socks-response property for the network process. This also fixes a bug in socks--open-network-stream involving the socks-service property being set to the host instead of the port. --- lisp/net/socks.el | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 9b22a5083fb..cb50a0adbea 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -260,7 +260,7 @@ socks-filter (setq state (process-get proc 'socks-state)) (cond ((= state socks-state-waiting-for-auth) - (cl-callf (lambda (s) (setq string (concat string s))) + (cl-callf (lambda (s) (setq string (concat s string))) (process-get proc 'socks-scratch)) (if (< (length string) 2) nil ; We need to spin some more @@ -272,7 +272,7 @@ socks-filter ((= state socks-state-authenticated) ) ((= state socks-state-waiting) - (cl-callf (lambda (s) (setq string (concat string s))) + (cl-callf (lambda (s) (setq string (concat s string))) (process-get proc 'socks-scratch)) (setq version (process-get proc 'socks-server-protocol)) (cond @@ -542,7 +542,7 @@ socks--open-network-stream service)) (process-put proc 'socks-buffer buffer) (process-put proc 'socks-host host) - (process-put proc 'socks-service host) + (process-put proc 'socks-service service) (set-process-filter proc nil) (set-process-buffer proc (if buffer (get-buffer-create buffer))) proc)))) -- 2.28.0
"J.P." <jp@neverwas.me> writes: > Rather than try to explain, I've included a throwaway before/after test > showing current and expected behavior, as well as a small patch that > seems to fix the issue. If this is all nonsense, please forgive me. I am > no expert, just a grateful end user. Thanks; the patch makes sense to me, so I've applied it to Emacs 28. This change was small enough to apply without assigning copyright to the FSF, but for future patches you want to submit, it might make sense to get the paperwork started now, so that subsequent patches can be applied speedily. Would you be willing to sign such paperwork? -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
> Thanks; the patch makes sense to me, so I've applied it to Emacs 28. Oh no, thank YOU, Lars and Emacs! > ... Would you be willing to sign such paperwork? Yes sir, I'd be willing.
"J.P." <jp@neverwas.me> writes:
>> ... Would you be willing to sign such paperwork?
>
> Yes sir, I'd be willing.
Great! Here's the paperwork to get started:
Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.
Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES
[What is the name of the program or package you're contributing to?]
Emacs
[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]
[Do you have an employer who might have a basis to claim to own
your changes? Do you attend a school which might make such a claim?]
[For the copyright registration, what country are you a citizen of?]
[What year were you born?]
[Please write your email address here.]
[Please write your postal address here.]
[Which files have you changed so far, and which new files have you written
so far?]
[-- 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
"J.P." <jp@neverwas.me> writes: > 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. Thanks; I've now applied the fix to Emacs 28, but not the test (because the amount of code would require a copyright assignment). I forgot whether I've asked before -- would you be willing to sign such paperwork? If so, reworking that test into an ert-deftest would be nice. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
Lars Ingebrigtsen <larsi@gnus.org> writes:
> I forgot whether I've asked before -- would you be willing to sign such
> paperwork? If so, reworking that test into an ert-deftest would be
> nice.
I did indeed begin the paperwork process and sent the signed documents
on December 28. I'd be happy to rework the test portion to conform to
your standards and, when I hear back, resubmit it (in this thread?).
BTW, in case you didn't notice, the one test in the last attachment is
already an ert-deftest. But I'm sure I failed to adequately follow the
coding conventions. I'll definitely review those and revise accordingly.
Thanks so much, Lars, for being so gracious and tolerating my
sloppiness. I pledge to be more responsible in the future.
[-- Attachment #1: Type: text/plain, Size: 308 bytes --] Just got my paperwork sorted, yay! (Famous last words.) I've attempted to rework the test as suggested. It now runs in process instead of spawning another Emacs. I wasn't sure whether having url-http as a dependency made sense but left it as is. Happy to change whatever needs changing. Thanks again, J.P. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Create-new-test-file-for-socks.el.patch --] [-- Type: text/x-patch, Size: 5277 bytes --] From 9b89dcdc8702afaebff61708be4c5d6da90d1108 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Sat, 9 Jan 2021 02:52:08 -0800 Subject: [PATCH] Create new test file for socks.el * test/lisp/net/socks-tests.el (socks-tests-auth-filter-url-http): Add SOCKS5 authentication test and fake server. Requires url-http. --- test/lisp/net/socks-tests.el | 103 +++++++++++++++++++++++++++++++++++ 1 file changed, 103 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..b378ed2964e --- /dev/null +++ b/test/lisp/net/socks-tests.el @@ -0,0 +1,103 @@ +;;; socks-tests.el --- tests for SOCKS -*- coding: utf-8; lexical-binding: t; -*- + +;; Copyright (C) 2021 Free Software Foundation, Inc. + +;; This file is part of GNU Emacs. + +;; GNU Emacs is free software: you can redistribute it and/or modify +;; it under the terms of the GNU General Public License as published by +;; the Free Software Foundation, either version 3 of the License, or +;; (at your option) any later version. + +;; GNU Emacs is distributed in the hope that it will be useful, +;; but WITHOUT ANY WARRANTY; without even the implied warranty of +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;; GNU General Public License for more details. + +;; You should have received a copy of the GNU General Public License +;; along with GNU Emacs. If not, see <https://www.gnu.org/licenses/>. + +;;; Commentary: + +;;; Code: + +(require 'socks) +(require 'url-http) + +(defvar socks-tests-canned-server-port nil) + +(defun socks-tests-canned-server-create (verbatim patterns) + "Create a fake SOCKS server and return the process. + +`VERBATIM' and `PATTERNS' are dotted alists containing responses. +Requests are tried in order. On failure, an error is raised." + (let* ((buf (generate-new-buffer "*canned-socks-server*")) + (filt (lambda (proc line) + (let ((resp (or (assoc-default line verbatim + (lambda (k s) ; s is line + (string= (concat k) s))) + (assoc-default line patterns + (lambda (p s) + (string-match-p p s)))))) + (unless resp + (error "Unknown request: %s" line)) + (let ((print-escape-control-characters t)) + (princ (format "<- %s\n" (prin1-to-string line)) buf) + (princ (format "-> %s\n" (prin1-to-string resp)) buf)) + (process-send-string proc (concat resp))))) + (srv (make-network-process :server 1 + :buffer buf + :filter filt + :name "server" + :family 'ipv4 + :host 'local + :service socks-tests-canned-server-port))) + (set-process-query-on-exit-flag srv nil) + (princ (format "[%s] Listening on localhost:10080\n" srv) buf) + srv)) + +;; Add ([5 3 0 1 2] . [5 2]) to the `verbatim' list below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com +;; +;; If later implementing version 4a, try these: +;; [4 1 0 80 0 0 0 1 0 ?e ?x ?a ?m ?p ?l ?e ?. ?c ?o ?m 0] . [0 90 0 0 0 0 0 0] +;; $ curl --verbose --proxy socks4a://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-auth-filter-url-http () + "Verify correct handling of SOCKS5 user/pass authentication." + (let* ((socks-server '("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")) + (verbatim '(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 3 ?b ?a ?r] . [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]))) + (patterns + `(("^GET /" . ,(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")))) + (socks-tests-canned-server-port 10080) + (server (socks-tests-canned-server-create verbatim patterns)) + (tries 10) + ;; + done + ;; + (cb (lambda (&rest _r) + (goto-char (point-min)) + (should (search-forward "Hello World" nil t)) + (setq done t))) + (buf (url-http url cb '(nil)))) + (ert-info ("Connect to HTTP endpoint over SOCKS5 with USER/PASS method") + (while (and (not done) (< 0 (cl-decf tries))) ; cl-lib via url-http + (sleep-for 0.1))) + (should done) + (delete-process server) + (kill-buffer (process-buffer server)) + (kill-buffer buf) + (ignore url-gateway-method))) + +;;; socks-tests.el ends here -- 2.29.2
"J.P." <jp@neverwas.me> writes: > Just got my paperwork sorted, yay! (Famous last words.) Great timing. :-) > I've attempted to rework the test as suggested. It now runs in process > instead of spawning another Emacs. I wasn't sure whether having url-http > as a dependency made sense but left it as is. Happy to change whatever > needs changing. Looks good to me; pushed to Emacs 28. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no