* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 @ 2021-02-06 11:46 J.P. 2021-02-06 12:26 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-06 11:46 UTC (permalink / raw) To: 46342 [-- Attachment #1: Type: text/plain, Size: 5186 bytes --] Tags: patch Hi, The behavior in question only affects SOCKS version 4 [1] and is shown in the second attachment. The first revises some helpers and adds additional tests I was too lazy or thoughtless to include in recent patches. The third contains a naive attempt at a fix but may suffer from my inadequate grasp of Emacs coding systems [2]. Please advise if that's so and/or I need to hit the books (or get my head examined). Thanks, J.P. [1] The function socks--open-network-stream assumes all v5 addresses to be of type 3, which means a domain name of variable length, presumably of the LDH character set: https://tools.ietf.org/html/rfc1928#section-5 [2] Re coding-systems and proposed fix: I'm wondering if I should have used encode-coding-string (or something else) instead. Also: while socks--open-network-stream is perhaps the real culprit because it creates the offending string, I figured it's more resilient to have the function doing the sending to massage any IP address it encounters. In GNU Emacs 28.0.50 (build 1, x86_64-redhat-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0) of 2021-01-31 built on localhost Repository revision: 431b098a206d27a2dff6a88312c28c36926f90e9 Repository branch: master Windowing system distributor 'Fedora Project', version 11.0.12010000 System Description: Fedora 33 (Workstation Edition) Configured using: 'configure --enable-check-lisp-object-type --enable-checking=yes,glyphs --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --program-prefix= --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info --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 build_alias=x86_64-redhat-linux-gnu host_alias=x86_64-redhat-linux-gnu CC=gcc 'CFLAGS=-DMAIL_USE_LOCKF -O0 -flto=auto -ffat-lto-objects -fexceptions -g3 -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection' LDFLAGS=-Wl,-z,relro PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig' Configured features: ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM XWIDGETS GTK3 ZLIB 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 51852 8897) (symbols 48 6598 1) (strings 32 18255 2081) (string-bytes 1 611217) (vectors 16 12543) (vector-slots 8 172252 11638) (floats 8 25 39) (intervals 56 275 0) (buffers 984 10)) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-more-auth-related-tests-for-socks.el.patch --] [-- Type: text/x-patch, Size: 15775 bytes --] From ef931d35a3ed345750dbf2b2553026266c81cea4 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:24:55 -0800 Subject: [PATCH 1/2] Add more auth-related tests for socks.el * test/lisp/net/socks-tests.el (auth-registration-and-suite-offer) (filter-response-parsing-v4, filter-response-parsing-v5): Assert auth-method selection wrangling and socks-filter parsing. (v5-auth-user-pass, v5-auth-user-pass-blank, v5-auth-none): Show prep and execution of the SOCKS connect command and proxying of an HTTP request; simplify fake server. --- test/lisp/net/socks-tests.el | 263 +++++++++++++++++++++++++++-------- 1 file changed, 208 insertions(+), 55 deletions(-) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index b378ed2964e..d2c21094e7f 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -21,68 +21,149 @@ ;;; Code: +(require 'ert) (require 'socks) (require 'url-http) -(defvar socks-tests-canned-server-port nil) +(ert-deftest socks-tests-auth-registration-and-suite-offer () + (ert-info ("Default favors user/pass auth") + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) ; length [offer ...] + (let (socks-authentication-methods) + (ert-info ("Empty selection/no methods offered") + (should (equal "\0" (socks-build-auth-list)))) + (ert-info ("Simulate library defaults") + (socks-register-authentication-method 0 "No authentication" + 'identity) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-register-authentication-method 2 "Username/Password" + 'socks-username/password-auth) + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) + (ert-info ("Removal") + (socks-unregister-authentication-method 2) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-unregister-authentication-method 0) + (should-not socks-authentication-methods) + (should (equal "\0" (socks-build-auth-list)))))) -(defun socks-tests-canned-server-create (verbatim patterns) - "Create a fake SOCKS server and return the process. +(ert-deftest socks-tests-filter-response-parsing-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") + (socks-filter proc (concat [0 90 0 0 93 184 216])) + ;; From example.com: OK status ^ ^ msg start + (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= (concat [0 90 0 0 93 184 216]) + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch and response fields hold stash w. last chunk") + (should (string= (concat [0 90 0 0 93 184 216 34]) + (process-get proc 'socks-response))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) -`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*")) +(ert-deftest socks-tests-filter-response-parsing-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") + ;; From fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + ;; 5004 ~~> Version Status (OK) NOOP Addr-Type (4 -> IPv6) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") ; unibyte + (ert-info ("State still waiting and response emtpy") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (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 ("Middle chunk arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State and response fields still untouched") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival appended (on RHS)") + (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 and response fields show last chunk 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))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) + +(defvar socks-tests-canned-server-patterns nil + "Alist containing request/response cons pairs to be tried in order. +Vectors must match verbatim. Strings are considered regex patterns.") + +(defun socks-tests-canned-server-create () + "Create and return a fake SOCKS server." + (let* ((port (nth 2 socks-server)) + (name (format "socks-server:%d" port)) + (pats socks-tests-canned-server-patterns) (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 + (pcase-let ((`(,pat . ,resp) (pop pats))) + (unless (or (and (vectorp pat) (equal pat (vconcat line))) + (string-match-p pat line)) (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)) + (message "[%s] <- %s" name (prin1-to-string line)) + (message "[%s] -> %s" name (prin1-to-string resp))) (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 + (serv (make-network-process :server 1 + :buffer (get-buffer-create name) + :filter filt + :name name + :family 'ipv4 + :host 'local + :coding 'binary + :service port))) + (set-process-query-on-exit-flag serv nil) + serv)) -(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) +(defvar socks-tests--hello-world-http-request-pattern + (cons "^GET /" (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n"))) + +(defun socks-tests-perform-hello-world-http-request () + "Start canned server, validate hello-world response, and finalize." + (let* ((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) + (server (socks-tests-canned-server-create)) ;; done ;; @@ -90,14 +171,86 @@ socks-tests-auth-filter-url-http (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))) + (buf (url-http url cb '(nil))) + (proc (get-buffer-process buf)) + (attempts 10)) + (while (and (not done) (< 0 (cl-decf attempts))) + (sleep-for 0.1)) (should done) (delete-process server) + (delete-process proc) ; otherwise seems client proc is sometimes reused (kill-buffer (process-buffer server)) (kill-buffer buf) (ignore url-gateway-method))) +;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-v5-auth-user-pass () + "Verify correct handling of SOCKS5 user/pass authentication." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-user-agent "Test/auth-user-pass") + (socks-tests-canned-server-patterns + `(([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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Services (like Tor) may be configured without auth but for some +;; reason still prefer the user/pass method over none when offered both. +;; Given this library's defaults, the scenario below is possible. +;; +;; FYI: RFC 1929 doesn't say that a username or password is required +;; but notes that the length of both fields should be at least one. +;; However, both socks.el and curl send zero-length fields (though +;; curl drops the user part too when the password is empty). +;; +;; To verify against curl 7.71, swap out the first two pattern pairs +;; with ([5 3 0 1 2] . [5 2]) and ([1 0 0] . [1 0]), then run: +;; $ curl verbose -U "foo:" --proxy socks5h://127.0.0.1:10081 example.com + +(ert-deftest socks-tests-v5-auth-user-pass-blank () + "Verify correct SOCKS5 user/pass authentication with empty pass." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10081 5)) + (socks-username "foo") ; defaults to (user-login-name) + (socks-password "") ; simulate user hitting enter when prompted + (url-user-agent "Test/auth-user-pass-blank") + (socks-tests-canned-server-patterns + `(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 0] . [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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Swap out ([5 2 0 1] . [5 0]) with the first pattern below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com + +(ert-deftest socks-tests-v5-auth-none () + "Verify correct handling of SOCKS5 when auth method 0 requested." + (let ((socks-server '("server" "127.0.0.1" 10082 5)) + (socks-authentication-methods (append socks-authentication-methods + nil)) + (url-user-agent "Test/auth-none") + (socks-tests-canned-server-patterns + `(([5 1 0] . [5 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]) + ,socks-tests--hello-world-http-request-pattern))) + (socks-unregister-authentication-method 2) + (should-not (assq 2 socks-authentication-methods)) + (ert-info ("Make HTTP request over SOCKS5 with no auth method") + (socks-tests-perform-hello-world-http-request))) + (should (assq 2 socks-authentication-methods))) + ;;; socks-tests.el ends here -- 2.29.2 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-DROP-ME-demo-UTF-8-encoding-of-numeric-addresses.patch --] [-- Type: text/x-patch, Size: 1922 bytes --] From 512cfb3b79311edbb503e4fafaa92211879899fe Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:29:22 -0800 Subject: [PATCH 2/2] DROP ME: demo UTF-8 encoding of numeric addresses --- test/lisp/net/socks-tests.el | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index d2c21094e7f..31857593dea 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -183,6 +183,29 @@ socks-tests-perform-hello-world-http-request (kill-buffer buf) (ignore url-gateway-method))) +;; XXX replace me: temporary demo showing current (flawed) behavior +;; +;; Numeric addresses with members in the Latin 1 range (127-255) get +;; encoded as their corresponding UTF-8 code points. + +(ert-deftest socks-tests-v4-bug-demo () + "Demo incorrect preparation of SOCKS4 connect command." + (let ((socks-server '("server" "127.0.0.1" 10078 4)) + (url-user-agent "Test/4-bug-demo") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 194 184 195 152 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ;; ^~~~~~~~^~~~~~~ multi-byte ^ actually 91 err + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + ;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate ;; against curl 7.71 with the following options: ;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com -- 2.29.2 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #4: 0002-Preserve-numeric-addresses-in-socks-send-command.patch --] [-- Type: text/x-patch, Size: 2352 bytes --] From d9401d76976eb8fc0e88d880bb1de3e095098742 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 19:41:04 -0800 Subject: [PATCH 2/2] Preserve numeric addresses in socks-send-command * lisp/net/socks.el: (socks-send-command): * test/lisp/net/socks-tests.el: (socks-tests-v4-basic): --- lisp/net/socks.el | 2 +- test/lisp/net/socks-tests.el | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 96fafc826b8..be1b8b80ff5 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -393,7 +393,7 @@ socks-send-command (let ((addr (cond ((or (= atype socks-address-type-v4) (= atype socks-address-type-v6)) - address) + (apply #'unibyte-string (append address nil))) ((= atype socks-address-type-name) (format "%c%s" (length address) address)) (t diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index d2c21094e7f..7112f42a67c 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -183,6 +183,26 @@ socks-tests-perform-hello-world-http-request (kill-buffer buf) (ignore url-gateway-method))) +;; Unlike curl, socks.el includes the ID field (but otherwise matches): +;; $ curl --proxy socks4://127.0.0.1:1080 example.com + +(ert-deftest socks-tests-v4-basic () + "Show correct preparation of SOCKS4 connect command." + (let ((socks-server '("server" "127.0.0.1" 10079 4)) + (url-user-agent "Test/4-basic") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 184 216 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + ;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate ;; against curl 7.71 with the following options: ;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com -- 2.29.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-06 11:46 bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 J.P. @ 2021-02-06 12:26 ` Eli Zaretskii 2021-02-06 14:19 ` J.P. 0 siblings, 1 reply; 21+ messages in thread From: Eli Zaretskii @ 2021-02-06 12:26 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Date: Sat, 06 Feb 2021 03:46:26 -0800 > > [2] Re coding-systems and proposed fix: I'm wondering if I should have > used encode-coding-string (or something else) instead. Also: while > socks--open-network-stream is perhaps the real culprit because it > creates the offending string, I figured it's more resilient to have the > function doing the sending to massage any IP address it encounters. Emacs holds all non-ASCII characters internally in (a superset of) UTF-8 encoding. When passing those strings to external programs or network connections, they should be encoded as appropriate. What I don't understand is what is the "appropriate" encoding in this case. Can you explain why you use literal bytes in the test? What are those bytes supposed to stand for, and which program is supposed to receive this sequence of bytes on the other end of the connect command? Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-06 12:26 ` Eli Zaretskii @ 2021-02-06 14:19 ` J.P. 2021-02-06 15:15 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-06 14:19 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 Hi Eli. Thanks for the quick reply. Eli Zaretskii <eliz@gnu.org> writes: > What I don't understand is what is the "appropriate" encoding in this > case. Can you explain why you use literal bytes in the test? What > are those bytes supposed to stand for, and which program is supposed > to receive this sequence of bytes on the other end of the connect > command? Re appropriate encoding: correct me if I'm wrong (internet), but among the Emacs coding systems, it'd be latin-1. In the proposed patch for socks-send-command, swapping out the call to unibyte-string with (encode-coding-string address 'latin-1) has the same effect of preserving, say, char 216 as the single byte "\330" and not "\303\230". Re literal bytes in test(s): I'm assuming you mean all the vectors. If that's annoying or distracting, my apologies. I used them for no other reason than I thought them easier to read. I also found them less error prone than octal or hex escapes in strings, as warned about in (info "(elisp) Non-ASCII in Strings"). However, I'm happy to follow whatever the convention is. Re meaning of the bytes: they stand for the fields in SOCKS 4 messages. The protocol is byte oriented, so it's easy enough to read with tcpdump since there's no meticulous packing going on when serializing, just verbatim octets. Not the most economical, but after a brief negotiation, it's off to proxying. For example, you'd read 4 1 0 80 93 184 216 34 as version 4, command 1 (connect), port 80 (2 bytes, big endian to 65535), IPv4 address 93.184.216.34 Re program on the other end: this would be any program offering a proxy service that speaks the same protocol. Popular ones include tor and ssh. There's also a "bind" command that allows your client (Emacs) to act as a server and listen/accept connections on the remote end as if they were present on your local network. I'm no expert, but I'll do my best to answer any further questions. Thanks so much and enjoy your weekend! ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-06 14:19 ` J.P. @ 2021-02-06 15:15 ` Eli Zaretskii 2021-02-07 14:22 ` J.P. 2021-02-09 15:17 ` J.P. 0 siblings, 2 replies; 21+ messages in thread From: Eli Zaretskii @ 2021-02-06 15:15 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Sat, 06 Feb 2021 06:19:55 -0800 > > Re appropriate encoding: correct me if I'm wrong (internet), but among > the Emacs coding systems, it'd be latin-1. That depends on what the other end expects. Does it expect latin-1 in this case? > In the proposed patch for socks-send-command, swapping out the call > to unibyte-string with (encode-coding-string address 'latin-1) has > the same effect of preserving, say, char 216 as the single byte > "\330" and not "\303\230". Does emitting the single byte \330 produce the correct result in this case? Then by all means please use (encode-coding-string address 'latin-1) > Re program on the other end: this would be any program offering a proxy > service that speaks the same protocol. Popular ones include tor and ssh. > There's also a "bind" command that allows your client (Emacs) to act as > a server and listen/accept connections on the remote end as if they were > present on your local network. And those expect Latin-1 encoding in this case? ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-06 15:15 ` Eli Zaretskii @ 2021-02-07 14:22 ` J.P. 2021-02-09 15:17 ` J.P. 1 sibling, 0 replies; 21+ messages in thread From: J.P. @ 2021-02-07 14:22 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 [-- Attachment #1: Type: text/plain, Size: 3005 bytes --] Eli Zaretskii <eliz@gnu.org> writes: >> Re appropriate encoding: correct me if I'm wrong (internet), but among >> the Emacs coding systems, it'd be latin-1. > > That depends on what the other end expects. Does it expect latin-1 in > this case? From the point of view of Emacs, I'd say yes: the other end, meaning the proxy service, expects latin-1. From the service's point of view, it only speaks byte sequences and doesn't interpret any fields as text [1]. This continues after proxying has commenced; incoming byte sequences are forwarded verbatim as opaque payloads. > Does emitting the single byte \330 produce the correct result in this > case? Then by all means please use > > (encode-coding-string address 'latin-1) It does indeed produce the correct result [2], and I've updated the patch to reflect this. I wasn't sure whether you wanted me to replace all the vectors in the tests with strings and/or annotate them with comments explaining the protocol, so I just left them as is for now. My main concern (based on sheer ignorance) was any possible side effects that may occur from encode-coding-string setting the variable last-coding-system-used to latin-1. I investigated a little by stepping through the subsequent send_process() call and found that the variable's value as latin-1 appears short lived because it's quickly reassigned to binary. I tried to demonstrate this in the attached log of my debug session (and also show that no conversion is performed). Please pardon my sad debugging skills. >> Re program on the other end: this would be any program offering a proxy >> service that speaks the same protocol. Popular ones include tor and ssh. >> [...] > > And those expect Latin-1 encoding in this case? I'd say yes, insofar as these programs are examples of a proxy service of the sort mentioned in the first answer above. Thanks again [1] Although, in the case of SOCKS 4A/5, non-numeric addresses, i.e., domain names, should probably be expressed in characters a resolver can understand, like the Punycode ASCII subset. [2] there is one tiny difference in behavior from the previous iteration of this patch, but it's not worth anyone's time, so I'll just note it here for the record: when called in the manner shown in the patch, encode-coding-string silently replaces multibyte characters with spaces. The only edge case I could think of in which accidentally passing a multibyte might be harder to debug than a normal typo would be when hitting an address like ec2-13-56-13-123.us-west-1.compute.amazonaws.com and accidentally passing 13.256.13.123 (as "\15\u0100\15\173"), which would be routed to 13.32.13.123 (flickr/cloudflare). One way to avoid this would be with validation like that performed by unibyte-string or, alternatively, by purposefully violating the protocol and sending say, "\15\15{" instead of "\15 \15{" (and thereby triggering an error response from the service). All in all, this seems unlikely enough not to warrant special attention. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-more-auth-related-tests-for-socks.el.patch --] [-- Type: text/x-patch, Size: 15775 bytes --] From ef931d35a3ed345750dbf2b2553026266c81cea4 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:24:55 -0800 Subject: [PATCH 1/2] Add more auth-related tests for socks.el * test/lisp/net/socks-tests.el (auth-registration-and-suite-offer) (filter-response-parsing-v4, filter-response-parsing-v5): Assert auth-method selection wrangling and socks-filter parsing. (v5-auth-user-pass, v5-auth-user-pass-blank, v5-auth-none): Show prep and execution of the SOCKS connect command and proxying of an HTTP request; simplify fake server. --- test/lisp/net/socks-tests.el | 263 +++++++++++++++++++++++++++-------- 1 file changed, 208 insertions(+), 55 deletions(-) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index b378ed2964e..d2c21094e7f 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -21,68 +21,149 @@ ;;; Code: +(require 'ert) (require 'socks) (require 'url-http) -(defvar socks-tests-canned-server-port nil) +(ert-deftest socks-tests-auth-registration-and-suite-offer () + (ert-info ("Default favors user/pass auth") + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) ; length [offer ...] + (let (socks-authentication-methods) + (ert-info ("Empty selection/no methods offered") + (should (equal "\0" (socks-build-auth-list)))) + (ert-info ("Simulate library defaults") + (socks-register-authentication-method 0 "No authentication" + 'identity) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-register-authentication-method 2 "Username/Password" + 'socks-username/password-auth) + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) + (ert-info ("Removal") + (socks-unregister-authentication-method 2) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-unregister-authentication-method 0) + (should-not socks-authentication-methods) + (should (equal "\0" (socks-build-auth-list)))))) -(defun socks-tests-canned-server-create (verbatim patterns) - "Create a fake SOCKS server and return the process. +(ert-deftest socks-tests-filter-response-parsing-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") + (socks-filter proc (concat [0 90 0 0 93 184 216])) + ;; From example.com: OK status ^ ^ msg start + (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= (concat [0 90 0 0 93 184 216]) + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch and response fields hold stash w. last chunk") + (should (string= (concat [0 90 0 0 93 184 216 34]) + (process-get proc 'socks-response))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) -`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*")) +(ert-deftest socks-tests-filter-response-parsing-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") + ;; From fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + ;; 5004 ~~> Version Status (OK) NOOP Addr-Type (4 -> IPv6) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") ; unibyte + (ert-info ("State still waiting and response emtpy") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (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 ("Middle chunk arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State and response fields still untouched") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival appended (on RHS)") + (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 and response fields show last chunk 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))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) + +(defvar socks-tests-canned-server-patterns nil + "Alist containing request/response cons pairs to be tried in order. +Vectors must match verbatim. Strings are considered regex patterns.") + +(defun socks-tests-canned-server-create () + "Create and return a fake SOCKS server." + (let* ((port (nth 2 socks-server)) + (name (format "socks-server:%d" port)) + (pats socks-tests-canned-server-patterns) (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 + (pcase-let ((`(,pat . ,resp) (pop pats))) + (unless (or (and (vectorp pat) (equal pat (vconcat line))) + (string-match-p pat line)) (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)) + (message "[%s] <- %s" name (prin1-to-string line)) + (message "[%s] -> %s" name (prin1-to-string resp))) (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 + (serv (make-network-process :server 1 + :buffer (get-buffer-create name) + :filter filt + :name name + :family 'ipv4 + :host 'local + :coding 'binary + :service port))) + (set-process-query-on-exit-flag serv nil) + serv)) -(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) +(defvar socks-tests--hello-world-http-request-pattern + (cons "^GET /" (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n"))) + +(defun socks-tests-perform-hello-world-http-request () + "Start canned server, validate hello-world response, and finalize." + (let* ((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) + (server (socks-tests-canned-server-create)) ;; done ;; @@ -90,14 +171,86 @@ socks-tests-auth-filter-url-http (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))) + (buf (url-http url cb '(nil))) + (proc (get-buffer-process buf)) + (attempts 10)) + (while (and (not done) (< 0 (cl-decf attempts))) + (sleep-for 0.1)) (should done) (delete-process server) + (delete-process proc) ; otherwise seems client proc is sometimes reused (kill-buffer (process-buffer server)) (kill-buffer buf) (ignore url-gateway-method))) +;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-v5-auth-user-pass () + "Verify correct handling of SOCKS5 user/pass authentication." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-user-agent "Test/auth-user-pass") + (socks-tests-canned-server-patterns + `(([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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Services (like Tor) may be configured without auth but for some +;; reason still prefer the user/pass method over none when offered both. +;; Given this library's defaults, the scenario below is possible. +;; +;; FYI: RFC 1929 doesn't say that a username or password is required +;; but notes that the length of both fields should be at least one. +;; However, both socks.el and curl send zero-length fields (though +;; curl drops the user part too when the password is empty). +;; +;; To verify against curl 7.71, swap out the first two pattern pairs +;; with ([5 3 0 1 2] . [5 2]) and ([1 0 0] . [1 0]), then run: +;; $ curl verbose -U "foo:" --proxy socks5h://127.0.0.1:10081 example.com + +(ert-deftest socks-tests-v5-auth-user-pass-blank () + "Verify correct SOCKS5 user/pass authentication with empty pass." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10081 5)) + (socks-username "foo") ; defaults to (user-login-name) + (socks-password "") ; simulate user hitting enter when prompted + (url-user-agent "Test/auth-user-pass-blank") + (socks-tests-canned-server-patterns + `(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 0] . [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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Swap out ([5 2 0 1] . [5 0]) with the first pattern below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com + +(ert-deftest socks-tests-v5-auth-none () + "Verify correct handling of SOCKS5 when auth method 0 requested." + (let ((socks-server '("server" "127.0.0.1" 10082 5)) + (socks-authentication-methods (append socks-authentication-methods + nil)) + (url-user-agent "Test/auth-none") + (socks-tests-canned-server-patterns + `(([5 1 0] . [5 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]) + ,socks-tests--hello-world-http-request-pattern))) + (socks-unregister-authentication-method 2) + (should-not (assq 2 socks-authentication-methods)) + (ert-info ("Make HTTP request over SOCKS5 with no auth method") + (socks-tests-perform-hello-world-http-request))) + (should (assq 2 socks-authentication-methods))) + ;;; socks-tests.el ends here -- 2.29.2 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-Preserve-numeric-addresses-in-socks-send-command.patch --] [-- Type: text/x-patch, Size: 2346 bytes --] From 96018fe24dbab8c1f0dbf31c120a088f1998519f Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 19:41:04 -0800 Subject: [PATCH 2/2] Preserve numeric addresses in socks-send-command * lisp/net/socks.el: (socks-send-command): * test/lisp/net/socks-tests.el: (socks-tests-v4-basic): --- lisp/net/socks.el | 2 +- test/lisp/net/socks-tests.el | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 96fafc826b8..019fadd1440 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -393,7 +393,7 @@ socks-send-command (let ((addr (cond ((or (= atype socks-address-type-v4) (= atype socks-address-type-v6)) - address) + (encode-coding-string address 'latin-1)) ((= atype socks-address-type-name) (format "%c%s" (length address) address)) (t diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index d2c21094e7f..7112f42a67c 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -183,6 +183,26 @@ socks-tests-perform-hello-world-http-request (kill-buffer buf) (ignore url-gateway-method))) +;; Unlike curl, socks.el includes the ID field (but otherwise matches): +;; $ curl --proxy socks4://127.0.0.1:1080 example.com + +(ert-deftest socks-tests-v4-basic () + "Show correct preparation of SOCKS4 connect command." + (let ((socks-server '("server" "127.0.0.1" 10079 4)) + (url-user-agent "Test/4-basic") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 184 216 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + ;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate ;; against curl 7.71 with the following options: ;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com -- 2.29.2 [-- Attachment #4: debug_session.log --] [-- Type: text/plain, Size: 11758 bytes --] // session 2 HEAD is 06e1e5eeacf67b11490431c3d36700a73cf49d88 Dmitry Gutov <dgutov@yandex.ru> Revert "Fix the previous change" // note: I broke a few long lines Current directory: /home/jp/emacs/test/ Command: gdb set annotate 1 set filename-display absolute GNU gdb (GDB) Fedora 10.1-2.fc33 Copyright (C) 2020 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-redhat-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <https://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word". .gdbinit:19: Error in sourced command file: No symbol table is loaded. Use the "file" command. gdb$ file ../src/emacs Reading symbols from ../src/emacs... gdb$ source .gdbinit DISPLAY = :1 TERM = xterm-256color Breakpoint 1 at 0x420251: file /home/jp/emacs/src/emacs.c, line 379. Breakpoint 2 at 0x4d9080: file /home/jp/emacs/src/xterm.c, line 10181. gdb$ break /home/jp/emacs/src/process.c:6357 Breakpoint 3 at 0x5c9d5f: file /home/jp/emacs/src/process.c, line 6359. gdb$ set args --module-assertions --no-init-file --no-site-file --no-site-lisp \ -L :. -l ert -l lisp/net/socks-tests.el --batch \ --eval \(ert-run-tests-batch-and-exit\ \(quote\ socks-tests-v4-basic\)\) gdb$ run Starting program: /home/jp/emacs/src/emacs \ --module-assertions --no-init-file --no-site-file --no-site-lisp \ -L :. -l ert -l lisp/net/socks-tests.el --batch \ --eval \(ert-run-tests-batch-and-exit\ \(quote\ socks-tests-v4-basic\)\) Missing separate debuginfos, use: dnf debuginfo-install glibc-2.32-4.fc33.x86_64 [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". [New Thread 0x7fffe63e1640 (LWP 386228)] Running 1 tests (2021-02-07 05:12:37-0800, selector ‘socks-tests-v4-basic’) Contacting host: example.com:80 Thread 1 "emacs" hit Breakpoint 3, send_process (proc=XIL(0xc18ed5), buf=0xc17f20 "\004\001", len=12, object=XIL(0xdb1964)) at /home/jp/emacs/src/process.c:6359 /home/jp/emacs/src/process.c:6359:187302:beg:0x5c9d5f gdb$ p *p $1 = { header = { size = 4611686018561691669 }, tty_name = XIL(0), name = XIL(0xb80044), command = XIL(0), filter = XIL(0x14d410), sentinel = XIL(0x83d0), log = XIL(0), buffer = XIL(0), childp = XIL(0xdad983), plist = XIL(0xdada43), type = XIL(0xa4d0), mark = XIL(0xce7ee5), status = XIL(0xbf70), decode_coding_system = XIL(0x7ffff3bb1388), decoding_buf = XIL(0x7ffff422f424), encode_coding_system = XIL(0x7ffff3bb1388), encoding_buf = XIL(0x7ffff422f424), write_queue = XIL(0), gnutls_cred_type = XIL(0), gnutls_boot_parameters = XIL(0), stderrproc = XIL(0), thread = XIL(0x9bc6c5), --Type <RET> for more, q to quit, c to continue without paging--c pid = 0, infd = 7, nbytes_read = 0, outfd = 7, open_fd = {7, -1, -1, -1, -1, -1}, tick = 0, update_tick = 0, decoding_carryover = 0, read_output_delay = 0, adaptive_read_buffering = 0, read_output_skip = false, kill_without_query = true, pty_flag = false, inherit_coding_system_flag = false, alive = false, raw_status_new = false, is_non_blocking_client = false, is_server = false, raw_status = 0, backlog = 5, port = 0, socktype = 1, dns_request = 0x0, gnutls_initstage = GNUTLS_STAGE_EMPTY, gnutls_state = 0x0, gnutls_x509_cred = 0x0, gnutls_anon_cred = 0x0, gnutls_certificates = 0x0, gnutls_certificates_length = 0, gnutls_peer_verification = 0, gnutls_extra_peer_verification = 0, gnutls_log_level = 0, gnutls_handshakes_tried = 0, gnutls_p = false, gnutls_complete_negotiation_p = false } gdb$ p p->decode_coding_system $2 = XIL(0x7ffff3bb1388) gdb$ pr binary gdb$ p p->encode_coding_system $3 = XIL(0x7ffff3bb1388) gdb$ pr binary gdb$ print Vlast_coding_system_used $4 = XIL(0x8df0) gdb$ pr latin-1 gdb$ n /home/jp/emacs/src/process.c:6360:187338:beg:0x5c9d64 gdb$ /home/jp/emacs/src/process.c:6363:187384:beg:0x5c976c gdb$ /home/jp/emacs/src/lisp.h:718:25723:beg:0x5c977e gdb$ /home/jp/emacs/src/process.c:6367:187517:beg:0x5c978a gdb$ /home/jp/emacs/src/process.c:6371:187644:beg:0x5c9799 gdb$ /home/jp/emacs/src/process.c:6372:187692:beg:0x5c97a1 gdb$ p *coding $5 = { id = 5, common_flags = 256, mode = 0, src_multibyte = true, dst_multibyte = true, chars_at_source = false, raw_destination = false, annotated = true, eol_seen = 5, result = 6, max_charset_id = 0, spec = { iso_2022 = { flags = 14359680, current_invocation = {0, 14359680}, current_designation = {0, 0, 0, 0}, ctext_extended_segment_len = 0, single_shifting = false, bol = false, embedded_utf_8 = false, cmp_status = { state = COMPOSING_NO, method = COMPOSITION_RELATIVE, old_form = false, --Type <RET> for more, q to quit, c to continue without paging--c length = 0, nchars = 0, ncomps = 0, carryover = {0 <repeats 68 times>} } }, ccl = 0xdb1c80, utf_16 = { bom = (unknown: 0xdb1c80), endian = utf_16_big_endian, surrogate = 14359680 }, utf_8_bom = (unknown: 0xdb1c80), emacs_mule = { cmp_status = { state = 14359680, method = COMPOSITION_RELATIVE, old_form = 128, length = 0, nchars = 0, ncomps = 0, carryover = {0 <repeats 68 times>} } }, undecided = { inhibit_nbd = 14359680, inhibit_ied = 0, prefer_utf_8 = 128 } }, safe_charsets = 0x7ffff49f7257 "\377", head_ascii = 0, detected_utf8_bytes = 0, detected_utf8_chars = 0, produced = 0, produced_char = 0, consumed = 0, consumed_char = 0, src_pos = 0, src_pos_byte = 0, src_chars = 0, src_bytes = 0, src_object = XIL(0), source = 0x0, dst_pos = 0, dst_pos_byte = 0, dst_bytes = 0, dst_object = XIL(0), destination = 0x0, charbuf = 0x0, charbuf_size = 0, charbuf_used = 0, carryover = '\000' <repeats 63 times>, carryover_bytes = 0, default_char = 0, detector = 0x0, decoder = 0x499df0 <decode_coding_raw_text>, encoder = 0x49efe0 <encode_coding_raw_text> } gdb$ l 6367 if (p->outfd < 0) 6368 error ("Output file descriptor of %s is closed", SDATA (p->name)); 6369 6370 eassert (p->outfd < FD_SETSIZE); 6371 coding = proc_encode_coding_system[p->outfd]; 6372 Vlast_coding_system_used = CODING_ID_NAME (coding->id); 6373 6374 if ((STRINGP (object) && STRING_MULTIBYTE (object)) 6375 || (BUFFERP (object) 6376 && !NILP (BVAR (XBUFFER (object), enable_multibyte_characters))) gdb$ fr Stack level 0, frame at 0x7fffffff9080: rip = 0x5c97a1 in send_process (/home/jp/emacs/src/process.c:6372); saved rip = 0x5c9f74 called by frame at 0x7fffffff9090 source language c. Arglist at 0x7fffffff9008, args: proc=XIL(0xc18ed5), buf=0xc17f20 "\004\001", len=12, object=XIL(0xdb1964) Locals at 0x7fffffff9008, Previous frame's sp is 0x7fffffff9080 Saved registers: rbx at 0x7fffffff9048, rbp at 0x7fffffff9050, r12 at 0x7fffffff9058, r13 at 0x7fffffff9060, r14 at 0x7fffffff9068, r15 at 0x7fffffff9070, rip at 0x7fffffff9078 proc = XIL(0xc18ed5) buf = 0xc17f20 "\004\001" len = 12 object = XIL(0xdb1964) p = 0xc18ed0 rv = <optimized out> coding = 0xdb1c90 gdb$ n /home/jp/emacs/src/process.c:6374:187751:beg:0x5c97b7 gdb$ /home/jp/emacs/src/lisp.h:718:25723:beg:0x5c97c4 gdb$ /home/jp/emacs/src/lisp.h:730:25858:beg:0x5c97ce gdb$ /home/jp/emacs/src/process.c:6375:187805:beg:0x5c9b1a gdb$ /home/jp/emacs/src/lisp.h:718:25723:beg:0x5c9b27 gdb$ /home/jp/emacs/src/process.c:6398:188683:beg:0x5c9b31 gdb$ /home/jp/emacs/src/process.c:6402:188909:beg:0x5c9b41 gdb$ /home/jp/emacs/src/process.c:6417:189358:beg:0x5c9800 gdb$ /home/jp/emacs/src/process.c:6419:189388:beg:0x5c9808 gdb$ p *coding $6 = { id = 5, common_flags = 256, mode = 0, src_multibyte = false, dst_multibyte = false, chars_at_source = false, raw_destination = false, annotated = true, eol_seen = 5, result = 6, max_charset_id = 0, spec = { iso_2022 = { flags = 14359680, current_invocation = {0, 14359680}, current_designation = {0, 0, 0, 0}, ctext_extended_segment_len = 0, single_shifting = false, bol = false, embedded_utf_8 = false, cmp_status = { state = COMPOSING_NO, method = COMPOSITION_RELATIVE, old_form = false, --Type <RET> for more, q to quit, c to continue without paging--c length = 0, nchars = 0, ncomps = 0, carryover = {0 <repeats 68 times>} } }, ccl = 0xdb1c80, utf_16 = { bom = (unknown: 0xdb1c80), endian = utf_16_big_endian, surrogate = 14359680 }, utf_8_bom = (unknown: 0xdb1c80), emacs_mule = { cmp_status = { state = 14359680, method = COMPOSITION_RELATIVE, old_form = 128, length = 0, nchars = 0, ncomps = 0, carryover = {0 <repeats 68 times>} } }, undecided = { inhibit_nbd = 14359680, inhibit_ied = 0, prefer_utf_8 = 128 } }, safe_charsets = 0x7ffff49f7257 "\377", head_ascii = 0, detected_utf8_bytes = 0, detected_utf8_chars = 0, produced = 0, produced_char = 0, consumed = 0, consumed_char = 0, src_pos = 0, src_pos_byte = 0, src_chars = 0, src_bytes = 0, src_object = XIL(0), source = 0x0, dst_pos = 0, dst_pos_byte = 0, dst_bytes = 0, dst_object = XIL(0), destination = 0x0, charbuf = 0x0, charbuf_size = 0, charbuf_used = 0, carryover = '\000' <repeats 63 times>, carryover_bytes = 0, default_char = 0, detector = 0x0, decoder = 0x499df0 <decode_coding_raw_text>, encoder = 0x49efe0 <encode_coding_raw_text> } gdb$ print Vlast_coding_system_used $7 = XIL(0x7ffff3bb1388) gdb$ pr binary gdb$ p object $8 = XIL(0xdb1964) gdb$ pr "^D^A^@P]\270\330\"foo^@" gdb$ xstring object $9 = (struct Lisp_String *) 0xdb1960 "\004\001\000P]\270\330\"foo" gdb$ p *$9 $10 = { u = { s = { size = 12, size_byte = -1, intervals = 0x0, data = 0xc17f20 "\004\001" }, next = 0xc, gcaligned = 12 '\f' } } gdb$ del 3 gdb$ c Continuing. [socks-server:10079] <- "\4\1\0P]\270\330\"foo\0" [socks-server:10079] -> [0 90 0 0 0 0 0 0] [socks-server:10079] <- "GET / HTTP/1.1\15\12MIME-Version: 1.0\15\12Connection: keep-alive\15\12Extension: Security/Digest Security/SSL\15\12Host: example.com\15\12Accept-encoding: gzip\15\12Accept: */*\15\12User-Agent: Test/4-basic\15\12\15\12" [socks-server:10079] -> "HTTP/1.1 200 OK\15\12Content-Type: text/plain\15\12Content-Length: 13\15\12\15\12Hello World!\12" passed 1/1 socks-tests-v4-basic (951.587240 sec) Ran 1 tests, 1 results as expected, 0 unexpected (2021-02-07 05:28:28-0800, 951.587586 sec) [Thread 0x7fffe63e1640 (LWP 386228) exited] [Inferior 1 (process 386123) exited normally] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-06 15:15 ` Eli Zaretskii 2021-02-07 14:22 ` J.P. @ 2021-02-09 15:17 ` J.P. 2021-02-09 16:14 ` Eli Zaretskii 1 sibling, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-09 15:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 [-- Attachment #1: Type: text/plain, Size: 727 bytes --] Eli, Apologies for my previous rambling reply. Your time is supremely valuable, and I should've been more mindful of that. I'm hoping you'll allow me this mulligan (ahem): > That depends on what the other end expects. Does it expect latin-1 in > this case? Yes. > Does emitting the single byte \330 produce the correct result in this > case? Yes. > Then by all means please use > > (encode-coding-string address 'latin-1) I have updated my changes to reflect this. > And those [tor/ssh] expect Latin-1 encoding in this case? Yes. When you've got a sec, please instruct me on how best to proceed (even if that means buggering off 'til I've learned more about Emacs!). :) Appreciate your patience, as always. J.P. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-more-auth-related-tests-for-socks.el.patch --] [-- Type: text/x-patch, Size: 15775 bytes --] From df68ca06563b9f4d943666c474d978fe66584485 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:24:55 -0800 Subject: [PATCH 1/2] Add more auth-related tests for socks.el * test/lisp/net/socks-tests.el (auth-registration-and-suite-offer) (filter-response-parsing-v4, filter-response-parsing-v5): Assert auth-method selection wrangling and socks-filter parsing. (v5-auth-user-pass, v5-auth-user-pass-blank, v5-auth-none): Show prep and execution of the SOCKS connect command and proxying of an HTTP request; simplify fake server. --- test/lisp/net/socks-tests.el | 263 +++++++++++++++++++++++++++-------- 1 file changed, 208 insertions(+), 55 deletions(-) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index b378ed2964e..d2c21094e7f 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -21,68 +21,149 @@ ;;; Code: +(require 'ert) (require 'socks) (require 'url-http) -(defvar socks-tests-canned-server-port nil) +(ert-deftest socks-tests-auth-registration-and-suite-offer () + (ert-info ("Default favors user/pass auth") + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) ; length [offer ...] + (let (socks-authentication-methods) + (ert-info ("Empty selection/no methods offered") + (should (equal "\0" (socks-build-auth-list)))) + (ert-info ("Simulate library defaults") + (socks-register-authentication-method 0 "No authentication" + 'identity) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-register-authentication-method 2 "Username/Password" + 'socks-username/password-auth) + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) + (ert-info ("Removal") + (socks-unregister-authentication-method 2) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-unregister-authentication-method 0) + (should-not socks-authentication-methods) + (should (equal "\0" (socks-build-auth-list)))))) -(defun socks-tests-canned-server-create (verbatim patterns) - "Create a fake SOCKS server and return the process. +(ert-deftest socks-tests-filter-response-parsing-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") + (socks-filter proc (concat [0 90 0 0 93 184 216])) + ;; From example.com: OK status ^ ^ msg start + (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= (concat [0 90 0 0 93 184 216]) + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch and response fields hold stash w. last chunk") + (should (string= (concat [0 90 0 0 93 184 216 34]) + (process-get proc 'socks-response))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) -`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*")) +(ert-deftest socks-tests-filter-response-parsing-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") + ;; From fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + ;; 5004 ~~> Version Status (OK) NOOP Addr-Type (4 -> IPv6) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") ; unibyte + (ert-info ("State still waiting and response emtpy") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (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 ("Middle chunk arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State and response fields still untouched") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival appended (on RHS)") + (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 and response fields show last chunk 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))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) + +(defvar socks-tests-canned-server-patterns nil + "Alist containing request/response cons pairs to be tried in order. +Vectors must match verbatim. Strings are considered regex patterns.") + +(defun socks-tests-canned-server-create () + "Create and return a fake SOCKS server." + (let* ((port (nth 2 socks-server)) + (name (format "socks-server:%d" port)) + (pats socks-tests-canned-server-patterns) (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 + (pcase-let ((`(,pat . ,resp) (pop pats))) + (unless (or (and (vectorp pat) (equal pat (vconcat line))) + (string-match-p pat line)) (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)) + (message "[%s] <- %s" name (prin1-to-string line)) + (message "[%s] -> %s" name (prin1-to-string resp))) (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 + (serv (make-network-process :server 1 + :buffer (get-buffer-create name) + :filter filt + :name name + :family 'ipv4 + :host 'local + :coding 'binary + :service port))) + (set-process-query-on-exit-flag serv nil) + serv)) -(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) +(defvar socks-tests--hello-world-http-request-pattern + (cons "^GET /" (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n"))) + +(defun socks-tests-perform-hello-world-http-request () + "Start canned server, validate hello-world response, and finalize." + (let* ((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) + (server (socks-tests-canned-server-create)) ;; done ;; @@ -90,14 +171,86 @@ socks-tests-auth-filter-url-http (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))) + (buf (url-http url cb '(nil))) + (proc (get-buffer-process buf)) + (attempts 10)) + (while (and (not done) (< 0 (cl-decf attempts))) + (sleep-for 0.1)) (should done) (delete-process server) + (delete-process proc) ; otherwise seems client proc is sometimes reused (kill-buffer (process-buffer server)) (kill-buffer buf) (ignore url-gateway-method))) +;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-v5-auth-user-pass () + "Verify correct handling of SOCKS5 user/pass authentication." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-user-agent "Test/auth-user-pass") + (socks-tests-canned-server-patterns + `(([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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Services (like Tor) may be configured without auth but for some +;; reason still prefer the user/pass method over none when offered both. +;; Given this library's defaults, the scenario below is possible. +;; +;; FYI: RFC 1929 doesn't say that a username or password is required +;; but notes that the length of both fields should be at least one. +;; However, both socks.el and curl send zero-length fields (though +;; curl drops the user part too when the password is empty). +;; +;; To verify against curl 7.71, swap out the first two pattern pairs +;; with ([5 3 0 1 2] . [5 2]) and ([1 0 0] . [1 0]), then run: +;; $ curl verbose -U "foo:" --proxy socks5h://127.0.0.1:10081 example.com + +(ert-deftest socks-tests-v5-auth-user-pass-blank () + "Verify correct SOCKS5 user/pass authentication with empty pass." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10081 5)) + (socks-username "foo") ; defaults to (user-login-name) + (socks-password "") ; simulate user hitting enter when prompted + (url-user-agent "Test/auth-user-pass-blank") + (socks-tests-canned-server-patterns + `(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 0] . [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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Swap out ([5 2 0 1] . [5 0]) with the first pattern below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com + +(ert-deftest socks-tests-v5-auth-none () + "Verify correct handling of SOCKS5 when auth method 0 requested." + (let ((socks-server '("server" "127.0.0.1" 10082 5)) + (socks-authentication-methods (append socks-authentication-methods + nil)) + (url-user-agent "Test/auth-none") + (socks-tests-canned-server-patterns + `(([5 1 0] . [5 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]) + ,socks-tests--hello-world-http-request-pattern))) + (socks-unregister-authentication-method 2) + (should-not (assq 2 socks-authentication-methods)) + (ert-info ("Make HTTP request over SOCKS5 with no auth method") + (socks-tests-perform-hello-world-http-request))) + (should (assq 2 socks-authentication-methods))) + ;;; socks-tests.el ends here -- 2.29.2 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-Preserve-numeric-addresses-in-socks-send-command.patch --] [-- Type: text/x-patch, Size: 2359 bytes --] From d7c4cbea85f991fe8b2ed3b1edba6e99d3cac98c Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 19:41:04 -0800 Subject: [PATCH 2/2] Preserve numeric addresses in socks-send-command * lisp/net/socks.el: (socks-send-command): * test/lisp/net/socks-tests.el: (socks-tests-v4-basic): (Bug#46342). --- lisp/net/socks.el | 2 +- test/lisp/net/socks-tests.el | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 96fafc826b8..019fadd1440 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -393,7 +393,7 @@ socks-send-command (let ((addr (cond ((or (= atype socks-address-type-v4) (= atype socks-address-type-v6)) - address) + (encode-coding-string address 'latin-1)) ((= atype socks-address-type-name) (format "%c%s" (length address) address)) (t diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index d2c21094e7f..7112f42a67c 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -183,6 +183,26 @@ socks-tests-perform-hello-world-http-request (kill-buffer buf) (ignore url-gateway-method))) +;; Unlike curl, socks.el includes the ID field (but otherwise matches): +;; $ curl --proxy socks4://127.0.0.1:1080 example.com + +(ert-deftest socks-tests-v4-basic () + "Show correct preparation of SOCKS4 connect command." + (let ((socks-server '("server" "127.0.0.1" 10079 4)) + (url-user-agent "Test/4-basic") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 184 216 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + ;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate ;; against curl 7.71 with the following options: ;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com -- 2.29.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-09 15:17 ` J.P. @ 2021-02-09 16:14 ` Eli Zaretskii 2021-02-10 13:16 ` J.P. 0 siblings, 1 reply; 21+ messages in thread From: Eli Zaretskii @ 2021-02-09 16:14 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Tue, 09 Feb 2021 07:17:29 -0800 > > > [1:text/plain Hide] > > Eli, > > Apologies for my previous rambling reply. Your time is supremely > valuable, and I should've been more mindful of that. I'm hoping you'll > allow me this mulligan (ahem): > > > That depends on what the other end expects. Does it expect latin-1 in > > this case? > > Yes. > > > Does emitting the single byte \330 produce the correct result in this > > case? > > Yes. > > > Then by all means please use > > > > (encode-coding-string address 'latin-1) > > I have updated my changes to reflect this. > > > And those [tor/ssh] expect Latin-1 encoding in this case? > > Yes. > > When you've got a sec, please instruct me on how best to proceed (even > if that means buggering off 'til I've learned more about Emacs!). :) (I'd really prefer that someone who knows more than I do about SOCKS review this.) If I must do that, I have a question: what kind of string can this ADDRESS be? My reading of RFC 1928 is that it normally is an IP address, in which case encoding is not relevant, as it's an ASCII string. But it can also be a domain, right? If so, what form can this domain take? If the domain has non-ASCII characters, shouldn't it be hex-encoded, or run through IDNA? I mean, are non-ASCII characters in that place at all allowed? ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-09 16:14 ` Eli Zaretskii @ 2021-02-10 13:16 ` J.P. 2021-02-10 16:04 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-10 13:16 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 Eli Zaretskii <eliz@gnu.org> writes: > what kind of string can this ADDRESS be? My reading of RFC 1928 is > that it normally is an IP address, in which case encoding is not > relevant, as it's an ASCII string. But it can also be a domain, right? This patch only affects IP addresses, but I'm happy to look into the domain name form as well. > If so, what form can this domain take? If the domain has non-ASCII > characters, shouldn't it be hex-encoded, or run through IDNA? I mean, > are non-ASCII characters in that place at all allowed? At first glance, both tor and ssh appear to call getaddrinfo() on the remote end without accounting for the sender's locale or passing any special IDN-related flags. But I'm still looking into these. For now, if we're allowing anecdotal caveman logic, I'd wager the answer is ASCII only. Here's why: It seems feeding tor and ssh the hostname for Яндекс.рф (Yandex) as the UTF-8 encoded byte string \xd0\xaf\xd0\xbd\xd0\xb4\xd0\xb5\xd0\xba\xd1\x81.\xd1\x80\xd1\x84 results in failure both when forwarding via CONNECT and when resolving via tor's nonstandard RESOLVE command. (This is direct, no Emacs.) However, passing the punified "xn--d1acpjx3f.xn--p1ai" works as intended, forwarding to (or, in the case of RESOLVE, producing) an IP from a Yandex-registered A record (for me, 77.88.55.66). To try this at home (on separate ttys): $ ssh -TND 4711 my.sshd # tcpdump -i lo -nnX "port 4711" $ curl --verbose --proxy socks5h://localhost:4711 Яндекс.рф Here's a trace for curl's actual call to the hostname conversion function idn2_lookup_ul() [1], which is provided by GNU libidn2 [2]. It's hard to see without context, but this happens before any connection is established (tcpdump will confirm this). #0 Curl_idnconvert_hostname at lib/url.c:1566 #1 create_conn at lib/url.c:3583 #2 Curl_connect at lib/url.c:4027 #3 multi_runsingle at lib/multi.c:1671 #4 curl_multi_perform at lib/multi.c:2412 #5 easy_transfer at lib/easy.c:606 #6 easy_perform at lib/easy.c:696 #7 curl_easy_perform at lib/easy.c:715 #8 serial_transfers at src/tool_operate.c:2327 #9 run_all_transfers at src/tool_operate.c:2505 #10 operate at src/tool_operate.c:2621 #11 main at src/tool_main.c:277 On my machine, curl was configured to pass these flags to idn2_lookup_ul[3]: /* IDN2_NFC_INPUT: Normalize input string using normalization form C. IDN2_NONTRANSITIONAL: Perform Unicode TR46 non-transitional processing. */ int flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL; Apparently there are two IDNA standards: 2003 and 2008 [4]. Curl uses the latter, but I'm not sure which, if any, puny.el favors. In the case of Yandex, (puny-encode-domain "Яндекс.рф") produces "xn--d1acpjx9e.xn--p1ai", which tor and ssh both reject (though it's very possible I'm missing something.) Anyway, passing the version above provided by libidn2 to socks-send-command works fine. [1] https://github.com/curl/curl/blob/ec5d9b44a2e837fc7b82d1c60d5fae3f851620dc/lib/url.c#L1559 [2] https://www.gnu.org/software/libidn/libidn2/reference/libidn2-idn2.html#idn2-lookup-ul [3] https://www.gnu.org/software/libidn/libidn2/reference/libidn2-idn2.html#idn2-flags [4] https://www.unicode.org/reports/tr46/#Table_Example_Processing ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-10 13:16 ` J.P. @ 2021-02-10 16:04 ` Eli Zaretskii 2021-02-11 14:58 ` J.P. 0 siblings, 1 reply; 21+ messages in thread From: Eli Zaretskii @ 2021-02-10 16:04 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Wed, 10 Feb 2021 05:16:58 -0800 > > Eli Zaretskii <eliz@gnu.org> writes: > > > what kind of string can this ADDRESS be? My reading of RFC 1928 is > > that it normally is an IP address, in which case encoding is not > > relevant, as it's an ASCII string. But it can also be a domain, right? > > This patch only affects IP addresses, but I'm happy to look into the > domain name form as well. Then I don't understand why we need to worry about encoding. IP addresses are pure ASCII strings, so they need no encoding whatsoever. I guess I will have to ask you to back up and describe what problems you saw with the original code, and show me the details of the strings involved in that. Thanks for the rest of your message, but I suggest that we limit ourselves to IP addresses for the time being, and only go to non-ASCII domains when we have the IP address use case figured out. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-10 16:04 ` Eli Zaretskii @ 2021-02-11 14:58 ` J.P. 2021-02-11 15:28 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-11 14:58 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 Eli Zaretskii <eliz@gnu.org> writes: > Then I don't understand why we need to worry about encoding. IP > addresses are pure ASCII strings, so they need no encoding whatsoever. Clearly, I'm failing you here. Between my dearth of communications skills and lack of Emacs know how, I've obviously managed to deceive you into thinking that SOCKS IP address fields ought to contain ASCII text characters such as the following: 1 9 2 . 1 6 8 . 1 . 1 31 39 32 2e 31 36 38 2e 31 2e 31 However, this is not the case [a]. In version 4, all addresses are four-byte sequences, one byte for each component of an IPv4 address, and the ordering is left-to-right. For example: 192 168 1 1 c0 a8 01 01 In version 5, the one covered by RFC 1928, this is extended to include 16-byte IPv6 addresses as well as ASCII domain names. All three are exclusive to one another but occupy the same field in a union of sorts. The first byte of that field, the "ATYP" flag, denotes which of the three to expect, and it appears as the "atype" argument to socks-send-command. > I guess I will have to ask you to back up and describe what problems > you saw with the original code, and show me the details of the strings > involved in that. The Elisp manual distinguishes between multibyte and unibyte "sources" of strings [1]. For these (SOCKS 4) IP address strings, the function socks--open-network-stream is that source (it creates them). When such a string includes characters with code points between 128 and 255 (the latin-1/iso-8859-1 range), single characters are sent as two utf-8 encoded bytes, which the SOCKS service rejects as violating protocol. Specifically, when a user passes "example.com" to the entry-point function socks-open-network-stream, its internal helper socks--open-network-stream resolves the host name into an IP in list form and then converts this to a string by calling (apply #'format "%c%c%c%c" '(93 184 216 34)) This produces a multibyte string of the same character length: "]¸Ø\"" However, when socks-send-command passes this to process-send-string, whose coding system is (binary . binary), the underlying six-byte sequence is emitted verbatim: "]\302\270\303\230\"" My initial idea was to leverage the function unibyte-string to ensure every character can be encoded in 8 bits before transmission. Regardless, performing some combination of validating and converting before sending may be worthwhile since it'll only run once per connection. Sorry for the extended play-by-play. I certainly hope none of it came off as insulting or pedantic. I'm quite certain your grasp of such concepts long ago outpaced any understanding I could ever hope to attain. J.P. [a] My versions of tor and ssh definitely honor requests like curl --proxy socks5h://localhost:1080 http://93.184.216.34 passing the IP address as a domain name. Although this defies RFC 1928, which specifies FQDNs only [1], I'm getting the sense that influential projects treat the latter more as a living standard. (Note: in its unit tests, tor only includes this form for its extension commands [2].) [1] (elisp) Non-ASCII in Strings, second paragraph [1] https://tools.ietf.org/html/rfc1928#section-5 [2] https://gitweb.torproject.org/tor.git/tree/src/test/test_socks.c#n335 ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-11 14:58 ` J.P. @ 2021-02-11 15:28 ` Eli Zaretskii 2021-02-12 14:30 ` J.P. 0 siblings, 1 reply; 21+ messages in thread From: Eli Zaretskii @ 2021-02-11 15:28 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Thu, 11 Feb 2021 06:58:00 -0800 > > Eli Zaretskii <eliz@gnu.org> writes: > > > Then I don't understand why we need to worry about encoding. IP > > addresses are pure ASCII strings, so they need no encoding whatsoever. > > Clearly, I'm failing you here. Between my dearth of communications > skills and lack of Emacs know how, I've obviously managed to deceive you > into thinking that SOCKS IP address fields ought to contain ASCII text > characters such as the following: > > 1 9 2 . 1 6 8 . 1 . 1 > 31 39 32 2e 31 36 38 2e 31 2e 31 > > However, this is not the case [a]. In version 4, all addresses are > four-byte sequences, one byte for each component of an IPv4 address, and > the ordering is left-to-right. For example: > > 192 168 1 1 > c0 a8 01 01 Then they are what we call "raw bytes", and encoding them with raw-text-unix should suffice. How does the code which calls socks.el create these raw bytes? ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-11 15:28 ` Eli Zaretskii @ 2021-02-12 14:30 ` J.P. 2021-02-12 15:04 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-12 14:30 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 Eli Zaretskii <eliz@gnu.org> writes: > Then they are what we call "raw bytes", and encoding them with > raw-text-unix should suffice. Thanks. Unfortunately, this produces the same utf-8 encoded bytes. (encode-coding-char 192 'raw-text-unix) ⇒ "\303\200" It looks like raw-text-unix is an alias for binary [1], the coding system already used by the network process sending the erroneous request. I suppose it's always possible to strong arm it like (encode-coding-char (or (decode-char 'eight-bit c) c) 'raw-text-unix) ⇒ "^@" ... "\377" But what about your original latin-1 suggestion? Is that no longer in contention? (encode-coding-char 192 'latin-1) ⇒ "\300" > How does the code which calls socks.el create these raw bytes? This library has an entry-point function that's part of the url-gateway dispatch mechanism. I can't say for certain, but it looks like url-http is the only library directly using this facility. Regardless, the function gets called with a (possibly multibyte) host name, which in rare cases may be an ASCII IP address created by url-gateway. With SOCKS4, that's kind of moot, since all names are looked up through socks-nslookup-host, which returns an IPv4 address as a list of fixnums. Its caller is an internal helper that converts this list into a multibyte string for socks-send-command to emit onto the wire (where it's then rejected by the service). Currently, IP addresses aren't used at all for v5 connect-command requests. And raw-byte IP addresses do not yet appear anywhere [2]. This patch would introduce them, either as an argument to socks-send-command or as something ephemeral produced by it (the current idea). [1] (elisp) Coding System Basics [2] Of course, these are generalities that don't apply to users who wire everything up manually. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-12 14:30 ` J.P. @ 2021-02-12 15:04 ` Eli Zaretskii 2021-02-13 15:43 ` J.P. 2021-02-17 14:59 ` J.P. 0 siblings, 2 replies; 21+ messages in thread From: Eli Zaretskii @ 2021-02-12 15:04 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Fri, 12 Feb 2021 06:30:32 -0800 > > Eli Zaretskii <eliz@gnu.org> writes: > > > Then they are what we call "raw bytes", and encoding them with > > raw-text-unix should suffice. > > Thanks. Unfortunately, this produces the same utf-8 encoded bytes. > > (encode-coding-char 192 'raw-text-unix) > ⇒ "\303\200" 192 is not a raw-byte, it's a character whose Unicode codepoint is 192. So you get its UTF-8 sequence. > It looks like raw-text-unix is an alias for binary [1], the coding > system already used by the network process sending the erroneous > request. The problem is with how the original request is generated, not how it is encoded. > I suppose it's always possible to strong arm it like > > (encode-coding-char (or (decode-char 'eight-bit c) c) 'raw-text-unix) > ⇒ "^@" ... "\377" That's one way, yes. But it isn't the best one. > But what about your original latin-1 suggestion? Is that no longer in > contention? No, it isn't. > (encode-coding-char 192 'latin-1) > ⇒ "\300" Not every byte above 127 is a valid character that Latin-1 can meaningfully encode. It is wrong to use Latin-1 for raw bytes. What you need is a way of generating a unibyte string from a series of raw bytes, > > How does the code which calls socks.el create these raw bytes? > > This library has an entry-point function that's part of the url-gateway > dispatch mechanism. I can't say for certain, but it looks like url-http > is the only library directly using this facility. Regardless, the > function gets called with a (possibly multibyte) host name, which in > rare cases may be an ASCII IP address created by url-gateway. > > With SOCKS4, that's kind of moot, since all names are looked up through > socks-nslookup-host, which returns an IPv4 address as a list of fixnums. > Its caller is an internal helper that converts this list into a > multibyte string for socks-send-command to emit onto the wire (where > it's then rejected by the service). > > Currently, IP addresses aren't used at all for v5 connect-command > requests. And raw-byte IP addresses do not yet appear anywhere [2]. This > patch would introduce them, either as an argument to socks-send-command > or as something ephemeral produced by it (the current idea). So what is the problem with using unibyte-string for producing a unibyte string from a list of bytes? It sounds like it's exactly what is needed here, and is actually used in some places in socks.el. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-12 15:04 ` Eli Zaretskii @ 2021-02-13 15:43 ` J.P. 2021-02-17 14:59 ` J.P. 1 sibling, 0 replies; 21+ messages in thread From: J.P. @ 2021-02-13 15:43 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 [-- Attachment #1: Type: text/plain, Size: 658 bytes --] Eli Zaretskii <eliz@gnu.org> writes: > The problem is with how the original request is generated, not how it > is encoded. Sorry, right. That's what I should have said. And explicitly applying an encoding to finagle the wrong characters into the right bytes is not the way to go, I now realize. > So what is the problem with using unibyte-string for producing a > unibyte string from a list of bytes? It sounds like it's exactly what > is needed here, and is actually used in some places in socks.el. Ah, that's the ticket. Thanks. I've also moved the conversion business out of socks-send-command and into its caller where it's hopefully a better fit. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-more-auth-related-tests-for-socks.el.patch --] [-- Type: text/x-patch, Size: 16142 bytes --] From a077fe903fef50d76f8b0d721a1c9894a7cf3a04 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:24:55 -0800 Subject: [PATCH 1/2] Add more auth-related tests for socks.el * test/lisp/net/socks-tests.el (auth-registration-and-suite-offer) (filter-response-parsing-v4, filter-response-parsing-v5): Assert auth-method selection wrangling and socks-filter parsing. (v5-auth-user-pass, v5-auth-user-pass-blank, v5-auth-none): Show prep and execution of the SOCKS connect command and proxying of an HTTP request; simplify fake server. --- test/lisp/net/socks-tests.el | 270 ++++++++++++++++++++++++++++------- 1 file changed, 215 insertions(+), 55 deletions(-) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index b378ed2964e..340a42d79cc 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -21,68 +21,151 @@ ;;; Code: +(require 'ert) (require 'socks) (require 'url-http) -(defvar socks-tests-canned-server-port nil) +(ert-deftest socks-tests-auth-registration-and-suite-offer () + (ert-info ("Default favors user/pass auth") + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) ; length [offer ...] + (let (socks-authentication-methods) + (ert-info ("Empty selection/no methods offered") + (should (equal "\0" (socks-build-auth-list)))) + (ert-info ("Simulate library defaults") + (socks-register-authentication-method 0 "No authentication" + 'identity) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-register-authentication-method 2 "Username/Password" + 'socks-username/password-auth) + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) + (ert-info ("Removal") + (socks-unregister-authentication-method 2) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-unregister-authentication-method 0) + (should-not socks-authentication-methods) + (should (equal "\0" (socks-build-auth-list)))))) -(defun socks-tests-canned-server-create (verbatim patterns) - "Create a fake SOCKS server and return the process. +(ert-deftest socks-tests-filter-response-parsing-v4 () + "Ensure new chunks added on right (Bug#45162)." + (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") + (socks-filter proc (concat [0 90 0 0 93 184 216])) + ;; From example.com: OK status ^ ^ msg start + (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= (concat [0 90 0 0 93 184 216]) + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch and response fields hold stash w. last chunk") + (should (string= (concat [0 90 0 0 93 184 216 34]) + (process-get proc 'socks-response))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) -`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*")) +(ert-deftest socks-tests-filter-response-parsing-v5 () + "Ensure new chunks added on right (Bug#45162)." + (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") + ;; From fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + ;; 5004 ~~> Version Status (OK) NOOP Addr-Type (4 -> IPv6) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") + (ert-info ("State still waiting and response emtpy") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (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 ("Middle chunk arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State and response fields still untouched") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival appended (on RHS)") + (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 and response fields show last chunk 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))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) + +(defvar socks-tests-canned-server-patterns nil + "Alist containing request/response cons pairs to be tried in order. +Vectors must match verbatim. Strings are considered regex patterns.") + +(defun socks-tests-canned-server-create () + "Create and return a fake SOCKS server." + (let* ((port (nth 2 socks-server)) + (name (format "socks-server:%d" port)) + (pats socks-tests-canned-server-patterns) (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 + (pcase-let ((`(,pat . ,resp) (pop pats))) + (unless (or (and (vectorp pat) (equal pat (vconcat line))) + (string-match-p pat line)) (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)) + (message "[%s] <- %s" name (prin1-to-string line)) + (message "[%s] -> %s" name (prin1-to-string resp))) (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 + (serv (make-network-process :server 1 + :buffer (get-buffer-create name) + :filter filt + :name name + :family 'ipv4 + :host 'local + :coding 'binary + :service port))) + (set-process-query-on-exit-flag serv nil) + serv)) -(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) +(defvar socks-tests--hello-world-http-request-pattern + (cons "^GET /" (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n"))) + +(defun socks-tests-perform-hello-world-http-request () + "Start canned server, validate hello-world response, and finalize." + (let* ((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) + (server (socks-tests-canned-server-create)) ;; done ;; @@ -90,14 +173,91 @@ socks-tests-auth-filter-url-http (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))) + (buf (url-http url cb '(nil))) + (proc (get-buffer-process buf)) + (attempts 10)) + (while (and (not done) (< 0 (cl-decf attempts))) + (sleep-for 0.1)) (should done) (delete-process server) + (delete-process proc) ; otherwise seems client proc is sometimes reused (kill-buffer (process-buffer server)) (kill-buffer buf) (ignore url-gateway-method))) +;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-v5-auth-user-pass () + "Verify correct handling of SOCKS5 user/pass authentication." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-user-agent "Test/auth-user-pass") + (socks-tests-canned-server-patterns + `(([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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Services (like Tor) may be configured without auth but for some +;; reason still prefer the user/pass method over none when offered both. +;; Given this library's defaults, the scenario below is possible. +;; +;; FYI: RFC 1929 doesn't say that a username or password is required +;; but notes that the length of both fields should be at least one. +;; However, both socks.el and curl send zero-length fields (though +;; curl drops the user part too when the password is empty). +;; +;; From Tor's docs /socks-extensions.txt, 1.1 Extent of support: +;; > We allow username/password fields of this message to be empty ... +;; line 41 in blob 5fd1f828f3e9d014f7b65fa3bd1d33c39e4129e2 +;; https://gitweb.torproject.org/torspec.git/tree/socks-extensions.txt +;; +;; To verify against curl 7.71, swap out the first two pattern pairs +;; with ([5 3 0 1 2] . [5 2]) and ([1 0 0] . [1 0]), then run: +;; $ curl verbose -U "foo:" --proxy socks5h://127.0.0.1:10081 example.com + +(ert-deftest socks-tests-v5-auth-user-pass-blank () + "Verify correct SOCKS5 user/pass authentication with empty pass." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10081 5)) + (socks-username "foo") ; defaults to (user-login-name) + (socks-password "") ; simulate user hitting enter when prompted + (url-user-agent "Test/auth-user-pass-blank") + (socks-tests-canned-server-patterns + `(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 0] . [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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Swap out ([5 2 0 1] . [5 0]) with the first pattern below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com + +(ert-deftest socks-tests-v5-auth-none () + "Verify correct handling of SOCKS5 when auth method 0 requested." + (let ((socks-server '("server" "127.0.0.1" 10082 5)) + (socks-authentication-methods (append socks-authentication-methods + nil)) + (url-user-agent "Test/auth-none") + (socks-tests-canned-server-patterns + `(([5 1 0] . [5 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]) + ,socks-tests--hello-world-http-request-pattern))) + (socks-unregister-authentication-method 2) + (should-not (assq 2 socks-authentication-methods)) + (ert-info ("Make HTTP request over SOCKS5 with no auth method") + (socks-tests-perform-hello-world-http-request))) + (should (assq 2 socks-authentication-methods))) + ;;; socks-tests.el ends here -- 2.29.2 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-Use-raw-bytes-for-SOCKS-4-IP-addresses.patch --] [-- Type: text/x-patch, Size: 2462 bytes --] From 8a1b4835b5ba60cae0a0ee325a3110d04c92948f Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 19:41:04 -0800 Subject: [PATCH 2/2] Use raw bytes for SOCKS 4 IP addresses * lisp/net/socks.el: (socks--open-network-stream): * test/lisp/net/socks-tests.el: (socks-tests-v4-basic): (Bug#46342). --- lisp/net/socks.el | 2 +- test/lisp/net/socks-tests.el | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 96fafc826b8..9aaa11ab416 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -528,7 +528,7 @@ socks--open-network-stream (setq host (socks-nslookup-host host)) (if (not (listp host)) (error "Could not get IP address for: %s" host)) - (setq host (apply #'format "%c%c%c%c" host)) + (setq host (apply #'unibyte-string host)) socks-address-type-v4) (t socks-address-type-name)))) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index 340a42d79cc..9a2dcba9daf 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -185,6 +185,26 @@ socks-tests-perform-hello-world-http-request (kill-buffer buf) (ignore url-gateway-method))) +;; Unlike curl, socks.el includes the ID field (but otherwise matches): +;; $ curl --proxy socks4://127.0.0.1:1080 example.com + +(ert-deftest socks-tests-v4-basic () + "Show correct preparation of SOCKS4 connect command (Bug#46342)." + (let ((socks-server '("server" "127.0.0.1" 10079 4)) + (url-user-agent "Test/4-basic") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 184 216 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + ;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate ;; against curl 7.71 with the following options: ;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com -- 2.29.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-12 15:04 ` Eli Zaretskii 2021-02-13 15:43 ` J.P. @ 2021-02-17 14:59 ` J.P. 2021-02-20 9:33 ` Eli Zaretskii 1 sibling, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-17 14:59 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 [-- Attachment #1: Type: text/plain, Size: 409 bytes --] Eli Zaretskii <eliz@gnu.org> writes: > So what is the problem with using unibyte-string for producing a > unibyte string from a list of bytes? It sounds like it's exactly > what is needed here, and is actually used in some places in socks.el. Great, thanks. That's what was needed indeed. I've updated things accordingly and have added a doc string to socks-send-command explaining the address parameter. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-more-auth-related-tests-for-socks.el.patch --] [-- Type: text/x-patch, Size: 16142 bytes --] From 4e058c6a78f8a219d159444aadaf5e9cf5004870 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:24:55 -0800 Subject: [PATCH 1/2] Add more auth-related tests for socks.el * test/lisp/net/socks-tests.el (auth-registration-and-suite-offer) (filter-response-parsing-v4, filter-response-parsing-v5): Assert auth-method selection wrangling and socks-filter parsing. (v5-auth-user-pass, v5-auth-user-pass-blank, v5-auth-none): Show prep and execution of the SOCKS connect command and proxying of an HTTP request; simplify fake server. --- test/lisp/net/socks-tests.el | 270 ++++++++++++++++++++++++++++------- 1 file changed, 215 insertions(+), 55 deletions(-) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index b378ed2964e..340a42d79cc 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -21,68 +21,151 @@ ;;; Code: +(require 'ert) (require 'socks) (require 'url-http) -(defvar socks-tests-canned-server-port nil) +(ert-deftest socks-tests-auth-registration-and-suite-offer () + (ert-info ("Default favors user/pass auth") + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) ; length [offer ...] + (let (socks-authentication-methods) + (ert-info ("Empty selection/no methods offered") + (should (equal "\0" (socks-build-auth-list)))) + (ert-info ("Simulate library defaults") + (socks-register-authentication-method 0 "No authentication" + 'identity) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-register-authentication-method 2 "Username/Password" + 'socks-username/password-auth) + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) + (ert-info ("Removal") + (socks-unregister-authentication-method 2) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-unregister-authentication-method 0) + (should-not socks-authentication-methods) + (should (equal "\0" (socks-build-auth-list)))))) -(defun socks-tests-canned-server-create (verbatim patterns) - "Create a fake SOCKS server and return the process. +(ert-deftest socks-tests-filter-response-parsing-v4 () + "Ensure new chunks added on right (Bug#45162)." + (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") + (socks-filter proc (concat [0 90 0 0 93 184 216])) + ;; From example.com: OK status ^ ^ msg start + (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= (concat [0 90 0 0 93 184 216]) + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch and response fields hold stash w. last chunk") + (should (string= (concat [0 90 0 0 93 184 216 34]) + (process-get proc 'socks-response))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) -`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*")) +(ert-deftest socks-tests-filter-response-parsing-v5 () + "Ensure new chunks added on right (Bug#45162)." + (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") + ;; From fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + ;; 5004 ~~> Version Status (OK) NOOP Addr-Type (4 -> IPv6) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") + (ert-info ("State still waiting and response emtpy") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (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 ("Middle chunk arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State and response fields still untouched") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival appended (on RHS)") + (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 and response fields show last chunk 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))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) + +(defvar socks-tests-canned-server-patterns nil + "Alist containing request/response cons pairs to be tried in order. +Vectors must match verbatim. Strings are considered regex patterns.") + +(defun socks-tests-canned-server-create () + "Create and return a fake SOCKS server." + (let* ((port (nth 2 socks-server)) + (name (format "socks-server:%d" port)) + (pats socks-tests-canned-server-patterns) (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 + (pcase-let ((`(,pat . ,resp) (pop pats))) + (unless (or (and (vectorp pat) (equal pat (vconcat line))) + (string-match-p pat line)) (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)) + (message "[%s] <- %s" name (prin1-to-string line)) + (message "[%s] -> %s" name (prin1-to-string resp))) (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 + (serv (make-network-process :server 1 + :buffer (get-buffer-create name) + :filter filt + :name name + :family 'ipv4 + :host 'local + :coding 'binary + :service port))) + (set-process-query-on-exit-flag serv nil) + serv)) -(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) +(defvar socks-tests--hello-world-http-request-pattern + (cons "^GET /" (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n"))) + +(defun socks-tests-perform-hello-world-http-request () + "Start canned server, validate hello-world response, and finalize." + (let* ((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) + (server (socks-tests-canned-server-create)) ;; done ;; @@ -90,14 +173,91 @@ socks-tests-auth-filter-url-http (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))) + (buf (url-http url cb '(nil))) + (proc (get-buffer-process buf)) + (attempts 10)) + (while (and (not done) (< 0 (cl-decf attempts))) + (sleep-for 0.1)) (should done) (delete-process server) + (delete-process proc) ; otherwise seems client proc is sometimes reused (kill-buffer (process-buffer server)) (kill-buffer buf) (ignore url-gateway-method))) +;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-v5-auth-user-pass () + "Verify correct handling of SOCKS5 user/pass authentication." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-user-agent "Test/auth-user-pass") + (socks-tests-canned-server-patterns + `(([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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Services (like Tor) may be configured without auth but for some +;; reason still prefer the user/pass method over none when offered both. +;; Given this library's defaults, the scenario below is possible. +;; +;; FYI: RFC 1929 doesn't say that a username or password is required +;; but notes that the length of both fields should be at least one. +;; However, both socks.el and curl send zero-length fields (though +;; curl drops the user part too when the password is empty). +;; +;; From Tor's docs /socks-extensions.txt, 1.1 Extent of support: +;; > We allow username/password fields of this message to be empty ... +;; line 41 in blob 5fd1f828f3e9d014f7b65fa3bd1d33c39e4129e2 +;; https://gitweb.torproject.org/torspec.git/tree/socks-extensions.txt +;; +;; To verify against curl 7.71, swap out the first two pattern pairs +;; with ([5 3 0 1 2] . [5 2]) and ([1 0 0] . [1 0]), then run: +;; $ curl verbose -U "foo:" --proxy socks5h://127.0.0.1:10081 example.com + +(ert-deftest socks-tests-v5-auth-user-pass-blank () + "Verify correct SOCKS5 user/pass authentication with empty pass." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10081 5)) + (socks-username "foo") ; defaults to (user-login-name) + (socks-password "") ; simulate user hitting enter when prompted + (url-user-agent "Test/auth-user-pass-blank") + (socks-tests-canned-server-patterns + `(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 0] . [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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Swap out ([5 2 0 1] . [5 0]) with the first pattern below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com + +(ert-deftest socks-tests-v5-auth-none () + "Verify correct handling of SOCKS5 when auth method 0 requested." + (let ((socks-server '("server" "127.0.0.1" 10082 5)) + (socks-authentication-methods (append socks-authentication-methods + nil)) + (url-user-agent "Test/auth-none") + (socks-tests-canned-server-patterns + `(([5 1 0] . [5 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]) + ,socks-tests--hello-world-http-request-pattern))) + (socks-unregister-authentication-method 2) + (should-not (assq 2 socks-authentication-methods)) + (ert-info ("Make HTTP request over SOCKS5 with no auth method") + (socks-tests-perform-hello-world-http-request))) + (should (assq 2 socks-authentication-methods))) + ;;; socks-tests.el ends here -- 2.29.2 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-Use-raw-bytes-for-SOCKS-4-IP-addresses.patch --] [-- Type: text/x-patch, Size: 2853 bytes --] From 7daa1e7f1c8581e3aaab8cf94731de8e1d140fae Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 19:41:04 -0800 Subject: [PATCH 2/2] Use raw bytes for SOCKS 4 IP addresses * lisp/net/socks.el: (socks--open-network-stream, socks-send-command): * test/lisp/net/socks-tests.el: (socks-tests-v4-basic): (Bug#46342). --- lisp/net/socks.el | 4 +++- test/lisp/net/socks-tests.el | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 96fafc826b8..1da1d31d678 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -390,6 +390,8 @@ socks-open-connection proc))) (defun socks-send-command (proc command atype address port) + "Send COMMAND to SOCKS service PROC for proxying ADDRESS and PORT. +When ATYPE indicates an IP, param ADDRESS must be given as raw bytes." (let ((addr (cond ((or (= atype socks-address-type-v4) (= atype socks-address-type-v6)) @@ -528,7 +530,7 @@ socks--open-network-stream (setq host (socks-nslookup-host host)) (if (not (listp host)) (error "Could not get IP address for: %s" host)) - (setq host (apply #'format "%c%c%c%c" host)) + (setq host (apply #'unibyte-string host)) socks-address-type-v4) (t socks-address-type-name)))) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index 340a42d79cc..9a2dcba9daf 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -185,6 +185,26 @@ socks-tests-perform-hello-world-http-request (kill-buffer buf) (ignore url-gateway-method))) +;; Unlike curl, socks.el includes the ID field (but otherwise matches): +;; $ curl --proxy socks4://127.0.0.1:1080 example.com + +(ert-deftest socks-tests-v4-basic () + "Show correct preparation of SOCKS4 connect command (Bug#46342)." + (let ((socks-server '("server" "127.0.0.1" 10079 4)) + (url-user-agent "Test/4-basic") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 184 216 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + ;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate ;; against curl 7.71 with the following options: ;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com -- 2.29.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-17 14:59 ` J.P. @ 2021-02-20 9:33 ` Eli Zaretskii 2021-02-20 10:13 ` J.P. 2021-02-20 10:41 ` J.P. 0 siblings, 2 replies; 21+ messages in thread From: Eli Zaretskii @ 2021-02-20 9:33 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Wed, 17 Feb 2021 06:59:07 -0800 > > Eli Zaretskii <eliz@gnu.org> writes: > > > So what is the problem with using unibyte-string for producing a > > unibyte string from a list of bytes? It sounds like it's exactly > > what is needed here, and is actually used in some places in socks.el. > > Great, thanks. That's what was needed indeed. I've updated things > accordingly and have added a doc string to socks-send-command explaining > the address parameter. Thanks. I wanted to install this, but it failed to apply to the master branch. Could you please rebase on the current master branch and resubmit? ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-20 9:33 ` Eli Zaretskii @ 2021-02-20 10:13 ` J.P. 2021-02-20 11:08 ` Eli Zaretskii 2021-02-20 10:41 ` J.P. 1 sibling, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-20 10:13 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 [-- Attachment #1: Type: text/plain, Size: 935 bytes --] Eli Zaretskii <eliz@gnu.org> writes: > Thanks. I wanted to install this, but it failed to apply to the > master branch. Could you please rebase on the current master branch > and resubmit? Hi Eli, I've gone ahead and rebased atop commit c85c8e7d42ae2a5fc95fa7b14257389d8383b34d Author: Stefan Kangas <stefan@marxist.se> Add toolbar for help-mode lisp/help-mode.el | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) However the src/dest blob hashes (i.e., git-hash-object(1) SHAs in the diffs' index dead..beef 0644 lines) remain unchanged. Which leads me to think maybe the failure is related to these being two individual patches (a "change set"). I've had trouble in the past applying similar sets using debbugs-gnu-apply-patch, but that might've just been incompetence. Regardless, I'd be happy to squash these down into a single commit if you think that'd help. Thanks. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-more-auth-related-tests-for-socks.el.patch --] [-- Type: text/x-patch, Size: 16142 bytes --] From 2fba12e3c260c2e11e115cf0e2384e0624103cf5 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:24:55 -0800 Subject: [PATCH 1/2] Add more auth-related tests for socks.el * test/lisp/net/socks-tests.el (auth-registration-and-suite-offer) (filter-response-parsing-v4, filter-response-parsing-v5): Assert auth-method selection wrangling and socks-filter parsing. (v5-auth-user-pass, v5-auth-user-pass-blank, v5-auth-none): Show prep and execution of the SOCKS connect command and proxying of an HTTP request; simplify fake server. --- test/lisp/net/socks-tests.el | 270 ++++++++++++++++++++++++++++------- 1 file changed, 215 insertions(+), 55 deletions(-) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index b378ed2964e..340a42d79cc 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -21,68 +21,151 @@ ;;; Code: +(require 'ert) (require 'socks) (require 'url-http) -(defvar socks-tests-canned-server-port nil) +(ert-deftest socks-tests-auth-registration-and-suite-offer () + (ert-info ("Default favors user/pass auth") + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) ; length [offer ...] + (let (socks-authentication-methods) + (ert-info ("Empty selection/no methods offered") + (should (equal "\0" (socks-build-auth-list)))) + (ert-info ("Simulate library defaults") + (socks-register-authentication-method 0 "No authentication" + 'identity) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-register-authentication-method 2 "Username/Password" + 'socks-username/password-auth) + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) + (ert-info ("Removal") + (socks-unregister-authentication-method 2) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-unregister-authentication-method 0) + (should-not socks-authentication-methods) + (should (equal "\0" (socks-build-auth-list)))))) -(defun socks-tests-canned-server-create (verbatim patterns) - "Create a fake SOCKS server and return the process. +(ert-deftest socks-tests-filter-response-parsing-v4 () + "Ensure new chunks added on right (Bug#45162)." + (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") + (socks-filter proc (concat [0 90 0 0 93 184 216])) + ;; From example.com: OK status ^ ^ msg start + (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= (concat [0 90 0 0 93 184 216]) + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch and response fields hold stash w. last chunk") + (should (string= (concat [0 90 0 0 93 184 216 34]) + (process-get proc 'socks-response))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) -`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*")) +(ert-deftest socks-tests-filter-response-parsing-v5 () + "Ensure new chunks added on right (Bug#45162)." + (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") + ;; From fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + ;; 5004 ~~> Version Status (OK) NOOP Addr-Type (4 -> IPv6) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") + (ert-info ("State still waiting and response emtpy") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (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 ("Middle chunk arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State and response fields still untouched") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival appended (on RHS)") + (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 and response fields show last chunk 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))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) + +(defvar socks-tests-canned-server-patterns nil + "Alist containing request/response cons pairs to be tried in order. +Vectors must match verbatim. Strings are considered regex patterns.") + +(defun socks-tests-canned-server-create () + "Create and return a fake SOCKS server." + (let* ((port (nth 2 socks-server)) + (name (format "socks-server:%d" port)) + (pats socks-tests-canned-server-patterns) (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 + (pcase-let ((`(,pat . ,resp) (pop pats))) + (unless (or (and (vectorp pat) (equal pat (vconcat line))) + (string-match-p pat line)) (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)) + (message "[%s] <- %s" name (prin1-to-string line)) + (message "[%s] -> %s" name (prin1-to-string resp))) (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 + (serv (make-network-process :server 1 + :buffer (get-buffer-create name) + :filter filt + :name name + :family 'ipv4 + :host 'local + :coding 'binary + :service port))) + (set-process-query-on-exit-flag serv nil) + serv)) -(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) +(defvar socks-tests--hello-world-http-request-pattern + (cons "^GET /" (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n"))) + +(defun socks-tests-perform-hello-world-http-request () + "Start canned server, validate hello-world response, and finalize." + (let* ((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) + (server (socks-tests-canned-server-create)) ;; done ;; @@ -90,14 +173,91 @@ socks-tests-auth-filter-url-http (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))) + (buf (url-http url cb '(nil))) + (proc (get-buffer-process buf)) + (attempts 10)) + (while (and (not done) (< 0 (cl-decf attempts))) + (sleep-for 0.1)) (should done) (delete-process server) + (delete-process proc) ; otherwise seems client proc is sometimes reused (kill-buffer (process-buffer server)) (kill-buffer buf) (ignore url-gateway-method))) +;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-v5-auth-user-pass () + "Verify correct handling of SOCKS5 user/pass authentication." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-user-agent "Test/auth-user-pass") + (socks-tests-canned-server-patterns + `(([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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Services (like Tor) may be configured without auth but for some +;; reason still prefer the user/pass method over none when offered both. +;; Given this library's defaults, the scenario below is possible. +;; +;; FYI: RFC 1929 doesn't say that a username or password is required +;; but notes that the length of both fields should be at least one. +;; However, both socks.el and curl send zero-length fields (though +;; curl drops the user part too when the password is empty). +;; +;; From Tor's docs /socks-extensions.txt, 1.1 Extent of support: +;; > We allow username/password fields of this message to be empty ... +;; line 41 in blob 5fd1f828f3e9d014f7b65fa3bd1d33c39e4129e2 +;; https://gitweb.torproject.org/torspec.git/tree/socks-extensions.txt +;; +;; To verify against curl 7.71, swap out the first two pattern pairs +;; with ([5 3 0 1 2] . [5 2]) and ([1 0 0] . [1 0]), then run: +;; $ curl verbose -U "foo:" --proxy socks5h://127.0.0.1:10081 example.com + +(ert-deftest socks-tests-v5-auth-user-pass-blank () + "Verify correct SOCKS5 user/pass authentication with empty pass." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10081 5)) + (socks-username "foo") ; defaults to (user-login-name) + (socks-password "") ; simulate user hitting enter when prompted + (url-user-agent "Test/auth-user-pass-blank") + (socks-tests-canned-server-patterns + `(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 0] . [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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Swap out ([5 2 0 1] . [5 0]) with the first pattern below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com + +(ert-deftest socks-tests-v5-auth-none () + "Verify correct handling of SOCKS5 when auth method 0 requested." + (let ((socks-server '("server" "127.0.0.1" 10082 5)) + (socks-authentication-methods (append socks-authentication-methods + nil)) + (url-user-agent "Test/auth-none") + (socks-tests-canned-server-patterns + `(([5 1 0] . [5 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]) + ,socks-tests--hello-world-http-request-pattern))) + (socks-unregister-authentication-method 2) + (should-not (assq 2 socks-authentication-methods)) + (ert-info ("Make HTTP request over SOCKS5 with no auth method") + (socks-tests-perform-hello-world-http-request))) + (should (assq 2 socks-authentication-methods))) + ;;; socks-tests.el ends here -- 2.29.2 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-Use-raw-bytes-for-SOCKS-4-IP-addresses.patch --] [-- Type: text/x-patch, Size: 2853 bytes --] From 9c403d1936ccd63e25488615beb4d2fde29d375f Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 19:41:04 -0800 Subject: [PATCH 2/2] Use raw bytes for SOCKS 4 IP addresses * lisp/net/socks.el: (socks--open-network-stream, socks-send-command): * test/lisp/net/socks-tests.el: (socks-tests-v4-basic): (Bug#46342). --- lisp/net/socks.el | 4 +++- test/lisp/net/socks-tests.el | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 96fafc826b8..1da1d31d678 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -390,6 +390,8 @@ socks-open-connection proc))) (defun socks-send-command (proc command atype address port) + "Send COMMAND to SOCKS service PROC for proxying ADDRESS and PORT. +When ATYPE indicates an IP, param ADDRESS must be given as raw bytes." (let ((addr (cond ((or (= atype socks-address-type-v4) (= atype socks-address-type-v6)) @@ -528,7 +530,7 @@ socks--open-network-stream (setq host (socks-nslookup-host host)) (if (not (listp host)) (error "Could not get IP address for: %s" host)) - (setq host (apply #'format "%c%c%c%c" host)) + (setq host (apply #'unibyte-string host)) socks-address-type-v4) (t socks-address-type-name)))) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index 340a42d79cc..9a2dcba9daf 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -185,6 +185,26 @@ socks-tests-perform-hello-world-http-request (kill-buffer buf) (ignore url-gateway-method))) +;; Unlike curl, socks.el includes the ID field (but otherwise matches): +;; $ curl --proxy socks4://127.0.0.1:1080 example.com + +(ert-deftest socks-tests-v4-basic () + "Show correct preparation of SOCKS4 connect command (Bug#46342)." + (let ((socks-server '("server" "127.0.0.1" 10079 4)) + (url-user-agent "Test/4-basic") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 184 216 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + ;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate ;; against curl 7.71 with the following options: ;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com -- 2.29.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-20 10:13 ` J.P. @ 2021-02-20 11:08 ` Eli Zaretskii 2021-02-20 15:08 ` J.P. 0 siblings, 1 reply; 21+ messages in thread From: Eli Zaretskii @ 2021-02-20 11:08 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Sat, 20 Feb 2021 02:13:27 -0800 > > > Thanks. I wanted to install this, but it failed to apply to the > > master branch. Could you please rebase on the current master branch > > and resubmit? > > Hi Eli, I've gone ahead and rebased atop > > commit c85c8e7d42ae2a5fc95fa7b14257389d8383b34d > Author: Stefan Kangas <stefan@marxist.se> > > Add toolbar for help-mode > > lisp/help-mode.el | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) > > However the src/dest blob hashes (i.e., git-hash-object(1) SHAs in the > diffs' index dead..beef 0644 lines) remain unchanged. Which leads me to > think maybe the failure is related to these being two individual patches > (a "change set"). No, that's not the reason; the changes applied cleanly this time. So this is now installed on the master branch. Running the tests, I see a lot of stuff on the console. Do we really need that? can some of it be shut up? Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-20 11:08 ` Eli Zaretskii @ 2021-02-20 15:08 ` J.P. 2021-02-20 15:19 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: J.P. @ 2021-02-20 15:08 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 [-- Attachment #1: Type: text/plain, Size: 281 bytes --] Eli Zaretskii <eliz@gnu.org> writes: > Running the tests, I see a lot of stuff on the console. Do we really > need that? can some of it be shut up? Sorry about that. I changed it so it's only noisy during interactive sessions. Is that enough or should I go all the way? Thanks. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Mute-noisy-test-fixture-for-socks.el.patch --] [-- Type: text/x-patch, Size: 899 bytes --] From cf8a990b27c91990f7cfa5954e37ebdae67fd42a Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Sat, 20 Feb 2021 06:50:30 -0800 Subject: [PATCH] Mute noisy test fixture for socks.el * test/lisp/net/socks-tests.el: (socks-tests-perform-hello-world-http-request): --- test/lisp/net/socks-tests.el | 1 + 1 file changed, 1 insertion(+) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index 9a2dcba9daf..71bdd74890a 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -173,6 +173,7 @@ socks-tests-perform-hello-world-http-request (goto-char (point-min)) (should (search-forward "Hello World" nil t)) (setq done t))) + (inhibit-message noninteractive) (buf (url-http url cb '(nil))) (proc (get-buffer-process buf)) (attempts 10)) -- 2.29.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-20 15:08 ` J.P. @ 2021-02-20 15:19 ` Eli Zaretskii 0 siblings, 0 replies; 21+ messages in thread From: Eli Zaretskii @ 2021-02-20 15:19 UTC (permalink / raw) To: J.P.; +Cc: 46342 > From: "J.P." <jp@neverwas.me> > Cc: 46342@debbugs.gnu.org > Date: Sat, 20 Feb 2021 07:08:55 -0800 > > > Running the tests, I see a lot of stuff on the console. Do we really > > need that? can some of it be shut up? > > Sorry about that. I changed it so it's only noisy during interactive > sessions. Is that enough or should I go all the way? Thanks. Thanks, installed. And with that, I think we can close this bug? ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 2021-02-20 9:33 ` Eli Zaretskii 2021-02-20 10:13 ` J.P. @ 2021-02-20 10:41 ` J.P. 1 sibling, 0 replies; 21+ messages in thread From: J.P. @ 2021-02-20 10:41 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46342 [-- Attachment #1: Type: text/plain, Size: 170 bytes --] I went ahead and squashed them down for good measure, in case you prefer the single commit. BTW, I meant before/after, not src/dest re the git diff index lines. Thanks. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Use-raw-bytes-for-SOCKS-4-IP-addresses.patch --] [-- Type: text/x-patch, Size: 18152 bytes --] From 5fc7a0404f3d042900309c17f60c46f2da10e050 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" <jp@neverwas.me> Date: Fri, 5 Feb 2021 05:24:55 -0800 Subject: [PATCH] Use raw bytes for SOCKS 4 IP addresses * lisp/net/socks.el: (socks--open-network-stream, socks-send-command): * test/lisp/net/socks-tests.el: (socks-tests-v4-basic): (Bug#46342). (auth-registration-and-suite-offer): Add more auth-related tests. (filter-response-parsing-v4, filter-response-parsing-v5): Assert auth-method selection wrangling and socks-filter parsing. (v5-auth-user-pass, v5-auth-user-pass-blank, v5-auth-none): Show prep and execution of the SOCKS connect command and proxying of an HTTP request; simplify fake server. --- lisp/net/socks.el | 4 +- test/lisp/net/socks-tests.el | 290 ++++++++++++++++++++++++++++------- 2 files changed, 238 insertions(+), 56 deletions(-) diff --git a/lisp/net/socks.el b/lisp/net/socks.el index 96fafc826b8..1da1d31d678 100644 --- a/lisp/net/socks.el +++ b/lisp/net/socks.el @@ -390,6 +390,8 @@ socks-open-connection proc))) (defun socks-send-command (proc command atype address port) + "Send COMMAND to SOCKS service PROC for proxying ADDRESS and PORT. +When ATYPE indicates an IP, param ADDRESS must be given as raw bytes." (let ((addr (cond ((or (= atype socks-address-type-v4) (= atype socks-address-type-v6)) @@ -528,7 +530,7 @@ socks--open-network-stream (setq host (socks-nslookup-host host)) (if (not (listp host)) (error "Could not get IP address for: %s" host)) - (setq host (apply #'format "%c%c%c%c" host)) + (setq host (apply #'unibyte-string host)) socks-address-type-v4) (t socks-address-type-name)))) diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el index b378ed2964e..9a2dcba9daf 100644 --- a/test/lisp/net/socks-tests.el +++ b/test/lisp/net/socks-tests.el @@ -21,68 +21,151 @@ ;;; Code: +(require 'ert) (require 'socks) (require 'url-http) -(defvar socks-tests-canned-server-port nil) +(ert-deftest socks-tests-auth-registration-and-suite-offer () + (ert-info ("Default favors user/pass auth") + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) ; length [offer ...] + (let (socks-authentication-methods) + (ert-info ("Empty selection/no methods offered") + (should (equal "\0" (socks-build-auth-list)))) + (ert-info ("Simulate library defaults") + (socks-register-authentication-method 0 "No authentication" + 'identity) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-register-authentication-method 2 "Username/Password" + 'socks-username/password-auth) + (should (equal socks-authentication-methods + '((2 "Username/Password" . socks-username/password-auth) + (0 "No authentication" . identity)))) + (should (equal "\2\0\2" (socks-build-auth-list)))) + (ert-info ("Removal") + (socks-unregister-authentication-method 2) + (should (equal socks-authentication-methods + '((0 "No authentication" . identity)))) + (should (equal "\1\0" (socks-build-auth-list))) + (socks-unregister-authentication-method 0) + (should-not socks-authentication-methods) + (should (equal "\0" (socks-build-auth-list)))))) -(defun socks-tests-canned-server-create (verbatim patterns) - "Create a fake SOCKS server and return the process. +(ert-deftest socks-tests-filter-response-parsing-v4 () + "Ensure new chunks added on right (Bug#45162)." + (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") + (socks-filter proc (concat [0 90 0 0 93 184 216])) + ;; From example.com: OK status ^ ^ msg start + (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= (concat [0 90 0 0 93 184 216]) + (process-get proc 'socks-scratch))))) + (ert-info ("Last part arrives") + (socks-filter proc "\42") ; ?\" 34 + (ert-info ("State transitions to complete (length check passes)") + (should (eq (process-get proc 'socks-state) socks-state-connected))) + (ert-info ("Scratch and response fields hold stash w. last chunk") + (should (string= (concat [0 90 0 0 93 184 216 34]) + (process-get proc 'socks-response))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) -`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*")) +(ert-deftest socks-tests-filter-response-parsing-v5 () + "Ensure new chunks added on right (Bug#45162)." + (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") + ;; From fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9 + ;; 5004 ~~> Version Status (OK) NOOP Addr-Type (4 -> IPv6) + (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60") + (ert-info ("State still waiting and response emtpy") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (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 ("Middle chunk arrives") + (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9") + (ert-info ("State and response fields still untouched") + (should (eq (process-get proc 'socks-state) socks-state-waiting)) + (should-not (process-get proc 'socks-response))) + (ert-info ("Scratch contains new arrival appended (on RHS)") + (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 and response fields show last chunk 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))) + (should (string= (process-get proc 'socks-response) + (process-get proc 'socks-scratch))))) + (delete-process proc) + (kill-buffer buf))) + +(defvar socks-tests-canned-server-patterns nil + "Alist containing request/response cons pairs to be tried in order. +Vectors must match verbatim. Strings are considered regex patterns.") + +(defun socks-tests-canned-server-create () + "Create and return a fake SOCKS server." + (let* ((port (nth 2 socks-server)) + (name (format "socks-server:%d" port)) + (pats socks-tests-canned-server-patterns) (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 + (pcase-let ((`(,pat . ,resp) (pop pats))) + (unless (or (and (vectorp pat) (equal pat (vconcat line))) + (string-match-p pat line)) (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)) + (message "[%s] <- %s" name (prin1-to-string line)) + (message "[%s] -> %s" name (prin1-to-string resp))) (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 + (serv (make-network-process :server 1 + :buffer (get-buffer-create name) + :filter filt + :name name + :family 'ipv4 + :host 'local + :coding 'binary + :service port))) + (set-process-query-on-exit-flag serv nil) + serv)) -(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) +(defvar socks-tests--hello-world-http-request-pattern + (cons "^GET /" (concat "HTTP/1.1 200 OK\r\n" + "Content-Type: text/plain\r\n" + "Content-Length: 13\r\n\r\n" + "Hello World!\n"))) + +(defun socks-tests-perform-hello-world-http-request () + "Start canned server, validate hello-world response, and finalize." + (let* ((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) + (server (socks-tests-canned-server-create)) ;; done ;; @@ -90,14 +173,111 @@ socks-tests-auth-filter-url-http (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))) + (buf (url-http url cb '(nil))) + (proc (get-buffer-process buf)) + (attempts 10)) + (while (and (not done) (< 0 (cl-decf attempts))) + (sleep-for 0.1)) (should done) (delete-process server) + (delete-process proc) ; otherwise seems client proc is sometimes reused (kill-buffer (process-buffer server)) (kill-buffer buf) (ignore url-gateway-method))) +;; Unlike curl, socks.el includes the ID field (but otherwise matches): +;; $ curl --proxy socks4://127.0.0.1:1080 example.com + +(ert-deftest socks-tests-v4-basic () + "Show correct preparation of SOCKS4 connect command (Bug#46342)." + (let ((socks-server '("server" "127.0.0.1" 10079 4)) + (url-user-agent "Test/4-basic") + (socks-tests-canned-server-patterns + `(([4 1 0 80 93 184 216 34 ?f ?o ?o 0] . [0 90 0 0 0 0 0 0]) + ,socks-tests--hello-world-http-request-pattern)) + socks-nslookup-program) + (ert-info ("Make HTTP request over SOCKS4") + (cl-letf (((symbol-function 'socks-nslookup-host) + (lambda (host) + (should (equal host "example.com")) + (list 93 184 216 34))) + ((symbol-function 'user-full-name) + (lambda () "foo"))) + (socks-tests-perform-hello-world-http-request))))) + +;; Replace first pattern below with ([5 3 0 1 2] . [5 2]) to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com + +(ert-deftest socks-tests-v5-auth-user-pass () + "Verify correct handling of SOCKS5 user/pass authentication." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10080 5)) + (socks-username "foo") + (socks-password "bar") + (url-user-agent "Test/auth-user-pass") + (socks-tests-canned-server-patterns + `(([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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Services (like Tor) may be configured without auth but for some +;; reason still prefer the user/pass method over none when offered both. +;; Given this library's defaults, the scenario below is possible. +;; +;; FYI: RFC 1929 doesn't say that a username or password is required +;; but notes that the length of both fields should be at least one. +;; However, both socks.el and curl send zero-length fields (though +;; curl drops the user part too when the password is empty). +;; +;; From Tor's docs /socks-extensions.txt, 1.1 Extent of support: +;; > We allow username/password fields of this message to be empty ... +;; line 41 in blob 5fd1f828f3e9d014f7b65fa3bd1d33c39e4129e2 +;; https://gitweb.torproject.org/torspec.git/tree/socks-extensions.txt +;; +;; To verify against curl 7.71, swap out the first two pattern pairs +;; with ([5 3 0 1 2] . [5 2]) and ([1 0 0] . [1 0]), then run: +;; $ curl verbose -U "foo:" --proxy socks5h://127.0.0.1:10081 example.com + +(ert-deftest socks-tests-v5-auth-user-pass-blank () + "Verify correct SOCKS5 user/pass authentication with empty pass." + (should (assq 2 socks-authentication-methods)) + (let ((socks-server '("server" "127.0.0.1" 10081 5)) + (socks-username "foo") ; defaults to (user-login-name) + (socks-password "") ; simulate user hitting enter when prompted + (url-user-agent "Test/auth-user-pass-blank") + (socks-tests-canned-server-patterns + `(([5 2 0 2] . [5 2]) + ([1 3 ?f ?o ?o 0] . [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]) + ,socks-tests--hello-world-http-request-pattern))) + (ert-info ("Make HTTP request over SOCKS5 with USER/PASS auth method") + (socks-tests-perform-hello-world-http-request)))) + +;; Swap out ([5 2 0 1] . [5 0]) with the first pattern below to validate +;; against curl 7.71 with the following options: +;; $ curl --verbose --proxy socks5h://127.0.0.1:10082 example.com + +(ert-deftest socks-tests-v5-auth-none () + "Verify correct handling of SOCKS5 when auth method 0 requested." + (let ((socks-server '("server" "127.0.0.1" 10082 5)) + (socks-authentication-methods (append socks-authentication-methods + nil)) + (url-user-agent "Test/auth-none") + (socks-tests-canned-server-patterns + `(([5 1 0] . [5 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]) + ,socks-tests--hello-world-http-request-pattern))) + (socks-unregister-authentication-method 2) + (should-not (assq 2 socks-authentication-methods)) + (ert-info ("Make HTTP request over SOCKS5 with no auth method") + (socks-tests-perform-hello-world-http-request))) + (should (assq 2 socks-authentication-methods))) + ;;; socks-tests.el ends here -- 2.29.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-02-20 15:19 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-06 11:46 bug#46342: 28.0.50; socks-send-command munges IP address bytes to UTF-8 J.P. 2021-02-06 12:26 ` Eli Zaretskii 2021-02-06 14:19 ` J.P. 2021-02-06 15:15 ` Eli Zaretskii 2021-02-07 14:22 ` J.P. 2021-02-09 15:17 ` J.P. 2021-02-09 16:14 ` Eli Zaretskii 2021-02-10 13:16 ` J.P. 2021-02-10 16:04 ` Eli Zaretskii 2021-02-11 14:58 ` J.P. 2021-02-11 15:28 ` Eli Zaretskii 2021-02-12 14:30 ` J.P. 2021-02-12 15:04 ` Eli Zaretskii 2021-02-13 15:43 ` J.P. 2021-02-17 14:59 ` J.P. 2021-02-20 9:33 ` Eli Zaretskii 2021-02-20 10:13 ` J.P. 2021-02-20 11:08 ` Eli Zaretskii 2021-02-20 15:08 ` J.P. 2021-02-20 15:19 ` Eli Zaretskii 2021-02-20 10:41 ` J.P.
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).