Re: master eaf224f: Repad the Face header in Gnus

Re: master eaf224f: Repad the Face header in Gnus

[Robert Pluim]

>>>>> On Mon, 28 Sep 2020 08:09:18 -0400 (EDT), larsi@gnus.org (Lars Ingebrigtsen) said:
    Lars> +  (let ((splitstr (split-string str "[\r\n]" t)))
    Lars> +    (when (and reject-newlines (> (length splitstr) 1))
    Lars> +      (error "Invalid Base64 string"))
    Lars> +    (dolist (substr splitstr)
    Lars> +      (when (and line-length (> (length substr) line-length))
    Lars> +	(error "Base64 string exceeds line-length"))
    Lars> +      (when (string-match "[^A-Za-z0-9+/=]" substr)
    Lars> +	(error "Invalid Base64 string")))

This signals an error because at this point 'substr' is a line of the
Face header with a leading space. Any of Lars' messages will trigger it.

Robert

Re: master eaf224f: Repad the Face header in Gnus

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> This signals an error because at this point 'substr' is a line of the
> Face header with a leading space. Any of Lars' messages will trigger it.

I pushed a fix a couple of hours ago...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master eaf224f: Repad the Face header in Gnus

[Robert Pluim]

>>>>> On Tue, 29 Sep 2020 17:03:56 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> This signals an error because at this point 'substr' is a line of the
    >> Face header with a leading space. Any of Lars' messages will trigger it.

    Lars> I pushed a fix a couple of hours ago...

So you did. Am I going to have to 'git pull' multiple times a day now?

Robert
-- 

Re: master eaf224f: Repad the Face header in Gnus

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Tue, 29 Sep 2020 17:11:12 +0200
> Cc: Alex Bochannek <alex@bochannek.com>, emacs-devel@gnu.org
> 
> Am I going to have to 'git pull' multiple times a day now?

Every 5 minutes, more like it.

Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Robert Pluim <rpluim@gmail.com> writes:
>
>> This signals an error because at this point 'substr' is a line of the
>> Face header with a leading space. Any of Lars' messages will trigger it.
>
> I pushed a fix a couple of hours ago...

Thanks Robert for finding the failure and thanks Lars for responding so
quickly!

This is my bad for not properly testing for the folding whitespace
continuation in long header fields. While spaces are illegal characters
in base 64, which is what I was testing for, it makes sense to deal with
potential FWSP sequences first since this function is specifically for
Gnus.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix folding whitespace in gnus-base64-repad --]
[-- Type: text/x-patch, Size: 817 bytes --]

diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index 0e15ebce6c..1cf6bb7053 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -1368,7 +1368,11 @@ gnus-base64-repad
   ;;   input (3.1, 3.3)
   ;; - if line-length is set, error on input exceeding the limit (3.1)
   ;; - reject characters outside base encoding (3.3, also section 12)
-  (let ((splitstr (split-string str "[\n\r \t]+" t)))
+  ;;
+  ;; RFC 5322 section 2.2.3 consideration:
+  ;; Because base 64-encoded strings can appear in long header fields, remove
+  ;; folding whitespace while still observing the RFC 4648 decisions above.
+  (let ((splitstr (split-string str "[ \t]*[\r\n]+[ \t]?" t)))
     (when (and reject-newlines (> (length splitstr) 1))
       (error "Invalid Base64 string"))
     (dolist (substr splitstr)

[-- Attachment #3: Type: text/plain, Size: 752 bytes --]


The above is a bit stricter than the latest commit by Lars: SP and HTAB
can occur before a CRLF and there can only be one SP or HTAB on the next
line. I am not entirely sure how end-of-line works here cross-platform
and whether the string will still use the convention of the message
format or the local convention on the receiving end, so [\r\n]+ seems
the safest.

The fix Lars submitted is more lenient and I am OK with that one too, of
course. I just felt since I made the original contribution, it is my
responsibility to offer a fix as well.

I also updated the tests accordingly. Please note that this
implementation /will/ fail a whitespace if it isn't followed by an
end-of-line. While updating that test, I cleaned up the other ones a
bit.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Fix tests for folding whitespace in gnus-base64-repad --]
[-- Type: text/x-patch, Size: 1467 bytes --]

diff --git a/test/lisp/gnus/gnus-util-tests.el b/test/lisp/gnus/gnus-util-tests.el
index ec58032e84..47f0a9cf76 100644
--- a/test/lisp/gnus/gnus-util-tests.el
+++ b/test/lisp/gnus/gnus-util-tests.el
@@ -151,8 +151,10 @@ gnus-base64-repad
   (should (equal "Zg==" (gnus-base64-repad "Zg")))
   (should (equal "Zg==" (gnus-base64-repad "Zg====")))
 
-  (should (equal (gnus-base64-repad " ") ""))
-  (should (equal (gnus-base64-repad "Zg== ") "Zg=="))
+  (should-error (gnus-base64-repad " ")
+                :type 'error)
+  (should-error (gnus-base64-repad "Zg== ")
+                :type 'error)
   (should-error (gnus-base64-repad "Z?\x00g==")
                 :type 'error)
   ;; line-length
@@ -162,9 +164,10 @@ gnus-base64-repad
   (should-error (gnus-base64-repad "Zm9v\r\nYmFy" t)
                 :type 'error)
   (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9vYmFy" t)))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy" nil)))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy\n" nil)))
-  (should (equal (gnus-base64-repad "Zm9v\r\n YmFy\r\n" nil) "Zm9vYmFy"))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy")))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy\n")))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\n YmFy\r\n")))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v \r\n\tYmFy")))
   (should-error (gnus-base64-repad "Zm9v\r\nYmFy" nil 3)
                 :type 'error))
 

[-- Attachment #5: Type: text/plain, Size: 149 bytes --]


Let me know if you have any concerns about this or want me to make any
additional changes. Thanks again for finding the failure, Robert!

-- 
Alex.

Re: master eaf224f: Repad the Face header in Gnus

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> I also updated the tests accordingly. Please note that this
> implementation /will/ fail a whitespace if it isn't followed by an
> end-of-line. While updating that test, I cleaned up the other ones a
> bit.

But the use case here is an end-of-line followed by whitespace...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> I also updated the tests accordingly. Please note that this
>> implementation /will/ fail a whitespace if it isn't followed by an
>> end-of-line. While updating that test, I cleaned up the other ones a
>> bit.
>
> But the use case here is an end-of-line followed by whitespace...

I meant a trailing whitespace by itself will fail because it isn't
unfolded and then constitutes a invalid base 64 character.

  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\n YmFy\r\n")))
  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v \r\n\tYmFy")))

should cover the header field folding cases and

  (should-error (gnus-base64-repad "Zg== ")
                :type 'error)

covers a case for an invalid character in the base 64-string.

-- 
Alex.

Re: master eaf224f: Repad the Face header in Gnus

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> I meant a trailing whitespace by itself will fail because it isn't
> unfolded and then constitutes a invalid base 64 character.

Ah, I see.  OK; now applied the patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> I meant a trailing whitespace by itself will fail because it isn't
>> unfolded and then constitutes a invalid base 64 character.
>
> Ah, I see.  OK; now applied the patch.

Thank you, Lars.

I went through a few years of Gnus newsgroups and other mailing lists to
see if I can find Face headers that break. I found two people who posted
invalid base 64 with embedded space characters. One case (a) was a
simple issue of improper header folding. The other case (b) is an odd
corner case where there is a single space right in a folded header field
and the proper padding assigned as well. How about something like this
to make case (a) display properly and not fail for case (b)?


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

diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index 1cf6bb7053..6d348e290e 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -1343,6 +1343,16 @@ gnus-url-unhex-string
     (setq tmp (concat tmp str))
     tmp))
 
+(defcustom gnus-base64-repad-regexp "[A-Za-z0-9+/= ]"
+  "Regular expression defining valid base 64-encoded character strings
+for the gnus-base64-repad function, which is used for Face-headers.
+By default the space character is included even though it is invalid
+in base 64 because it does occasionally (and incorrectly) occur in
+Face-headers."
+  :type 'regexp
+  :version "28.0"
+  :group 'gnus-various)
+
 (defun gnus-base64-repad (str &optional reject-newlines line-length)
   "Take a base 64-encoded string and return it padded correctly.
 Existing padding is ignored.
@@ -1378,7 +1388,8 @@ gnus-base64-repad
     (dolist (substr splitstr)
       (when (and line-length (> (length substr) line-length))
 	(error "Base64 string exceeds line-length"))
-      (when (string-match "[^A-Za-z0-9+/=]" substr)
+      (when (not (string= ""
+	     (replace-regexp-in-string gnus-base64-repad-regexp "" substr)))
 	(error "Invalid Base64 string")))
     (let* ((str (string-join splitstr))
 	   (len (length str)))

[-- Attachment #3: Type: text/plain, Size: 149 bytes --]


If this approach makes sense to you, I will refine it as I update the
tests. Thanks for your patience with this, I really appreciate it!

-- 
Alex.

Re: master eaf224f: Repad the Face header in Gnus

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> I went through a few years of Gnus newsgroups and other mailing lists to
> see if I can find Face headers that break. I found two people who posted
> invalid base 64 with embedded space characters. One case (a) was a
> simple issue of improper header folding. The other case (b) is an odd
> corner case where there is a single space right in a folded header field
> and the proper padding assigned as well. How about something like this
> to make case (a) display properly and not fail for case (b)?

[...]

> +(defcustom gnus-base64-repad-regexp "[A-Za-z0-9+/= ]"

I don't think this is necessary -- nobody would want to customise
something like this.

The philosophy in network communication is "be liberal in what you
receive", so when handling base64, all whitespace should simply be
discarded.  There is no downside to doing so, and trying to make base64
decoding be strict here is unnecessary.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> I went through a few years of Gnus newsgroups and other mailing lists to
>> see if I can find Face headers that break. I found two people who posted
>> invalid base 64 with embedded space characters. One case (a) was a
>> simple issue of improper header folding. The other case (b) is an odd
>> corner case where there is a single space right in a folded header field
>> and the proper padding assigned as well. How about something like this
>> to make case (a) display properly and not fail for case (b)?
>
> [...]
>
>> +(defcustom gnus-base64-repad-regexp "[A-Za-z0-9+/= ]"
>
> I don't think this is necessary -- nobody would want to customise
> something like this.

Follow-up on this from last year. I didn't get a chance to fully
implement the functionality and I believe this code path is now
disabled. I can pick it up again if you would like me to. If I can get
some more test emails that would make it a bit easier to handle the
corner cases.

-- 
Alex.

Re: master eaf224f: Repad the Face header in Gnus

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> Follow-up on this from last year. I didn't get a chance to fully
> implement the functionality and I believe this code path is now
> disabled. I can pick it up again if you would like me to. If I can get
> some more test emails that would make it a bit easier to handle the
> corner cases.

I don't think validating/fixing/etc the base64 in the Face header really
makes that much sense -- we should be liberal in what we receive, and
liberalness in the base64 world isn't something that needs customisation.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> Follow-up on this from last year. I didn't get a chance to fully
>> implement the functionality and I believe this code path is now
>> disabled. I can pick it up again if you would like me to. If I can get
>> some more test emails that would make it a bit easier to handle the
>> corner cases.
>
> I don't think validating/fixing/etc the base64 in the Face header really
> makes that much sense -- we should be liberal in what we receive, and
> liberalness in the base64 world isn't something that needs customisation.

OK, agreed. Do you want me to submit a patch to revert that code and the
tests?

-- 
Alex.

Re: master eaf224f: Repad the Face header in Gnus

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> OK, agreed. Do you want me to submit a patch to revert that code and the
> tests?

I think that might make sense, but I have no strong feelings about the
matter.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Alex Bochannek <alex@bochannek.com> writes:
>
>> OK, agreed. Do you want me to submit a patch to revert that code and the
>> tests?
>
> I think that might make sense, but I have no strong feelings about the
> matter.

Here's a patch that reverts that code and the tests.


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

diff --git a/lisp/gnus/gnus-fun.el b/lisp/gnus/gnus-fun.el
index c2e72aba93..8bca4ffe38 100644
--- a/lisp/gnus/gnus-fun.el
+++ b/lisp/gnus/gnus-fun.el
@@ -206,12 +206,11 @@ gnus-face-encode
 (defun gnus-convert-face-to-png (face)
   "Convert FACE (which is base64-encoded) to a PNG.
 The PNG is returned as a string."
-  (let ((face (gnus-base64-repad face nil nil t)))
-    (mm-with-unibyte-buffer
-      (insert face)
-      (ignore-errors
-	(base64-decode-region (point-min) (point-max)))
-      (buffer-string))))
+  (mm-with-unibyte-buffer
+    (insert face)
+    (ignore-errors
+      (base64-decode-region (point-min) (point-max)))
+    (buffer-string)))
 
 ;;;###autoload
 (defun gnus-convert-png-to-face (file)
diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index e558f639e4..0f4df34740 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -1,3 +1,4 @@
+
 ;;; gnus-util.el --- utility functions for Gnus  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 1996-2021 Free Software Foundation, Inc.
@@ -1291,61 +1292,6 @@ gnus-url-unhex-string
     (setq tmp (concat tmp str))
     tmp))
 
-(defun gnus-base64-repad (str &optional reject-newlines line-length no-check)
-  "Take a base 64-encoded string and return it padded correctly.
-Existing padding is ignored.
-
-If any combination of CR and LF characters are present and
-REJECT-NEWLINES is nil, remove them; otherwise raise an error.
-If LINE-LENGTH is set and the string (or any line in the string
-if REJECT-NEWLINES is nil) is longer than that number, raise an
-error.  Common line length for input characters are 76 plus CRLF
-\(RFC 2045 MIME), 64 plus CRLF (RFC 1421 PEM), and 1000 including
-CRLF (RFC 5321 SMTP).
-
-If NOCHECK, don't check anything, but just repad."
-  ;; RFC 4648 specifies that:
-  ;; - three 8-bit inputs make up a 24-bit group
-  ;; - the 24-bit group is broken up into four 6-bit values
-  ;; - each 6-bit value is mapped to one character of the base 64 alphabet
-  ;; - if the final 24-bit quantum is filled with only 8 bits the output
-  ;;   will be two base 64 characters followed by two "=" padding characters
-  ;; - if the final 24-bit quantum is filled with only 16 bits the output
-  ;;   will be three base 64 character followed by one "=" padding character
-  ;;
-  ;; RFC 4648 section 3 considerations:
-  ;; - if reject-newlines is nil (default), concatenate multi-line
-  ;;   input (3.1, 3.3)
-  ;; - if line-length is set, error on input exceeding the limit (3.1)
-  ;; - reject characters outside base encoding (3.3, also section 12)
-  ;;
-  ;; RFC 5322 section 2.2.3 consideration:
-  ;; Because base 64-encoded strings can appear in long header fields, remove
-  ;; folding whitespace while still observing the RFC 4648 decisions above.
-  (when no-check
-    (setq str (replace-regexp-in-string "[\n\r \t]+" "" str)));
-  (let ((splitstr (split-string str "[ \t]*[\r\n]+[ \t]?" t)))
-    (when (and reject-newlines (> (length splitstr) 1))
-      (error "Invalid Base64 string"))
-    (dolist (substr splitstr)
-      (when (and line-length (> (length substr) line-length))
-	(error "Base64 string exceeds line-length"))
-      (when (string-match "[^A-Za-z0-9+/=]" substr)
-	(error "Invalid Base64 string")))
-    (let* ((str (string-join splitstr))
-	   (len (length str)))
-      (when (string-match "=" str)
-	(setq len (match-beginning 0)))
-      (concat
-       (substring str 0 len)
-       (make-string (/
-		     (- 24
-			(pcase (mod (* len 6) 24)
-			  (`0 24)
-			  (n n)))
-		     6)
-		    ?=)))))
-
 (defun gnus-make-predicate (spec)
   "Transform SPEC into a function that can be called.
 SPEC is a predicate specifier that contains stuff like `or', `and',
diff --git a/test/lisp/gnus/gnus-util-tests.el b/test/lisp/gnus/gnus-util-tests.el
index 959be7987d..f8d30f6373 100644
--- a/test/lisp/gnus/gnus-util-tests.el
+++ b/test/lisp/gnus/gnus-util-tests.el
@@ -132,41 +132,4 @@ gnus-setdiff
   (should (equal '("1") (gnus-setdiff '(2 "1" 2) '(2))))
   (should (equal '("1" "1") (gnus-setdiff '(2 "1" 2 "1") '(2)))))
 
-(ert-deftest gnus-base64-repad ()
-  (should-error (gnus-base64-repad 1)
-                :type 'wrong-type-argument)
-
-  ;; RFC4648 test vectors
-  (should (equal "" (gnus-base64-repad "")))
-  (should (equal "Zg==" (gnus-base64-repad "Zg==")))
-  (should (equal "Zm8=" (gnus-base64-repad "Zm8=")))
-  (should (equal "Zm9v" (gnus-base64-repad "Zm9v")))
-  (should (equal "Zm9vYg==" (gnus-base64-repad "Zm9vYg==")))
-  (should (equal "Zm9vYmE=" (gnus-base64-repad "Zm9vYmE=")))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9vYmFy")))
-
-  (should (equal "Zm8=" (gnus-base64-repad "Zm8")))
-  (should (equal "Zg==" (gnus-base64-repad "Zg")))
-  (should (equal "Zg==" (gnus-base64-repad "Zg====")))
-
-  (should-error (gnus-base64-repad " ")
-                :type 'error)
-  (should-error (gnus-base64-repad "Zg== ")
-                :type 'error)
-  (should-error (gnus-base64-repad "Z?\x00g==")
-                :type 'error)
-  ;; line-length
-  (should-error (gnus-base64-repad "Zg====" nil 4)
-                :type 'error)
-  ;; reject-newlines
-  (should-error (gnus-base64-repad "Zm9v\r\nYmFy" t)
-                :type 'error)
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9vYmFy" t)))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy")))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy\n")))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\n YmFy\r\n")))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v \r\n\tYmFy")))
-  (should-error (gnus-base64-repad "Zm9v\r\nYmFy" nil 3)
-                :type 'error))
-
 ;;; gnustest-gnus-util.el ends here

[-- Attachment #3: Type: text/plain, Size: 11 bytes --]


-- 
Alex.

Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

Alex Bochannek <alex@bochannek.com> writes:

> --- a/lisp/gnus/gnus-util.el
> +++ b/lisp/gnus/gnus-util.el
> @@ -1,3 +1,4 @@
> +
>  ;;; gnus-util.el --- utility functions for Gnus  -*- lexical-binding: t; -*-

That blank line is incorrect, of course. Not sure how that made it
through. Sorry about that.

-- 
Alex.

Re: master eaf224f: Repad the Face header in Gnus

[Lars Ingebrigtsen]

Alex Bochannek <alex@bochannek.com> writes:

> Here's a patch that reverts that code and the tests.

Thanks; applied to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no