pgg-output-buffer gets created as a unibyte buffer

pgg-output-buffer gets created as a unibyte buffer

[Katsumi Yamaoka]

Hi,

I'm using latest No Gnus.  I noticed decrypting messages which
contain text encoded by utf-8 or by shift_jis fails on a certain
situation.  It occurs if the purpose to use pgg for the first
time is to verify a signed message, and never recovers afterward.
The cause is that `pgg-output-buffer' is created as a unibyte
buffer first.  It is reused and never killed.

When decrypting messages, decrypted data are copied into the
other multibyte buffer using:

(insert-buffer-substring pgg-output-buffer)

At that time, the contents will be broken if there are 8-bit data
because a unibyte string is inserted into a multibyte buffer.  I
tested it only with GnuPG, and I have no idea to fix the problem.
The workaround is to kill the " *PGG output*" buffer manually.

Regards,

Re: pgg-output-buffer gets created as a unibyte buffer

[Daiki Ueno]

>>>>> In <b4maca88q6i.fsf@jpl.org> 
>>>>>	Katsumi Yamaoka <yamaoka@jpl.org> wrote:
> I'm using latest No Gnus.

Try v5-10, and if your problem is not fixed, please remove me from Cc:.
-- 
Daiki Ueno

Re: pgg-output-buffer gets created as a unibyte buffer

[Katsumi Yamaoka]

>>>>> In <def1aabc-69b9-4b1d-bb84-e65c63540eac@well-done.deisui.org>
>>>>>	Daiki Ueno wrote:

> Try v5-10, and if your problem is not fixed, please remove me from Cc:.

It happens with v5-10 as well.  Okay, I'll fix it by myself.

Re: pgg-output-buffer gets created as a unibyte buffer

[Katsumi Yamaoka]

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

>>>>> In <b4mmze82cse.fsf@jpl.org> Katsumi Yamaoka wrote:

>>>>>> In <def1aabc-69b9-4b1d-bb84-e65c63540eac@well-done.deisui.org>
>>>>>>	Daiki Ueno wrote:

>> Try v5-10, and if your problem is not fixed, please remove me from Cc:.

> It happens with v5-10 as well.  Okay, I'll fix it by myself.

The cause is that the multibyteness of `pgg-output-buffer' is
turned off after inserting a temp file which contains GnuPG's
output, as the case may be.  It occurs while evaluating
`pgg-gpg-process-sentinel' (trunk) or `pgg-gpg-process-region'
(v5-10).  As long as I investigated, it takes place when
`coding-system-for-read' is bound to `raw-text' or `binary' and
an empty file is inserted into an empty multibyte buffer.  Here
is a test case:


[-- Attachment #2: Type: application/emacs-lisp, Size: 265 bytes --]

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


Only Emacs 23 returns t, the others return nil.

Fixed in the trunk and the v5-10 branch.

Re: pgg-output-buffer gets created as a unibyte buffer

[Daiki Ueno]

>>>>> In <b4mwtdbfqob.fsf@jpl.org> 
>>>>>	Katsumi Yamaoka <yamaoka@jpl.org> wrote:
> Fixed in the trunk and the v5-10 branch.

;; Buffer's multibyteness might be turned off after
;; inserting file's contents, as the case may be.
(let ((coding-system-for-read (if pgg-text-mode
                                  'raw-text
                                'binary)))
  (insert-file-contents output-file-name))
(when (and (fboundp 'set-buffer-multibyte)
           (subrp (symbol-function 'set-buffer-multibyte))
           (not enable-multibyte-characters))
  (if (zerop (buffer-size))
      (set-buffer-multibyte t)
    (insert (pgg-string-to-multibyte
             (prog1
                 (buffer-string)
               (erase-buffer)
               (set-buffer-multibyte t)))))

Oops, It was a mistake that I didn't advise you not to fix in such an
ugly way, though I predicted you would do so.  Is it impossible to fix
this by changing callers of PGG i.e. ml2015.el or mml1991.el?

I suppose PGG is a low level library and it shall assume that
pgg-*-buffer contains unibyte data.

Regards,
-- 
Daiki Ueno

Re: pgg-output-buffer gets created as a unibyte buffer

[Katsumi Yamaoka]

>>>>> In <9c79059a-61a9-4fa4-8376-638753320a14@well-done.deisui.org>
>>>>>	Daiki Ueno wrote:

>>>>>> In <b4mwtdbfqob.fsf@jpl.org>
>>>>>>	Katsumi Yamaoka <yamaoka@jpl.org> wrote:

>> Fixed in the trunk and the v5-10 branch.

> ;; Buffer's multibyteness might be turned off after
> ;; inserting file's contents, as the case may be.
> (let ((coding-system-for-read (if pgg-text-mode
>                                   'raw-text
>                                 'binary)))
>   (insert-file-contents output-file-name))
> (when (and (fboundp 'set-buffer-multibyte)
>            (subrp (symbol-function 'set-buffer-multibyte))
>            (not enable-multibyte-characters))
>   (if (zerop (buffer-size))
>       (set-buffer-multibyte t)
>     (insert (pgg-string-to-multibyte
>              (prog1
>                  (buffer-string)
>                (erase-buffer)
>                (set-buffer-multibyte t)))))

> Oops, It was a mistake that I didn't advise you not to fix in such an
> ugly way, though I predicted you would do so.  Is it impossible to fix
> this by changing callers of PGG i.e. ml2015.el or mml1991.el?

I didn't hit on such a way.  If anything, I guess there's no way
to do so.

> I suppose PGG is a low level library and it shall assume that
> pgg-*-buffer contains unibyte data.

The problem occurs in a low level either.  Any alternative plan?

> Regards,
> --
> Daiki Ueno

Re: pgg-output-buffer gets created as a unibyte buffer

[Katsumi Yamaoka]

>>>>> In <b4mpsj3gw1s.fsf@jpl.org> Katsumi Yamaoka wrote:

>>>>>> In <9c79059a-61a9-4fa4-8376-638753320a14@well-done.deisui.org>
>>>>>>	Daiki Ueno wrote:

>> I suppose PGG is a low level library and it shall assume that
>> pgg-*-buffer contains unibyte data.

> The problem occurs in a low level either.

Please ignore last sentence of mine.  Making pgg-*-buffer always
unibyte is a good idea.  However, all the wrappers seem to have
expected that it is multibyte, and it is actually so.  Although
we will be able to change all of them, it might cause bugs at
the beginning.  Moreover, we have to be careful especially in
the v5-10 branch since the Emacs release is approaching.  Even
so, we should run a risk?  In other words,

> Any alternative plan?

Re: pgg-output-buffer gets created as a unibyte buffer

[Daiki Ueno]

>>>>> In <b4my7xrfg5o.fsf@jpl.org> 
>>>>>	Katsumi Yamaoka <yamaoka@jpl.org> wrote:
> >>>>> In <b4mpsj3gw1s.fsf@jpl.org> Katsumi Yamaoka wrote:
> Please ignore last sentence of mine.  Making pgg-*-buffer always
> unibyte is a good idea.  However, all the wrappers seem to have
> expected that it is multibyte, and it is actually so.

There is no problem with other MUA using PGG such as Wanderlust and
T-Gnus.  I think Gnus' usage of PGG is wrong.

> Although
> we will be able to change all of them, it might cause bugs at
> the beginning.  Moreover, we have to be careful especially in
> the v5-10 branch since the Emacs release is approaching.  Even
> so, we should run a risk?  In other words,

Well, if you would like to add such a kludge, please add your name on
the Maintainer: field and rename PGG to Gnus/PGG or such.  Of course I'm
not the developer nor the maintainer of that package.

I know that most Gnus people seem to prefer convenience over clearner
design or security.  For example, the original PGG does not use
`call-process-region' for security reason -- this function writes data
to a temporary file.  About three years ago, Gnus decided to use
`call-process-region' in PGG to avoid display blinking.  Since that
time, Gnus has been one of world's most insecure MUA.  Your passphrases
always leak to the filesystem.
-- 
Daiki Ueno

Re: pgg-output-buffer gets created as a unibyte buffer

[Romain Francoise]

Daiki Ueno <ueno@unixuser.org> writes:

> For example, the original PGG does not use `call-process-region' for
> security reason -- this function writes data to a temporary file.
> About three years ago, Gnus decided to use `call-process-region' in
> PGG to avoid display blinking.

The current version of PGG in the trunk doesn't do that anymore.
That sounds like a good enough reason to sync that version in v5-10!

-- 
Romain Francoise <romain@orebokech.com> | The sea! the sea! the open
it's a miracle -- http://orebokech.com/ | sea! The blue, the fresh, the
                                        | ever free! --Bryan W. Procter

Re: pgg-output-buffer gets created as a unibyte buffer

[Katsumi Yamaoka]

>>>>> In <4aaf7080-0e3d-4a75-aff5-f9d5bcd0437f@well-done.deisui.org>
>>>>>	Daiki Ueno wrote:

> Well, if you would like to add such a kludge, please add your name on
> the Maintainer: field and rename PGG to Gnus/PGG or such.  Of course I'm
> not the developer nor the maintainer of that package.

I've withdrawn all the changes in pgg*.el.  Instead, I'll do
something in the Gnus side tomorrow.

Re: pgg-output-buffer gets created as a unibyte buffer

[Katsumi Yamaoka]

>>>>> In <b4m64kvyp9l.fsf@jpl.org> Katsumi Yamaoka wrote:

>>>>>> In <4aaf7080-0e3d-4a75-aff5-f9d5bcd0437f@well-done.deisui.org>
>>>>>>	Daiki Ueno wrote:

>> Well, if you would like to add such a kludge, please add your name on
>> the Maintainer: field and rename PGG to Gnus/PGG or such.  Of course I'm
>> not the developer nor the maintainer of that package.

> I've withdrawn all the changes in pgg*.el.  Instead, I'll do
> something in the Gnus side tomorrow.

I've fixed it, narrowing down to the problem that I encountered.
In `mml2015-pgg-clear-decrypt', now the buffer is made unibyte
before inserting the contents of `pgg-output-buffer', and in
`mm-uu-pgp-encrypted-extract-1', the buffer is made multibyte
again before replacing the contents with the one decoded by
charset.

Although there are still the codes which insert contents of
`pgg-output-buffer' into a multibyte buffer, they don't seem to
cause any problem as long as I investigated.  It is because data
being inserted don't contain 8bit characters.

By the way, I verified my changes using the messages which I
made as follows:

1. Modify the value of `mm-coding-system-priorities' so that
   utf-8 (or shift_jis) might be most preferred when encoding
   non-ASCII messages.

   (push 'utf-8 mm-coding-system-priorities)

2. Open a message buffer.  Fill the To header with my e-mail
   address.  Put a MML tag in the beginning of the body using
   M-x mml-secure-message-encrypt-pgp RET.  Write any Japanese
   text below.  And then type C-c C-c.

The tag causes Message to call the `mml1991-pgg-encrypt' function
to encrypt the message.  I heard that Thunderbird and Outlook
with the CryptoEx plugin encrypt messages in the style similar to
the one that it makes (cf. <gg.87r73oi4b9@hush.zonix.de> in the
gnus.gnus-bug newsgroup).

When decrypting such messages, Gnus uses the
`mml2015-pgg-clear-decrypt' function by way of the mm-uu feature
(namely, `gnus-article-emulate-mime' should be set to t).

Regards,

pgg-gpg-process-region (was: pgg-output-buffer gets created as a unibyte buffer)

[Reiner Steib]

On Thu, Apr 27 2006, Romain Francoise wrote:

> Daiki Ueno <ueno@unixuser.org> writes:
>
>> For example, the original PGG does not use `call-process-region' for
>> security reason -- this function writes data to a temporary file.

Did you check which versions of Emacs or XEmacs do this?  (I don't
have the C sources here ATM, so I can't check myself.)

>> About three years ago, Gnus decided to use `call-process-region' in
>> PGG to avoid display blinking.
>
> The current version of PGG in the trunk doesn't do that anymore.
> That sounds like a good enough reason to sync that version in v5-10!

Maybe we should rather revert the change introducing
`call-process-region' [1]?  Have all the problems that led us to
revert pgg-gpg.el before the 5.10.8 release been fixed in the trunk
version (or in Daiki's version)?

Bye, Reiner.

[1]
,----[ ChangeLog.2 ]
| 2003-02-08  Simon Josefsson  <jas@extundo.com>
| 
| 	* gnus-sum.el (gnus-summary-select-article): Remove blink removal
| 	code that only worked under Emacs.
| 
| 	* pgg-gpg.el (pgg-gpg-process-region): Don't blink.  From Satyaki
| 	Das <satyaki@chicory.stanford.edu>.
`----

--8<---------------cut here---------------start------------->8---
--- pgg-gpg.el	2 Nov 2002 04:27:00 -0000	6.8
+++ pgg-gpg.el	8 Feb 2003 18:58:23 -0000	6.9
@@ -59,27 +59,22 @@
 	 (errors-buffer pgg-errors-buffer)
 	 (orig-mode (default-file-modes))
 	 (process-connection-type nil)
-	 process status exit-status)
+	 exit-status)
     (with-current-buffer (get-buffer-create errors-buffer)
       (buffer-disable-undo)
       (erase-buffer))
     (unwind-protect
 	(progn
 	  (set-default-file-modes 448)
-	  (let ((coding-system-for-write 'binary))
-	    (setq process
-		  (apply #'start-process "*GnuPG*" errors-buffer
-			 program args)))
-	  (set-process-sentinel process #'ignore)
-	  (when passphrase
-	    (process-send-string process (concat passphrase "\n")))
-	  (process-send-region process start end)
-	  (process-send-eof process)
-	  (while (eq 'run (process-status process))
-	    (accept-process-output process 5))
-	  (setq status (process-status process)
-		exit-status (process-exit-status process))
-	  (delete-process process)
+          (let* ((coding-system-for-write 'binary)
+                 (input (buffer-substring-no-properties start end)))
+            (with-temp-buffer
+              (when passphrase
+                (insert passphrase "\n"))
+              (insert input)
+              (setq exit-status
+                    (apply #'call-process-region (point-min) (point-max) program
+                           nil errors-buffer nil args))))
 	  (with-current-buffer (get-buffer-create output-buffer)
 	    (buffer-disable-undo)
 	    (erase-buffer)
@@ -87,12 +82,8 @@
 		(let ((coding-system-for-read 'raw-text-dos))
 		  (insert-file-contents output-file-name)))
 	    (set-buffer errors-buffer)
-	    (if (memq status '(stop signal))
-		(error "%s exited abnormally: '%s'" program exit-status))
-	    (if (= 127 exit-status)
-		(error "%s could not be found" program))))
-      (if (and process (eq 'run (process-status process)))
-	  (interrupt-process process))
+	    (if (not (equal exit-status 0))
+		(error "%s exited abnormally: '%s'" program exit-status))))
       (if (file-exists-p output-file-name)
 	  (delete-file output-file-name))
       (set-default-file-modes orig-mode))))
--8<---------------cut here---------------end--------------->8---
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Security flaw in pgg-gpg-process-region? (was: pgg-gpg-process-region)

[Reiner Steib]

[ Adding emacs-devel; therefore not trimming quotes.  See
  <http://thread.gmane.org/gmane.emacs.devel/43396/focus=52626> for
  the rest of the discussion. ]

On Sat, May 06 2006, Reiner Steib wrote:

> On Thu, Apr 27 2006, Romain Francoise wrote:
>
>> Daiki Ueno <ueno@unixuser.org> writes:
>>
>>> For example, the original PGG does not use `call-process-region' for
>>> security reason -- this function writes data to a temporary file.
>
> Did you check which versions of Emacs or XEmacs do this?  (I don't
> have the C sources here ATM, so I can't check myself.)

In current Emacs CVS in fact `call-process-region' uses temp files.
Bad.  I think this is a severe security problem, isn't it?  I think
this should be fixed before the release.

>>> About three years ago, Gnus decided to use `call-process-region' in
>>> PGG to avoid display blinking.
>>
>> The current version of PGG in the trunk doesn't do that anymore.
>> That sounds like a good enough reason to sync that version in v5-10!
>
> Maybe we should rather revert the change introducing
> `call-process-region' [1]?  

The revered patch doesn't apply anymore.  Could someone please look
for a possibility to avoid `call-process-region' in
`pgg-gpg-process-region' or suggest an alternative solution?

> Have all the problems that led us to revert pgg-gpg.el before the
> 5.10.8 release been fixed in the trunk version (or in Daiki's
> version)?
>
> Bye, Reiner.
>
> [1]
> ,----[ ChangeLog.2 ]
> | 2003-02-08  Simon Josefsson  <jas@extundo.com>
> | 
> | 	* gnus-sum.el (gnus-summary-select-article): Remove blink removal
> | 	code that only worked under Emacs.
> | 
> | 	* pgg-gpg.el (pgg-gpg-process-region): Don't blink.  From Satyaki
> | 	Das <satyaki@chicory.stanford.edu>.
> `----
>
> --- pgg-gpg.el	2 Nov 2002 04:27:00 -0000	6.8
> +++ pgg-gpg.el	8 Feb 2003 18:58:23 -0000	6.9
> @@ -59,27 +59,22 @@
>  	 (errors-buffer pgg-errors-buffer)
>  	 (orig-mode (default-file-modes))
>  	 (process-connection-type nil)
> -	 process status exit-status)
> +	 exit-status)
>      (with-current-buffer (get-buffer-create errors-buffer)
>        (buffer-disable-undo)
>        (erase-buffer))
>      (unwind-protect
>  	(progn
>  	  (set-default-file-modes 448)
> -	  (let ((coding-system-for-write 'binary))
> -	    (setq process
> -		  (apply #'start-process "*GnuPG*" errors-buffer
> -			 program args)))
> -	  (set-process-sentinel process #'ignore)
> -	  (when passphrase
> -	    (process-send-string process (concat passphrase "\n")))
> -	  (process-send-region process start end)
> -	  (process-send-eof process)
> -	  (while (eq 'run (process-status process))
> -	    (accept-process-output process 5))
> -	  (setq status (process-status process)
> -		exit-status (process-exit-status process))
> -	  (delete-process process)
> +          (let* ((coding-system-for-write 'binary)
> +                 (input (buffer-substring-no-properties start end)))
> +            (with-temp-buffer
> +              (when passphrase
> +                (insert passphrase "\n"))
> +              (insert input)
> +              (setq exit-status
> +                    (apply #'call-process-region (point-min) (point-max) program
> +                           nil errors-buffer nil args))))
>  	  (with-current-buffer (get-buffer-create output-buffer)
>  	    (buffer-disable-undo)
>  	    (erase-buffer)
> @@ -87,12 +82,8 @@
>  		(let ((coding-system-for-read 'raw-text-dos))
>  		  (insert-file-contents output-file-name)))
>  	    (set-buffer errors-buffer)
> -	    (if (memq status '(stop signal))
> -		(error "%s exited abnormally: '%s'" program exit-status))
> -	    (if (= 127 exit-status)
> -		(error "%s could not be found" program))))
> -      (if (and process (eq 'run (process-status process)))
> -	  (interrupt-process process))
> +	    (if (not (equal exit-status 0))
> +		(error "%s exited abnormally: '%s'" program exit-status))))
>        (if (file-exists-p output-file-name)
>  	  (delete-file output-file-name))
>        (set-default-file-modes orig-mode))))

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

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <v9mz9itt6y.fsf_-_@marauder.physik.uni-ulm.de> 
>>>>>	Reiner Steib <reinersteib+gmane@imap.cc> wrote:
> [ Adding emacs-devel; therefore not trimming quotes.  See
>   <http://thread.gmane.org/gmane.emacs.devel/43396/focus=52626> for
>   the rest of the discussion. ]

Huh?  I don't understand you refered to that article, there is no
related issue with allout.el.
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <8980fd83-08b6-4aef-97f2-a659cd2eadb2@well-done.deisui.org> 
>>>>>	Daiki Ueno <ueno@unixuser.org> wrote:
> >>>>> In <v9mz9itt6y.fsf_-_@marauder.physik.uni-ulm.de> 
> >>>>>	Reiner Steib <reinersteib+gmane@imap.cc> wrote:
> > [ Adding emacs-devel; therefore not trimming quotes.  See
> >   <http://thread.gmane.org/gmane.emacs.devel/43396/focus=52626> for
> >   the rest of the discussion. ]

> Huh?  I don't understand you refered to that article, there is no
> related issue with allout.el.

Anyway, if you mention the security problem of PGG, you should also
mention the multibyteness problem

http://article.gmane.org/gmane.emacs.gnus.general/62428

Which causes PGG almost unusable by users who use mutibyte locales.

I would say that Gnus developers have horribly degraded the quality of
PGG since it was merged into Gnus...

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region? (was: pgg-gpg-process-region)

[Richard Stallman]

    In current Emacs CVS in fact `call-process-region' uses temp files.
    Bad.  I think this is a severe security problem, isn't it?

As far as I know, this not bad at all.
Would you please explain why you think it is a problem?

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

In the CVS Emacs we have moved PGG out of Gnus.  Does our version
have these problems?  If so, can you show us how to fix them?

We want to start pretesting, but we can still install bug fixes
if they don't involve widespread change.

Re: Security flaw in pgg-gpg-process-region?

[Florian Weimer]

* Reiner Steib:

> In current Emacs CVS in fact `call-process-region' uses temp files.
> Bad.  I think this is a severe security problem, isn't it?

Why?  AFAICS, Emacs uses mkstemp when available, which should get the
permissions right.

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <E1GJti6-0003U0-No@fencepost.gnu.org> 
>>>>>	Richard Stallman <rms@gnu.org> wrote:
> In the CVS Emacs we have moved PGG out of Gnus.  Does our version
> have these problems?  If so, can you show us how to fix them?

Yes it does.  To solve them we should revert a couple of changes from
Satyaki Das

http://article.gmane.org/gmane.emacs.gnus.general/49947
http://article.gmane.org/gmane.emacs.gnus.general/50457

I've done this in Gnus CVS.  I expect that the fixes will appear in
Emacs CVS shortly.

> We want to start pretesting, but we can still install bug fixes
> if they don't involve widespread change.

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <87ac5gnccs.fsf@mid.deneb.enyo.de> 
>>>>>	Florian Weimer <fw@deneb.enyo.de> wrote:
> * Reiner Steib:

> > In current Emacs CVS in fact `call-process-region' uses temp files.
> > Bad.  I think this is a severe security problem, isn't it?

> Why?  AFAICS, Emacs uses mkstemp when available, which should get the
> permissions right.

May I answer the question on behalf of Reiner Steib?

When decrypting PGP messages PGG will send your passphrase along with
data, so if Emacs process is killed and you have stolen your note PC,
your passphrase can also be stolen from the temp file.

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[Miles Bader]

Daiki Ueno <ueno@unixuser.org> writes:
>> > In current Emacs CVS in fact `call-process-region' uses temp files.
>> > Bad.  I think this is a severe security problem, isn't it?
>
>> Why?  AFAICS, Emacs uses mkstemp when available, which should get the
>> permissions right.
>
> May I answer the question on behalf of Reiner Steib?
>
> When decrypting PGP messages PGG will send your passphrase along with
> data, so if Emacs process is killed and you have stolen your note PC,
> your passphrase can also be stolen from the temp file.

It would probably be fairly simple to change the implementation to
unlink the temp file _before_ writing the contents and pass only the
still-open file-descriptor (after rewinding) to Fcall_process (or
rather, to some common subroutine derived from Fcall_process).

I suppose the annoying part would be making sure everything still worked
on systems like ms-windows; I don't know if they support the common
"open and unlink before using" idiom for temp files in unix.

-Miles
-- 
Quidquid latine dictum sit, altum viditur.

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    Yes it does.  To solve them we should revert a couple of changes from
    Satyaki Das

    http://article.gmane.org/gmane.emacs.gnus.general/49947
    http://article.gmane.org/gmane.emacs.gnus.general/50457

I'm sure your right that these changes caused a problem.
I am sure there was also a motive for the changes.
Do you know what it was?  If the changes solved some problems,
does reverting the changes bring those problems back?
If so, how should we solve them?

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <E1GKI5D-00005c-8Y@fencepost.gnu.org> 
>>>>>	Richard Stallman <rms@gnu.org> wrote:
>     Yes it does.  To solve them we should revert a couple of changes from
>     Satyaki Das

>     http://article.gmane.org/gmane.emacs.gnus.general/49947 (1)
>     http://article.gmane.org/gmane.emacs.gnus.general/50457 (2)

> I'm sure your right that these changes caused a problem.
> I am sure there was also a motive for the changes.
> Do you know what it was?

There are appearantly two motives as he mentioned in the above article.

First, in (1) he didn't like the "display blinking" behavior since PGG
had been used asynchronous process instead of synchronous process.
As he said, this was not a real problem.

Second, (1) causes a problem which forbids using ISO-8859-1 characters
in passphrases.  So he proposed (2), but it was not a correct fix
(passphrases should be encoded in locale-coding-system rather than just
making them unibyte) and it was not working before the reversion.  I
think this is not so important problem, since it can be avoided by using
ASCII only passphrases in practice.

> If the changes solved some problems,
> does reverting the changes bring those problems back?

If you think "display blinking" is really a problem, it can be resolved
by simply binding (inhibit-redisplay t) in pgg-gpg-*-region.

The latter problem is bit difficulut to solve in the right way.

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[David Kastrup]

Daiki Ueno <ueno@unixuser.org> writes:

> Second, (1) causes a problem which forbids using ISO-8859-1
> characters in passphrases.  So he proposed (2), but it was not a
> correct fix (passphrases should be encoded in locale-coding-system
> rather than just making them unibyte) and it was not working before
> the reversion.  I think this is not so important problem, since it
> can be avoided by using ASCII only passphrases in practice.

Passphrases exist outside of Emacs, and you don't have the option of
just typing something else.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <854pvnsetc.fsf@lola.goethe.zz> 
>>>>>	David Kastrup <dak@gnu.org> wrote:
> Daiki Ueno <ueno@unixuser.org> writes:

> > Second, (1) causes a problem which forbids using ISO-8859-1
> > characters in passphrases.  So he proposed (2), but it was not a
> > correct fix (passphrases should be encoded in locale-coding-system
> > rather than just making them unibyte) and it was not working before
> > the reversion.  I think this is not so important problem, since it
> > can be avoided by using ASCII only passphrases in practice.

> Passphrases exist outside of Emacs, and you don't have the option of
> just typing something else.

In theory you are right.  However, since GnuPG treats passphrase input
as a byte sequence not characters, if you set your passphrase on a
ISO-8859-1 terminal, you can't input the same passphrase on any UTF-8
terminals.

Anyway, I fixed it in Gnus CVS so that passphrases are encoded with
locale-coding-system.  I'm not sure if we should add a new user option
to control the encoding.

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    When decrypting PGP messages PGG will send your passphrase along
    with data, so if Emacs process is killed and [someone else has]
    stolen your note PC, your passphrase can also be stolen from the
    temp file.

Since it is not likely for Emacs to be killed just while it is running
GPG, I think that very few users have such temp files lying around.
So the thief would need to be very lucky (as well as knowing about
such things) in order get anyone's pass phrase.

Therefore, I think it is not desperately urgent to fix this.
We should fix it if it is feasible, but it may be hard.

    It would probably be fairly simple to change the implementation to
    unlink the temp file _before_ writing the contents and pass only the
    still-open file-descriptor (after rewinding) to Fcall_process (or
    rather, to some common subroutine derived from Fcall_process).

We would have to unlink the file before writing the contents into it.
That would be somewhat more work, since Fwrite_region needs to be able
to use an already-open descriptor, too.  Still, it is possible in
principle.  Would someone like to try it?


We could make the problem even more unlikely if we can arrange for
Emacs to delete any such temp files that are lying around when it
starts.  For this, the hard part is dealing with multiple machines
that share the same temp file directory.  In that case, Emacs can't
tell whether the Emacs that wrote a certain temp file is still alive.
However, if Emacs put the machine name, user name and process ID into
the file name, then each Emacs could tell which of these temp files
are from the same machine and same user; then it could check whether
those processes are still alive.

This way, the thief would have to get your machine after Emacs dies
while running GPG and before you start another Emacs.

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <E1GKXSp-0002f5-Gr@fencepost.gnu.org> 
>>>>>	Richard Stallman <rms@gnu.org> wrote:
>     When decrypting PGP messages PGG will send your passphrase along
>     with data, so if Emacs process is killed and [someone else has]
>     stolen your note PC, your passphrase can also be stolen from the
>     temp file.

> Since it is not likely for Emacs to be killed just while it is running
> GPG, I think that very few users have such temp files lying around.
> So the thief would need to be very lucky (as well as knowing about
> such things) in order get anyone's pass phrase.

I don't think so.  The rationale is, (1) decrypting large data takes
some time, (2) the user tends to interrupt Emacs from the terminal, and
(3) every file PGG writes out are in the same format

"p@ssphr@se
-----BEGIN PGP MESSAGE-----
...
-----END PGP MESSAGE-----"

I think every security problem looks not feasible, at a glance.

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[Chong Yidong]

Daiki Ueno <ueno@unixuser.org> writes:

> Anyway, I fixed it in Gnus CVS so that passphrases are encoded with
> locale-coding-system.  I'm not sure if we should add a new user option
> to control the encoding.

Is there anything else that needs to be done, or can we consider this
bug closed?

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    First, in (1) he didn't like the "display blinking" behavior since PGG
    had been used asynchronous process instead of synchronous process.
    As he said, this was not a real problem.

Ok.

    Second, (1) causes a problem which forbids using ISO-8859-1 characters
    in passphrases.  So he proposed (2), but it was not a correct fix
    (passphrases should be encoded in locale-coding-system rather than just
    making them unibyte) and it was not working before the reversion.  I
    think this is not so important problem, since it can be avoided by using
    ASCII only passphrases in practice.

Are you saying this problem does not occur if his change (1) is removed?
If so, we don't need to solve it, since we have removed (1).

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    Anyway, I fixed it in Gnus CVS so that passphrases are encoded with
    locale-coding-system.  I'm not sure if we should add a new user option
    to control the encoding.

locale-coding-system exists to tell Emacs how to decode some system
messages which are encoded (outside Emacs) in the current locale.  It
seems to be wrong for this use.  Do you see some reason to think it is
right?

If you see a reason, please explain it to me.  Otherwise, please make
a new option for this.

And then we should install this in Emacs.

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <E1GKt6a-0005PE-Gv@fencepost.gnu.org> 
>>>>>	Richard Stallman <rms@gnu.org> wrote:
>     Anyway, I fixed it in Gnus CVS so that passphrases are encoded with
>     locale-coding-system.  I'm not sure if we should add a new user option
>     to control the encoding.

> locale-coding-system exists to tell Emacs how to decode some system
> messages which are encoded (outside Emacs) in the current locale.  It
> seems to be wrong for this use.  Do you see some reason to think it is
> right?

> If you see a reason, please explain it to me.  Otherwise, please make
> a new option for this.

I have no reason.  I just followed the example of setenv which uses
locale-coding-system for encoding the value of envvars.

I prepared pgg-passphrase-coding-system in Gnus CVS.

FYI

$ grep locale-coding-system **/*.el | grep encode-coding-string
env.el:      (setq variable (encode-coding-string variable locale-coding-system)))
env.el:      (setq value (encode-coding-string value locale-coding-system)))
hexl.el:           (mapcar (lambda (s) (encode-coding-string s locale-coding-system))
term/x-win.el:         (encode-coding-string text (or locale-coding-system

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    I don't think so.  The rationale is, (1) decrypting large data takes
    some time, (2) the user tends to interrupt Emacs from the terminal, and

What do you mean by "interrupt Emacs from the terminal"?
I don't understand the scenario that you have in mind here.

Re: Security flaw in pgg-gpg-process-region?

[gdt]

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


I think there's a higher-level point that hasn't been made explicit,
although I'm sure it's what Daiki is thinking: Anything that can cause
the passphrase to be written to the filesystem is horribly broken; the
whole point of the passphrase is that while the secret key (encrypted
in the passphrase) is on disk, without the passphrase one can't get
the key even if one has the disk.  As soon as the passphrase ends up
on disk, through a temp file, core file, swap space, the plan is
compromised.  Programs like gnupg take care to mlock(2) or similar to
keep key data from being paged out.  (One also needs to disable kernel
crash dumps.)

The right solution might instead be to push for gpg-agent to be
production ready, so that entire notion of emacs dealing with
passphrases can be deprecated.

-- 
    Greg Troxel <gdt@work.lexort.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 185 bytes --]

Re: Security flaw in pgg-gpg-process-region?

[Florian Weimer]

* Richard Stallman:

>     It would probably be fairly simple to change the implementation to
>     unlink the temp file _before_ writing the contents and pass only the
>     still-open file-descriptor (after rewinding) to Fcall_process (or
>     rather, to some common subroutine derived from Fcall_process).
>
> We would have to unlink the file before writing the contents into it.

This doesn't achieve much, I'm afraid.  Even unnamed files can be
written to disk by the kernel.  It's not much different from
passphrases stored in process images ending up in the swap file,
though.  I'm pretty sure I looked at the situation when I wrote gpg.el
a couple of years ago, and decided that all things considered, it's
not terribly important.  It's a significant PR issue, admittedly, but
back then, I didn't care about that. 8-)

As Greg suggested, the passphrase handling should be moved from Emacs
into a separate process (which may request special privileges to lock
memory regions etc.).

Re: Security flaw in pgg-gpg-process-region?

[Miles Bader]

gdt@work.lexort.com writes:
> The right solution might instead be to push for gpg-agent to be
> production ready, so that entire notion of emacs dealing with
> passphrases can be deprecated.

The existing pinentry user-interfaces used by gpg-agent are all quite
sucky though; it would be really nice if _emacs_ could be the pinentry
frontend !

-Miles
-- 
"An atheist doesn't have to be someone who thinks he has a proof that there
can't be a god.  He only has to be someone who believes that the evidence
on the God question is at a similar level to the evidence on the werewolf
question."  [John McCarthy]

Re: Security flaw in pgg-gpg-process-region?

[Daiki Ueno]

>>>>> In <E1GL2iA-0000uI-22@fencepost.gnu.org> 
>>>>>	Richard Stallman <rms@gnu.org> wrote:
>     I don't think so.  The rationale is, (1) decrypting large data takes
>     some time, (2) the user tends to interrupt Emacs from the terminal, and

> What do you mean by "interrupt Emacs from the terminal"?
> I don't understand the scenario that you have in mind here.

^C in the terminal where the user launched Emacs (without -nw.)  In this
case Emacs can't be said to be "killed" but it is enough to leave the
tempfile on the filesystem after the Emacs process terminated.

Regards,
-- 
Daiki Ueno

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    I prepared pgg-passphrase-coding-system in Gnus CVS.

Thanks.

Re: Security flaw in pgg-gpg-process-region?

[Chong Yidong]

Florian Weimer <fw@deneb.enyo.de> writes:

> * Richard Stallman:
>
>>     It would probably be fairly simple to change the implementation to
>>     unlink the temp file _before_ writing the contents and pass only the
>>     still-open file-descriptor (after rewinding) to Fcall_process (or
>>     rather, to some common subroutine derived from Fcall_process).
>>
>> We would have to unlink the file before writing the contents into it.
>
> This doesn't achieve much, I'm afraid.  Even unnamed files can be
> written to disk by the kernel.  It's not much different from
> passphrases stored in process images ending up in the swap file,
> though.  I'm pretty sure I looked at the situation when I wrote gpg.el
> a couple of years ago, and decided that all things considered, it's
> not terribly important.

In any case, I've looked into changing Fcall_process_region to do the
unlink-before-write trick, and changing Fcall_process to accept a file
descriptor.  It's a rather big and messy job.  Since it wouldn't
completely solve the problem anyway, could we postphone this for after
the release?

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

      As soon as the passphrase ends up
    on disk, through a temp file, core file, swap space, the plan is
    compromised.  Programs like gnupg take care to mlock(2) or similar to
    keep key data from being paged out.  (One also needs to disable kernel
    crash dumps.)

I think that the only feasible way Emacs could do that is with a
special C-level feature.

    The right solution might instead be to push for gpg-agent to be
    production ready, so that entire notion of emacs dealing with
    passphrases can be deprecated.

What's the state of work on this?

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    As Greg suggested, the passphrase handling should be moved from Emacs
    into a separate process (which may request special privileges to lock
    memory regions etc.).

I agree it is a good solution.

We would still face the issue of how users who never exit Emacs
can provide the passphrase to gpg-agent thru the shell buffer
without its being saved on disk.

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    ^C in the terminal where the user launched Emacs (without -nw.)  In this
    case Emacs can't be said to be "killed" but it is enough to leave the
    tempfile on the filesystem after the Emacs process terminated.

Do you actually find that users do this while running mailcrypt?
It seems like a strange thing to do; wouldn't they try C-g first,
most of the time?

By unlinking the temp file before writing it, we could avoid the
problem that the file might remain in /tmp.  As others have pointed
out, this won't avoid the problem that the passphrase could have been
written to some disk block while it was in the unlinked file, and it
could remain there, readable by reading the raw disk.  It could also
be saved on disk due swapping of Emacs.

So the real question is, how far should we go?  To what level of
smallness do we need to reduce this problem?  And how far do we need
to go now, before the Emacs 22 release?

I have cc'd Werner Koch, in the hope that he can give us some advice.

Re: Security flaw in pgg-gpg-process-region?

[Sascha Wilde]

Richard Stallman <rms@gnu.org> wrote:

>     The right solution might instead be to push for gpg-agent to be
>     production ready, so that entire notion of emacs dealing with
>     passphrases can be deprecated.
>
> What's the state of work on this?

Apart from the general problems with gpg-agent/pinentry (it seems
gpg-agent is optimized for use with card readers) the use of gpg-agent
is integrated and documented in the current PGG from CVS Emacs as well
as in the current released version of gnus.

Non the less Miles is right, that there are known issues when using
pinentry, and gpg-agent is not yet part of the stable gnupg releases.

So I would say that deprecating input of key passphrases into Emacs is
not an option yet.

Finlay I do agree that the current handling of passphrases in Emacs is
a serious security problem, which should be solved.

cheers
sascha
-- 
Sascha Wilde 
- no sig today... sorry!

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    Finlay I do agree that the current handling of passphrases in Emacs is
    a serious security problem, which should be solved.

The solution of waiting a while and urging people to start using
gpg-agent is by far the easiest.  If you think we need another interim
solution, would you please implement it?

Re: Security flaw in pgg-gpg-process-region?

[Sascha Wilde]

Richard Stallman <rms@gnu.org> wrote:

First of all: sorry for this _really_ late reply...

>     Finlay I do agree that the current handling of passphrases in Emacs is
>     a serious security problem, which should be solved.
>
> The solution of waiting a while and urging people to start using
> gpg-agent is by far the easiest.

Ack.  This is a working solution, and as it seems the only realistic
for the upcoming release.

> If you think we need another interim solution, would you please
> implement it?

I thought of it, but as far as I can see the necessary changes would
involve some substantial changes/extensions of parts of emacs I'm not
very familiar with -- so I guess it wouldn't be a good thing to do at
this point of time.

Maybe we should point out the problem somewhere in the docs?

cheers
sascha
-- 
Sascha Wilde
Real programmers don't want "what you see is what you get", they want
"you asked for it, you got it".  They want editors that are terse,
powerful, cryptic, and unforgiving.  In a word, Teco.

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

What is the current state of gpg-agent?

Re: Security flaw in pgg-gpg-process-region?

[Sascha Wilde]

Richard Stallman <rms@gnu.org> wrote:

> What is the current state of gpg-agent?

gpg-agent is not yet part of the stable GnuPG distribution, but
despite that it's considered stable and ready for production use.

There are some general usability problems with gpg-agent in
conjunction with pinentry (a utility program used for passphrase input
when gpg-agent is used without the smartcard support) -- but these
issues only show up in certain complex situations and I wouldn't
consider them a show stopper.

My gpg-agent related code in pgg is part of CVS Emacs as well as the
current stable gnus release, and I'm not aware of any problems with
it.  (I remember Daiki was working on a technically more elegant
version, but don't know its current status.)

In conclusion I would say that using pgg with gpg-agent can be
recommended as best practice, but there are some obstacles which might
make it hard for the average user to follow this advise:
- GNU/Linux distributions might not have packages of gpg-agent
- I don't know about support for gpg-agent on non unixoid platforms
- users might encounter situations in which gpg-agent/pinentry won't
  work as expected

cheers
sascha
-- 
Sascha Wilde
Life's too short to read boring signatures

Re: Security flaw in pgg-gpg-process-region?

[Richard Stallman]

    My gpg-agent related code in pgg is part of CVS Emacs as well as the
    current stable gnus release, and I'm not aware of any problems with
    it.  (I remember Daiki was working on a technically more elegant
    version, but don't know its current status.)

So what we need is text to tell people to use gpg-agent, and telling
them how to do so.

Who wants to write it?

Re: Security flaw in pgg-gpg-process-region?

[Sascha Wilde]

Sascha Wilde <wilde@sha-bang.de> wrote:
> Richard Stallman <rms@gnu.org> wrote:
>
>> What is the current state of gpg-agent?
>
> gpg-agent is not yet part of the stable GnuPG distribution, but
> despite that it's considered stable and ready for production use.

As GnuPG 2.0 is now released:
http://lists.gnupg.org/pipermail/gnupg-announce/2006q4/000239.html
this no longer holds true, gpg-agent is now part of the current GnuPG
stable release.  :-)

cheers
sascha
-- 
Sascha Wilde
Well, *my* brain likes to think it's vastly more powerful than any
finite Turing machine but it hasn't proven that to me...
  -- Christopher Koppler in comp.lang.lisp