[BUG] mml2015-epg-find-usable-key finds unusable key

[BUG] mml2015-epg-find-usable-key finds unusable key

[Łukasz Stelmach]

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

Hi,

It looks like there is a sort of bug in mml2015-epg-encrypt at line
1117.

http://git.gnus.org/cgit/gnus.git/tree/lisp/mml2015.el?id=6e422932976e94d6dbb7ad602c5be3eb481fe46c#n1117)

The whole form that begins at line 1113 converts list of recipients
provided as email addresses, key IDs or anything gpg can digest into epg
"objects". Unfortunately there is one corner case that makes this code
fail. That is when a key is valid but a uid on it has been revoked.

Let's follow the code. epg-list-keys returns all keys for an ID. That
seems correct, however, since it knows the uid we are looking for it
might check if the uid hasn't been revoked. mml2015-epg-find-usable-key,
on the other hand, knows nothing about the recipient, it just checks
validity of the *keys* not uids.

epg.el is a part of emacs, mml2015.el belongs to gnus. IMHO it is
mml2015-epg-find-usable-key that should be fixed. The function should
accept and additional (optional?) parameter with a recipient uid as
passed to epg-list-keys and reject keys with all uids matching the
recipient parameter revoked.

I'd like to know opinion of someone who knows better than I the ideas
behind this code.

-- 
Miłego dnia,
Łukasz Stelmach

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

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Łukasz Stelmach]

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

Łukasz Stelmach <stlman@poczta.fm> writes:

> It looks like there is a sort of bug in mml2015-epg-encrypt at line
> 1117.
[...]
> I'd like to know opinion of someone who knows better than I the ideas
> behind this code.

I did some more research and here is what I found.

1. It might be actually gpg's business not to return keys with revoked
uids. I've sent a question about this to the gpg mailing list.

2. Finding a revoked uid in epg or mml2015 means reimplementing of a bit
complex chunk of code responsibel for matching uids (uids are not
structured) from gpg in elisp. Not necessarily a lot of fun to do.

If you know something I still haven't mentioned please do share. I'll
keep you informed about my findings.

-- 
Miłego dnia,
Łukasz Stelmach

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

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Daiki Ueno]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=iso-2022-jp-2, Size: 4850 bytes --]

^[$(D)(^[(Bukasz Stelmach <stlman@poczta.fm> writes:

> It looks like there is a sort of bug in mml2015-epg-encrypt at line
> 1117.
>
> http://git.gnus.org/cgit/gnus.git/tree/lisp/mml2015.el?id=6e422932976e94d6dbb7ad602c5be3eb481fe46c#n1117)
>
> The whole form that begins at line 1113 converts list of recipients
> provided as email addresses, key IDs or anything gpg can digest into epg
> "objects". Unfortunately there is one corner case that makes this code
> fail. That is when a key is valid but a uid on it has been revoked.

Thanks for the report.

> epg.el is a part of emacs, mml2015.el belongs to gnus. IMHO it is
> mml2015-epg-find-usable-key that should be fixed. The function should
> accept and additional (optional?) parameter with a recipient uid as
> passed to epg-list-keys and reject keys with all uids matching the
> recipient parameter revoked.

Sounds reasonable.  I plan to apply the attached patch.

Regards,
-- 
Daiki Ueno
From 9440de44c8dc03a5b77e662539d3d388f6718549 Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
Date: Fri, 15 Feb 2013 12:58:09 +0900
Subject: [PATCH] mml2015.el (mml2015-epg-find-usable-key): handle revoked
 user-id

---
 lisp/ChangeLog  |  9 +++++++++
 lisp/mml2015.el | 57 +++++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 5edf6f6..3fda767 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,12 @@
+2013-02-15  Daiki Ueno  <ueno@gnu.org>
+
+	* mml2015.el (mml2015-epg-check-user-id): New function.
+	(mml2015-epg-check-sub-key): New function split from
+	mml2015-epg-find-usable-key.
+	(mml2015-epg-find-usable-key): Accept context, recipient, and usage, to
+	handle the case when user-id is unusable (all callers changed).
+	Reported by ^[$(D)(^[(Bukasz Stelmach <stlman@poczta.fm>.
+
 2013-02-14  Katsumi Yamaoka  <yamaoka@jpl.org>
 
 	* gnus-util.el (gnus-define-keys): Convert [?\S-\ ] to [(shift space)]
diff --git a/lisp/mml2015.el b/lisp/mml2015.el
index 275a486..6310ff3 100644
--- a/lisp/mml2015.el
+++ b/lisp/mml2015.el
@@ -786,21 +786,47 @@ If set, it overrides the setting of `mml2015-sign-with-sender'."
 	      (cons password-cache-key-id mml2015-epg-secret-key-id-list))
 	(copy-sequence passphrase)))))
 
-(defun mml2015-epg-find-usable-key (keys usage)
-  (catch 'found
+(defun mml2015-epg-check-user-id (key recipient)
+  (let ((pointer (epg-key-user-id-list key))
+	result)
+    (while pointer
+      (if (and (equal (car (mail-header-parse-address
+			    (epg-user-id-string (car pointer))))
+		      (car (mail-header-parse-address
+			    recipient)))
+	       (not (memq (epg-user-id-validity (car pointer))
+			  '(revoked expired))))
+	  (setq result t
+		pointer nil)
+	(setq pointer (cdr pointer))))
+    result))
+
+(defun mml2015-epg-check-sub-key (key usage)
+  (let ((pointer (epg-key-sub-key-list key))
+	result)
+    ;; The primary key will be marked as disabled, when the entire
+    ;; key is disabled (see 12 Field, Format of colon listings, in
+    ;; gnupg/doc/DETAILS)
+    (unless (memq 'disabled (epg-sub-key-capability (car pointer)))
+      (while pointer
+	(if (and (memq usage (epg-sub-key-capability (car pointer)))
+		 (not (memq (epg-sub-key-validity (car pointer))
+			    '(revoked expired))))
+ 	    (setq result t
+		  pointer nil)
+	  (setq pointer (cdr pointer)))))
+    result))
+
+(defun mml2015-epg-find-usable-key (context recipient usage)
+  (let ((keys (epg-list-keys context recipient))
+	key)
     (while keys
-      (let ((pointer (epg-key-sub-key-list (car keys))))
-	;; The primary key will be marked as disabled, when the entire
-	;; key is disabled (see 12 Field, Format of colon listings, in
-	;; gnupg/doc/DETAILS)
-	(unless (memq 'disabled (epg-sub-key-capability (car pointer)))
-	  (while pointer
-	    (if (and (memq usage (epg-sub-key-capability (car pointer)))
-		     (not (memq (epg-sub-key-validity (car pointer))
-				'(revoked expired))))
-		(throw 'found (car keys)))
-	    (setq pointer (cdr pointer)))))
-      (setq keys (cdr keys)))))
+      (if (and (mml2015-epg-check-user-id (car keys) recipient)
+	       (mml2015-epg-check-sub-key (car keys) usage))
+	  (setq key (car keys)
+		keys nil)
+	(setq keys (cdr keys))))
+    key))
 
 ;; XXX: since gpg --list-secret-keys does not return validity of each
 ;; key, `mml2015-epg-find-usable-key' defined above is not enough for
@@ -1115,8 +1141,7 @@ If no one is selected, symmetric encryption will be performed.  "
 		    (mapcar
 		     (lambda (recipient)
 		       (setq recipient-key (mml2015-epg-find-usable-key
-					    (epg-list-keys context recipient)
-					    'encrypt))
+					    context recipient 'encrypt))
 		       (unless (or recipient-key
 				   (y-or-n-p
 				    (format "No public key for %s; skip it? "
-- 
1.8.1.2

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Łukasz Stelmach]

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

Daiki Ueno <ueno@gnu.org> writes:

> Łukasz Stelmach <stlman@poczta.fm> writes:
>
>> It looks like there is a sort of bug in mml2015-epg-encrypt at line
>> 1117.
>>
>> http://git.gnus.org/cgit/gnus.git/tree/lisp/mml2015.el?id=6e422932976e94d6dbb7ad602c5be3eb481fe46c#n1117)
>>
>> The whole form that begins at line 1113 converts list of recipients
>> provided as email addresses, key IDs or anything gpg can digest into epg
>> "objects". Unfortunately there is one corner case that makes this code
>> fail. That is when a key is valid but a uid on it has been revoked.
>
> Thanks for the report.
>
>> epg.el is a part of emacs, mml2015.el belongs to gnus. IMHO it is
>> mml2015-epg-find-usable-key that should be fixed. The function should
>> accept and additional (optional?) parameter with a recipient uid as
>> passed to epg-list-keys and reject keys with all uids matching the
>> recipient parameter revoked.
>
> Sounds reasonable.  I plan to apply the attached patch.

The patch looks reasonable (-; I'll check tooday in the evening
(CET). Before applying please follow the discussion (if it appears) on
the gpg-devel mailing list:

http://permalink.gmane.org/gmane.comp.encryption.gpg.devel/17572 

Of course if a fix appears in gpg 2.0.20 your code is still relevant for
thos who use older versions.

-- 
Miłego dnia,
Łukasz Stelmach

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

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Daiki Ueno]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=iso-2022-jp-2, Size: 578 bytes --]

^[$(D)(^[(Bukasz Stelmach <lukasz.stelmach@iem.pw.edu.pl> writes:

> The patch looks reasonable (-; I'll check tooday in the evening
> (CET). Before applying please follow the discussion (if it appears) on
> the gpg-devel mailing list:
>
> http://permalink.gmane.org/gmane.comp.encryption.gpg.devel/17572 

I remember that the GPG author originally suggested the current code ;-)
http://thread.gmane.org/gmane.emacs.gnus.general/64349

But let's see how it goes.  Perhaps some built-in key filtering feature
in GPG (or GPGME) might be generally useful.

Regards,
-- 
Daiki Ueno

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Łukasz Stelmach]

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

Daiki Ueno <ueno@gnu.org> writes:

> Łukasz Stelmach <stlman@poczta.fm> writes:
>
>> It looks like there is a sort of bug in mml2015-epg-encrypt at line
>> 1117.
>>
>> http://git.gnus.org/cgit/gnus.git/tree/lisp/mml2015.el?id=6e422932976e94d6dbb7ad602c5be3eb481fe46c#n1117)
>>
>> The whole form that begins at line 1113 converts list of recipients
>> provided as email addresses, key IDs or anything gpg can digest into epg
>> "objects". Unfortunately there is one corner case that makes this code
>> fail. That is when a key is valid but a uid on it has been revoked.
>
> Thanks for the report.
>
>> epg.el is a part of emacs, mml2015.el belongs to gnus. IMHO it is
>> mml2015-epg-find-usable-key that should be fixed. The function should
>> accept and additional (optional?) parameter with a recipient uid as
>> passed to epg-list-keys and reject keys with all uids matching the
>> recipient parameter revoked.
>
> Sounds reasonable.  I plan to apply the attached patch.

The patch needed a fix: there was a space before tab and my name for the
changelog has been messed up (maybe due to encoding issues?).

See the first attachment.

There are two more issues:

1. mml2015-find-usable-key is called from
mml2015-epg-find-usable-secret-key. Your patch hasn't changed the
invocation.

See the second attachment.

2. IMHO mml2015-epg-check-user-id should return true if a key id is
passed as a uid to check. I assume that if a caller passes id to
mml2015-find-usable-key find

See the third attachment.

All atachments has been generated with git-format-patch and are ready to
apply with git-am onto 98f5834.

I've got some more patches for mml2015-epg-* but let's concentrate on the
revoked uid handling.

PS. It looks like this massage originally hasn't made it to the group
(because of attachments?)
-- 
Miłego dnia,
Łukasz Stelmach

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

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Łukasz Stelmach]

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

Daiki Ueno <ueno@gnu.org> writes:

> Łukasz Stelmach <stlman@poczta.fm> writes:
>
>> The patch needed a fix: there was a space before tab and my name for the
>> changelog has been messed up (maybe due to encoding issues?).
>
> Perhaps it is because I didn't use attachment, but just inserted the
> patch into the mail.  It will be fixed in the git commit.
>
>> There are two more issues:
>>
>> 1. mml2015-find-usable-key is called from
>> mml2015-epg-find-usable-secret-key. Your patch hasn't changed the
>> invocation.
>
> Oops, right.  Thanks for pointing this out.
>
>> 2. IMHO mml2015-epg-check-user-id should return true if a key id is
>> passed as a uid to check. I assume that if a caller passes id to
>> mml2015-find-usable-key find
>
> Yes,
>
>> +	    (string-match
>> +	     "^\\(0x\\)?[[:xdigit:]]\\{8\\}\\([[:xdigit:]]\\{8\\}\\)?$"
>> +	     recipient))
>
> I think this can be simplified to:
>
> (not (string-match "\\`<" recipient))
>
> since all the mml2015-epg-* functions normalize recipient addresses in
> the form of "<foo@bar>", so they only match email addresses.

Indeed, however I still think we should check this the hard way: return
t if and *only* if it is a key-id. The simpler check *might* result in
information disclosure if a message is encrypted to a revoked uid.

PS. This is a followup to some direct e-mails.
-- 
Miłego dnia,
Łukasz

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

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Daiki Ueno]

Łukasz Stelmach <stlman@poczta.fm> writes:

>>> +	    (string-match
>>> +	     "^\\(0x\\)?[[:xdigit:]]\\{8\\}\\([[:xdigit:]]\\{8\\}\\)?$"
>>> +	     recipient))
>>
>> I think this can be simplified to:
>>
>> (not (string-match "\\`<" recipient))
>>
>> since all the mml2015-epg-* functions normalize recipient addresses in
>> the form of "<foo@bar>", so they only match email addresses.
>
> Indeed, however I still think we should check this the hard way: return
> t if and *only* if it is a key-id. The simpler check *might* result in
> information disclosure if a message is encrypted to a revoked uid.

Sorry, I don't quite understand here.  What's the senario you are
thinking of?  I think the only case RECIPIENT doesn't start with "<" is,
a user sets mml2015-signers manually.  How does it cause information
disclosure?

Regards,
-- 
Daiki Ueno

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Łukasz Stelmach]

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

Daiki Ueno <ueno@gnu.org> writes:

> Łukasz Stelmach <stlman@poczta.fm> writes:
>
>>>> +	    (string-match
>>>> +	     "^\\(0x\\)?[[:xdigit:]]\\{8\\}\\([[:xdigit:]]\\{8\\}\\)?$"
>>>> +	     recipient))
>>>
>>> I think this can be simplified to:
>>>
>>> (not (string-match "\\`<" recipient))
>>>
>>> since all the mml2015-epg-* functions normalize recipient addresses in
>>> the form of "<foo@bar>", so they only match email addresses.
>>
>> Indeed, however I still think we should check this the hard way: return
>> t if and *only* if it is a key-id. The simpler check *might* result in
>> information disclosure if a message is encrypted to a revoked uid.
>
> Sorry, I don't quite understand here.  What's the senario you are
> thinking of?  I think the only case RECIPIENT doesn't start with "<" is,
> a user sets mml2015-signers manually.  How does it cause information
> disclosure?

It should not be a problem in case of mml2015-signers (I have analysed
this case thoroughly). However, I don't like mml2015-signers work now,
an have some ideas (and even some code) to improve it. What I mean, here
is a rather hypothetical case when someone writes some code forgetting
about your assumption of normalisation. We can say: "let the guy shoot
his own foot", but I think we shouldn't. We should rather follow the
logic: assume key-id is a correct uid. In such case we should check the
input for *being* the key-id rather than not being an e-mail address.

I am not a security expert but this seems more security-wise correct and
future proof to me.

-- 
Miłego dnia,
Łukasz Stelmach

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

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Daiki Ueno]

Łukasz Stelmach <stlman@poczta.fm> writes:

> What I mean, here is a rather hypothetical case when someone writes
> some code forgetting about your assumption of normalisation. We can
> say: "let the guy shoot his own foot", but I think we shouldn't. We
> should rather follow the logic: assume key-id is a correct uid. In
> such case we should check the input for *being* the key-id rather than
> not being an e-mail address.

In that case, I'd rather modify the caller of mml2015-epg-check-user-id,
not to call the function if the recipient is key-id.  That makes it
clearer that mml2015-epg-check-user-id only deals with user-id not
key-id.

Regards,
-- 
Daiki Ueno

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Daiki Ueno]

Daiki Ueno <ueno@gnu.org> writes:

> In that case, I'd rather modify the caller of mml2015-epg-check-user-id,
> not to call the function if the recipient is key-id.  That makes it
> clearer that mml2015-epg-check-user-id only deals with user-id not
> key-id.

Pushed a fix in that way.

Regards,
-- 
Daiki Ueno

Re: [BUG] mml2015-epg-find-usable-key finds unusable key

[Łukasz Stelmach]

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

Daiki Ueno <ueno@gnu.org> writes:

> Daiki Ueno <ueno@gnu.org> writes:
>
>> In that case, I'd rather modify the caller of mml2015-epg-check-user-id,
>> not to call the function if the recipient is key-id.  That makes it
>> clearer that mml2015-epg-check-user-id only deals with user-id not
>> key-id.
>
> Pushed a fix in that way.

That's great. I traveled a bit today, and will do some more soon too,
but hopefully I will find some time to polish and present the patches
for the mml2015-epg-encrypt function I have in my .gnus.el

-- 
Kind regards,
Łukasz Stelmach

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