Broken display of clearsigned PGP message

Broken display of clearsigned PGP message

[Andreas Seltenreich]

There is a report in de.comm.software.gnus about a clearsigned PGP
message being displayed incorrectly (<URL:news:874putv834.fsf@sopos.org>).
It turned out mm-uu-pgp-signed-extract-1 stumbled across whitespace
contained in the line following the Hash header of the message:

--8<---------------cut here---------------start------------->8---
\-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
[signed text]
--8<---------------cut here---------------end--------------->8---

I'm not sure if this message is legal according to RFC 2440.  In 6.2
it states that Armor Headers are followed by a blank line, which is
defined as "zero-length, or containing only whitespace", however in
7. it uses the term "empty line" when describing Armor Headers in
cleartext signatures.

IMHO we should follow "be liberal in what you accept" here, especially
since GnuPG considers the message legal.  Should I apply the attached
patch to v5-10 and HEAD?  Can anyone imagine regressions?

diff -u -r6.29.2.24 mm-uu.el
--- mm-uu.el	28 Apr 2006 05:17:32 -0000	6.29.2.24
+++ mm-uu.el	27 Sep 2006 16:28:24 -0000
@@ -373,7 +373,7 @@
 	   mm-security-handle 'gnus-details
 	   (format "Clear verification not supported by `%s'.\n" mml2015-use))))
       (goto-char (point-min))
-      (if (search-forward "\n\n" nil t)
+      (if (re-search-forward "\n[\t ]*\n" nil t)
 	  (delete-region (point-min) (point)))
       (if (re-search-forward mm-uu-pgp-beginning-signature nil t)
 	  (delete-region (match-beginning 0) (point-max)))

Re: Broken display of clearsigned PGP message

[Andreas Seltenreich]

I wrote:

[...]
> message being displayed incorrectly (<URL:news:874putv834.fsf@sopos.org>).
[...]
> I'm not sure if this message is legal according to RFC 2440.  In 6.2
> it states that Armor Headers are followed by a blank line, which is
> defined as "zero-length, or containing only whitespace", however in
> 7. it uses the term "empty line" when describing Armor Headers in
> cleartext signatures.
>
> IMHO we should follow "be liberal in what you accept" here, especially
> since GnuPG considers the message legal.  Should I apply the attached
> patch to v5-10 and HEAD?  Can anyone imagine regressions?

I just realized that with the current behavior, an attacker could
completely replace the text within Gnus' signed-message markup with
his own message without interfering with the verification process.  So
I guess it is hard to make the situation worse...  Will commit the
patch.

regards,
andreas

Re: Broken display of clearsigned PGP message

[Reiner Steib]

On Fri, Oct 13 2006, Andreas Seltenreich wrote:

>> I'm not sure if this message is legal according to RFC 2440.  In 6.2
>> it states that Armor Headers are followed by a blank line, which is
>> defined as "zero-length, or containing only whitespace", however in
>> 7. it uses the term "empty line" when describing Armor Headers in
>> cleartext signatures.
>>
>> IMHO we should follow "be liberal in what you accept" here, especially
>> since GnuPG considers the message legal.  Should I apply the attached
>> patch to v5-10 and HEAD?  Can anyone imagine regressions?
>
> I just realized that with the current behavior, an attacker could
> completely replace the text within Gnus' signed-message markup with
> his own message without interfering with the verification process.  So
> I guess it is hard to make the situation worse...  Will commit the
> patch.

Thanks.

I'd suggest to add a rather detailed explanation about this problem
and the relevant RFC in `mm-uu-pgp-signed-extract-1'.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Broken display of clearsigned PGP message

[Andreas Seltenreich]

Reiner Steib writes:

> I'd suggest to add a rather detailed explanation about this problem
> and the relevant RFC in `mm-uu-pgp-signed-extract-1'.

Ok, I've added a comment and also made the fix a bit more thorough -
Instead of stripping everything until we see what we think is
separating the armor headers, we're now stripping everything that we
know for sure isn't part of the signed text.  That may be a bit
paranoid, but I've heard this is a must in the security business ;-).

regards,
andreas

*** mm-uu.el	28 Sep 2006 03:16:58 +0200	6.29.2.25
--- mm-uu.el	18 Nov 2006 04:03:40 +0100	
***************
*** 373,380 ****
  	   mm-security-handle 'gnus-details
  	   (format "Clear verification not supported by `%s'.\n" mml2015-use))))
        (goto-char (point-min))
!       (if (re-search-forward "\n[\t ]*\n" nil t)
! 	  (delete-region (point-min) (point)))
        (if (re-search-forward mm-uu-pgp-beginning-signature nil t)
  	  (delete-region (match-beginning 0) (point-max)))
        (goto-char (point-min))
--- 373,388 ----
  	   mm-security-handle 'gnus-details
  	   (format "Clear verification not supported by `%s'.\n" mml2015-use))))
        (goto-char (point-min))
!       (forward-line)
!       ;; We need to be careful not to strip beyond the armor headers.
!       ;; Previously, an attacker could replace the text inside our
!       ;; markup with trailing garbage by injecting whitespace into the
!       ;; message.
!       (while (looking-at "Hash:") ; The only header allowed in cleartext
! 	(forward-line))		  ; signatures according to RFC2440.
!       (when (looking-at "[\t ]*$")
! 	(forward-line))
!       (delete-region (point-min) (point))
        (if (re-search-forward mm-uu-pgp-beginning-signature nil t)
  	  (delete-region (match-beginning 0) (point-max)))
        (goto-char (point-min))