bug#36233: 26.2; Tokenization error in rcirc parser

bug#36233: 26.2; Tokenization error in rcirc parser

[Jeff Johnson]

I'm reporting a bug that has to do with unused IRC
protocol colon escaping combined with the use of
certain unusual but not disallowed characters in 
channel keys.

It should be noted that certain IRC servers that
always colon escape the last token, (pedantic, but
certainly valid per the IRC RFC's) often triggers a class
of bugs, to most of which rcirc is immune, except when
combing the use of ':' in a channel key, which leads the
to the channel key being stripped of the ':' but not
final argument.

Example, if the server sends:
"MODE #cchan +kl a:b :999" 

That is somehow parsed by rcirc as:
"MODE +kl a b :999"

Clearly incorrect - it should be "MODE +kl a:b 999".

The colon *is* allowed as part of the channel key (as it
is not disallowed in the RFC), except, of course, as the
first character of the key, where it would break protocol.

ircrc client does parse the case of "MODE #chan +k :a:b" 
correctly, yet fails for "MODE #chan +kl a:b :999".

An example of an ircd that always escapes the final token 
and does not disallow colons in channel keys is inspircd, 
but there are many others.  inspircd exactly follows the
RFC - pedantically so - but does not conform to historical
behavior - only standards compliance. 

Inspircd has a testnet at testnet.inspircd.org which can be    
easily used to reproduce the behavior reported.

I've tested that this bugs exists and is reproducible on both
Emacs 26.2 and 25.1 with the included rcirc.

--  
Jeff Johnson
trnsz@pobox.com

bug#36233: 26.2; Tokenization error in rcirc parser

[Noam Postavsky]

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

severity 36233 minor
tags 36233 + patch
quit

"Jeff Johnson" <trnsz@pobox.com> writes:

> Example, if the server sends:
> "MODE #cchan +kl a:b :999" 
>
> That is somehow parsed by rcirc as:
> "MODE +kl a b :999"

Yeah, it was using [^:]* to match the middle args.  Patch below.  I
added a test for this case in the patch, although this could probably
use some more testing to make sure I haven't broken other cases.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 5104 bytes --]

From 9b0cb9e737e3c68f6553f1995983f524bdd92453 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 16 Jun 2019 13:48:56 -0400
Subject: [PATCH] Make rcirc parsing more RFC2812 compliant (Bug#36233)

Do continue to allow multiple spaces between arguments, even though
that is technically not allowed by the RFC.
* lisp/net/rcirc.el (rcirc-process-server-response-1): Fix parsing of
arguments which contain colons.
* test/lisp/net/rcirc-tests.el: New test.
---
 lisp/net/rcirc.el            | 25 ++++++++++++++-------
 test/lisp/net/rcirc-tests.el | 52 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 8 deletions(-)
 create mode 100644 test/lisp/net/rcirc-tests.el

diff --git a/lisp/net/rcirc.el b/lisp/net/rcirc.el
index 8926772b94..d81fa939b9 100644
--- a/lisp/net/rcirc.el
+++ b/lisp/net/rcirc.el
@@ -774,22 +774,31 @@ (defun rcirc-process-server-response (process text)
     (rcirc-process-server-response-1 process text)))
 
 (defun rcirc-process-server-response-1 (process text)
-  (if (string-match "^\\(:\\([^ ]+\\) \\)?\\([^ ]+\\) \\(.+\\)$" text)
+  ;; See https://tools.ietf.org/html/rfc2812#section-2.3.1.  We accept
+  ;; multiple spaces between args, even though the RFC doesn't allow
+  ;; that.
+  (if (string-match "^\\(:\\([^ ]+\\) \\)?\\([^ ]+\\)" text)
       (let* ((user (match-string 2 text))
 	     (sender (rcirc-user-nick user))
              (cmd (match-string 3 text))
-             (args (match-string 4 text))
+             (cmd-end (match-end 3))
+             (args nil)
              (handler (intern-soft (concat "rcirc-handler-" cmd))))
-        (string-match "^\\([^:]*\\):?\\(.+\\)?$" args)
-        (let* ((args1 (match-string 1 args))
-               (args2 (match-string 2 args))
-               (args (delq nil (append (split-string args1 " " t)
-				       (list args2)))))
+        (cl-loop with i = cmd-end
+                 repeat 14
+                 while (eql i (string-match " +\\([^: ][^ ]*\\)" text i))
+                 do (progn (push (match-string 1 text) args)
+                           (setq i (match-end 0)))
+                 finally
+                 (progn (if (eql i (string-match " +:?" text i))
+                            (push (substring text (match-end 0)) args)
+                          (cl-assert (= i (length text))))
+                        (cl-callf nreverse args)))
         (if (not (fboundp handler))
             (rcirc-handler-generic process cmd sender args text)
           (funcall handler process sender args text))
         (run-hook-with-args 'rcirc-receive-message-functions
-                            process cmd sender args text)))
+                            process cmd sender args text))
     (message "UNHANDLED: %s" text)))
 
 (defvar rcirc-responses-no-activity '("305" "306")
diff --git a/test/lisp/net/rcirc-tests.el b/test/lisp/net/rcirc-tests.el
new file mode 100644
index 0000000000..128cb2e754
--- /dev/null
+++ b/test/lisp/net/rcirc-tests.el
@@ -0,0 +1,52 @@
+;;; rcirc-tests.el --- Tests for rcirc -*- lexical-binding:t -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; This program is free software: you can redistribute it and/or
+;; modify it under the terms of the GNU General Public License as
+;; published by the Free Software Foundation, either version 3 of the
+;; License, or (at your option) any later version.
+;;
+;; This program is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; General Public License for more details.
+;;
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see `https://www.gnu.org/licenses/'.
+
+;;; Code:
+
+(require 'ert)
+(require 'rcirc)
+(require 'cl-lib)
+
+(defun rcirc-tests--parse-server-response (cmd text)
+  (cl-letf* ((received-args nil)
+             ((symbol-function (intern (concat "rcirc-handler-" cmd)))
+              (lambda (_process sender args text)
+                (setq received-args (list sender cmd args text))))
+             (rcirc-receive-message-functions nil)
+             (rcirc-trap-errors-flag nil))
+    (rcirc-process-server-response nil text)
+    received-args))
+
+(defmacro rcirc-tests--server-response-parse-should-be
+    (text expected-sender expected-cmd expected-args)
+  (declare (debug t))
+  (macroexp-let2* nil ((cmd expected-cmd))
+    `(should (equal (rcirc-tests--parse-server-response ,cmd ,text)
+                    (list ,expected-sender ,cmd ,expected-args ,text)))))
+
+(ert-deftest rcirc-process-server-response ()
+  (rcirc-tests--server-response-parse-should-be
+   "MODE #cchan +kl a:b :999"
+   nil "MODE" '("#cchan" "+kl" "a:b" "999"))
+  (rcirc-tests--server-response-parse-should-be
+   "MODE #cchan +kl a:b 999"
+   nil "MODE" '("#cchan" "+kl" "a:b" "999"))
+  (rcirc-tests--server-response-parse-should-be
+    "MODE #cchan +kl :a:b"
+    nil "MODE" '("#cchan" "+kl" "a:b")))
+
+;;; rcirc-tests.el ends here
-- 
2.11.0

bug#36233: 26.2; Tokenization error in rcirc parser

[trnsz]

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

On Jun 16, 2019, 4:10 PM -0400, Noam Postavsky , wrote:
> Yeah, it was using [^:]* to match the middle args. Patch below. I added a test for this case in the patch, although this could probably use some more testing to make sure I haven't broken other cases.

I can confirm this fix works, but I've not done extensive testing (I'm not normally an rcirc user).

Thanks for addressing this so quickly and I look forward to seeing the fix in a future release.

--
Jeffrey H. Johnson
jhj@trnsz.com

[-- Attachment #2: Type: text/html, Size: 1170 bytes --]

bug#36233: 26.2; Tokenization error in rcirc parser

[Noam Postavsky]

tags 36233 fixed
close 36233 27.1
quit

trnsz@pobox.com writes:

> On Jun 16, 2019, 4:10 PM -0400, Noam Postavsky , wrote:

> I can confirm this fix works, but I've not done extensive testing (I'm not normally an rcirc user).

Okay, I've run with it for a week and nothing bad happened (though I'm
not a very advanced IRC user, I just hang out in #emacs on freenode).
I'll push to master and we'll soon see if there are any prolems.

f46b16b9fb 2019-06-22T19:25:44-04:00 "Make rcirc parsing more RFC2812 compliant (Bug#36233)"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=f46b16b9fb00d341f222422a9514f5bd62f29971