6.0.0-beta1
7/6/25

[#7926] Message option "Show All Headers" causes error
Summary Message option "Show All Headers" causes error
Queue IMP
Queue Version 4.3.3
Type Bug
State Resolved
Priority 2. Medium
Owners Horde Developers (at) , slusarz (at) horde (dot) org
Requester mtecles (at) biof (dot) ufrj (dot) br
Created 02/01/2009 (5999 days ago)
Due
Updated 01/12/2010 (5654 days ago)
Assigned 02/17/2009 (5983 days ago)
Resolved 02/23/2009 (5977 days ago)
Github Issue Link
Github Pull Request
Milestone
Patch No

History
04/14/2009 02:52:01 PM kristian (dot) lance (at) crc (dot) ca Comment #30 Reply to this comment
Oh yeah, and the URL that's shown in these posts isn't at all what I 
pasted, so something's manipulating it and subsequently going nutz.

[Show Quoted Text - 21 lines]
04/14/2009 02:48:02 PM kristian (dot) lance (at) crc (dot) gc (dot) ca Comment #29 Reply to this comment
I clicked on the patch to download it, and all I got was a blank 
screen. Here's the URL:



http://bugs.horde.org/h/services/download/?module=whups&actionID=download_file&file=imp_bug7926.patch&ticket=7926&fn=%2Fimp_bug7926.patch



BTW, the first time I submitted this reply, I got the following error:



Fatal error: Uncaught exception 'Horde_Mime_Exception' with message 
'Validation failed for: amg1127@cefetrs.tche.br <@>' in 
/var/www/dev.horde.org/horde-git/framework/Mime/lib/Horde/Mime/Address.php:390 
Stack trace: #0 
/var/www/dev.horde.org/horde/whups/lib/Driver.php(443): 
Horde_Mime_Address::parseAddressList('amg1127@cefetrs...') #1 
/var/www/dev.horde.org/horde/whups/lib/Ticket.php(791): 
Whups_Driver->mail('7926', Array, 'Re: Message opt...', 'DO NOT REPLY 
TO...', '-53650_transact...', false, '32', false) #2 
/var/www/dev.horde.org/horde/whups/lib/Ticket.php(416): 
Whups_Ticket->notify('-53650_transact...', false) #3 
/var/www/dev.horde.org/horde/whups/ticket/comment.php(66): 
Whups_Ticket->commit() #4 {main} thrown in 
/var/www/dev.horde.org/horde-git/framework/Mime/lib/Horde/Mime/Address.php on 
line 390
We can confirm that the PGP problems are now fixed as well.

thanks!
Is a patch (diff) file available?
CVS does not easily allow this to be done.  The individual patches
can all be found in this ticket, however.
04/14/2009 02:45:50 PM kristian (dot) lance (at) crc (dot) ca Comment #28 Reply to this comment
I clicked the download icon next to the attached patch, and got a 
blank screen. Here's the URL:



http://bugs.horde.org/h/services/download/?module=whups&actionID=download_file&file=imp_bug7926.patch&ticket=7926&fn=%2Fimp_bug7926.patch
We can confirm that the PGP problems are now fixed as well.

thanks!
Is a patch (diff) file available?
CVS does not easily allow this to be done.  The individual patches
can all be found in this ticket, however.
04/13/2009 06:12:16 PM amg1127 (at) cefetrs (dot) tche (dot) br Comment #27 Reply to this comment
Why I can't watch this ticket?
It seems that global patch is empty...
04/13/2009 06:10:22 PM amg1127 (at) cefetrs (dot) tche (dot) br Comment #26 Reply to this comment
Based on all message from there, this is a global patch for bug 7926...

Voilà...

Ivan
It seems that global patch is empty...
04/10/2009 08:54:58 PM ivan (at) spadz (dot) com Comment #25
New Attachment: imp_bug7926.patch Download		 Reply to this comment
Based on all message from there, this is a global patch for bug 7926...



Voilà...



Ivan
03/18/2009 08:20:36 PM Michael Slusarz Comment #24 Reply to this comment
We can confirm that the PGP problems are now fixed as well.

thanks!
Is a patch (diff) file available?
CVS does not easily allow this to be done.  The individual patches can 
all be found in this ticket, however.
03/18/2009 08:01:21 PM samuel (dot) wolf (at) wolf-maschinenbau (dot) de Comment #23 Reply to this comment
We can confirm that the PGP problems are now fixed as well.

thanks!
Is a patch (diff) file available?
02/23/2009 12:12:08 PM Jan Schneider State ⇒ Resolved
 
02/22/2009 02:56:22 PM horde (at) immerda (dot) ch Comment #22 Reply to this comment
We can confirm that the PGP problems are now fixed as well.



thanks!
02/20/2009 12:53:47 PM Chuck Hagenbuch Comment #21 Reply to this comment
I am not following you here...  Are you talking about the current
code or potential future code?  Because for current code, the only
reason the html-entity detection (&amp) detection is in there is to
allow successive calls to Util::addParameter() rather than any kind
of security checking.
I'm talking about the current code, because it *is* a security check - 
if you expect URLs to be encoded, but based on user input, they might 
not be, that's a problem.
02/20/2009 05:25:40 AM Michael Slusarz Comment #20 Reply to this comment
Theoretically I agree. Realistically, though, we shouldn't be letting
through potentially unencoded data, but we also have this encoding
detection that tries to see if a URL is already html-entity encoded.
Do you see any way to reconcile that?
I am not following you here...  Are you talking about the current code 
or potential future code?  Because for current code, the only reason 
the html-entity detection (&amp) detection is in there is to allow 
successive calls to Util::addParameter() rather than any kind of 
security checking.
02/17/2009 07:43:06 PM Chuck Hagenbuch Comment #19
Priority ⇒ 2. Medium
State ⇒ Feedback
Taken from Chuck Hagenbuch		 Reply to this comment
Theoretically I agree. Realistically, though, we shouldn't be letting 
through potentially unencoded data, but we also have this encoding 
detection that tries to see if a URL is already html-entity encoded. 
Do you see any way to reconcile that?
02/17/2009 07:41:20 AM Michael Slusarz Comment #18 Reply to this comment
You are right about the '&amp' trick - it is possible to put a
superfluous one in an URL and it will skip the htmlentities() call.
For what we do here, do you agree that escapeOnce (from
http://cvs.horde.org/co.php/framework/View/lib/Horde/View/Helper/Url.php?r=ccfd50278baa306abee1acd1b310a168f8ae4925) would 
work
instead?
Not really following that code without any real-life context.  But 
theoretically, we really shouldn't be "fixing" double escaped 
parameters when parsing a URL from form data.  If double escaped 
parameters exist at that point, either the generating code is broken 
and should be fixed or something fishy is occurring.  It might be best 
to ignore those URLs completely and throw an exception or error rather 
than trying to magically fix it.
02/17/2009 07:32:13 AM CVS Commit Comment #17 Reply to this comment
Changes have been made in CVS for this ticket:

http://cvs.horde.org/diff.php/imp/message.php?rt=horde&r1=2.560.4.60&r2=2.560.4.61&ty=u
02/14/2009 08:12:31 PM Chuck Hagenbuch Comment #16 Reply to this comment
You are right about the '&amp' trick - it is possible to put a
superfluous one in an URL and it will skip the htmlentities() call.
For what we do here, do you agree that escapeOnce (from 
http://cvs.horde.org/co.php/framework/View/lib/Horde/View/Helper/Url.php?r=ccfd50278baa306abee1acd1b310a168f8ae4925) would work 
instead?
02/11/2009 09:40:46 PM Michael Slusarz State ⇒ Resolved
 
02/10/2009 06:56:46 PM Michael Slusarz Comment #15 Reply to this comment
I think I have fixed all issues now.
02/10/2009 06:49:40 PM CVS Commit Comment #14 Reply to this comment
Changes have been made in CVS for this ticket:

http://cvs.horde.org/diff.php/imp/docs/CHANGES?rt=horde&r1=1.699.2.380&r2=1.699.2.381&ty=u
02/07/2009 12:31:34 PM horde (at) immerda (dot) ch Comment #12 Reply to this comment
the show all headers is fixed. thanks for fixing!



unfortunately the pgp issue isn't fixed. I still get the same 
behaviour. you'd like to have additional infos?
02/06/2009 07:46:15 AM Michael Slusarz Comment #11
State ⇒ Feedback		 Reply to this comment
You are right about the '&amp' trick - it is possible to put a 
superfluous one in an URL and it will skip the htmlentities() call.



So, instead use the same html_entity_decode/htmlspecialchars trick 
that we do in pgp.php and smime.php.  Quick tests done tonigh seems to 
indicate that it is working.
02/06/2009 07:43:48 AM CVS Commit Comment #10 Reply to this comment
Changes have been made in CVS for this ticket:

http://cvs.horde.org/diff.php/imp/message.php?rt=horde&r1=2.560.4.59&r2=2.560.4.60&ty=u
02/05/2009 09:14:01 PM Michael Slusarz Comment #9
Priority ⇒ 3. High		 Reply to this comment
The {pgp/smime}.php issues should be fixed now.
02/04/2009 09:37:06 PM Chuck Hagenbuch Comment #7 Reply to this comment
We've already mentioned the pgp/smime changes, but just in case, see 
bug #6834 for mention of a similar issue in pgp.php and smime.php from 
the same set of escaping changes.
02/04/2009 09:28:28 PM Chuck Hagenbuch Comment #6 Reply to this comment
AFAICT, selfUrl() (as called by message.php) has the $full param set
to false; in selfUrl, Horde::url() is called with $full = false; and
the URL will necessarily have '&' param separators, instead of '&amp'
separators so htmlentities() will be called on the generated URL at
the bottom of url().  Thus, anything appearing in the URL will/should
be escaped.
Okay, this is unintuitive, but I follow you and agree - though looking 
at Horde::url(), at the very bottom of the function, couldn't you 
trick it into not encoding the URL by passing &amp; for one parameter, 
and raw data in another?
As for smime.php, we should probably use
htmlspecialchars(html_entity_decode(Util::getFormData('reload')))
instead of htmlspecialchars(Util::getFormData('reload')) (we use the
former elsewhere in that file).  Looks like we aren't doing the
html_entity_decode() call in pgp.php in either place we are
processing 'reload' form data, so we should probably be doing that.
If we already do that in those files, that seems fine.
02/03/2009 03:47:28 AM Michael Slusarz Comment #5 Reply to this comment
Right, it's not relevant for template evaluation, but
Horde::selfUrl() can contain raw query parameters, so those need to
be escaped somewhere along the way. I didn't see that happening
already, but if it is, then yes, the commits are irrelevant (and
incorrect).
AFAICT, selfUrl() (as called by message.php) has the $full param set 
to false; in selfUrl, Horde::url() is called with $full = false; and 
the URL will necessarily have '&' param separators, instead of '&amp' 
separators so htmlentities() will be called on the generated URL at 
the bottom of url().  Thus, anything appearing in the URL will/should 
be escaped.



As for smime.php, we should probably use 
htmlspecialchars(html_entity_decode(Util::getFormData('reload'))) 
instead of htmlspecialchars(Util::getFormData('reload')) (we use the 
former elsewhere in that file).  Looks like we aren't doing the 
html_entity_decode() call in pgp.php in either place we are processing 
'reload' form data, so we should probably be doing that.



Does any of this sound sane/rational/correct?
02/03/2009 03:37:14 AM Chuck Hagenbuch Comment #4 Reply to this comment
Right, it's not relevant for template evaluation, but Horde::selfUrl() 
can contain raw query parameters, so those need to be escaped 
somewhere along the way. I didn't see that happening already, but if 
it is, then yes, the commits are irrelevant (and incorrect).
02/03/2009 03:35:18 AM Michael Slusarz Comment #3 Reply to this comment
The show-header-action urls are htmlencoded twice. I think this is
happening in the Util::removeParameter() call not correctly
determining whether the url is already encoded.
No - that's not it.  The problem is that the URL, when generated, is 
htmlencoded and the & separator is also htmlencoded.  Then, for some 
reason, we are calling htmlspecialchars() again when injecting into 
the template object.



So I guess I don't understand what this commit is trying to protect against:



-----



   fix some unescaped output



   Revision     Changes    Path

   1.699.2.375  +2 -0      imp/docs/CHANGES

   2.560.4.58   +6 -6      imp/message.php

   2.79.6.19    +3 -3      imp/pgp.php

   2.48.4.14    +3 -3      imp/smime.php



   Chora Links:

   
http://cvs.horde.org/diff.php/imp/docs/CHANGES?rt=horde&r1=1.699.2.374&r2=1.699.2.375&ty=u

   
http://cvs.horde.org/diff.php/imp/message.php?rt=horde&r1=2.560.4.57&r2=2.560.4.58&ty=u

   
http://cvs.horde.org/diff.php/imp/pgp.php?rt=horde&r1=2.79.6.18&r2=2.79.6.19&ty=u

   
http://cvs.horde.org/diff.php/imp/smime.php?rt=horde&r1=2.48.4.13&r2=2.48.4.14&ty=u



-----



Removing those htmlspecialchars() calls fixes things.  This is *not* 
the false positive security vulnerability that Gunnar reported 
(QUERY_STRING data is irrelevant for purposes of Horde_Template 
evaluation).



Sorry if I didn't catch this previously - I've been up in the 
mountains a bunch the past few weeks and haven't had a bunch of time 
to peruse list traffic.
02/01/2009 11:01:26 PM Jan Schneider Comment #2 Reply to this comment
The show-header-action urls are htmlencoded twice. I think this is 
happening in the Util::removeParameter() call not correctly 
determining whether the url is already encoded.
02/01/2009 06:19:36 PM Chuck Hagenbuch Assigned to Chuck Hagenbuch
Assigned to Michael Slusarz
Assigned to Horde DevelopersHorde Developers
State ⇒ Assigned
 
02/01/2009 06:12:12 PM mtecles (at) biof (dot) ufrj (dot) br Comment #1
Priority ⇒ 1. Low
State ⇒ Unconfirmed
New Attachment: Re___horde__problem_with_showing_the_mail-headers.eml Download
Patch ⇒ No
Milestone ⇒
Queue ⇒ IMP
Type ⇒ Bug
Summary ⇒ Message option "Show All Headers" causes error		 Reply to this comment
Upgrading Horde from 3.3.2 to 3.3.3, IMP 4.3.2 works fine. Then, I 
upgraded IMP to 4.3.3. Opening a message, clicking "Show All Headers" 
it falls back to INBOX with the error "Requested message not found.".
New Ticket
My Tickets
Search
Query Builder
Reports

Saved Queries

Open Bugs
Bugs waiting for Feedback
Open Bugs in Releases
Open Enhancements
Enhancements waiting for Feedback
Bugs with Patches
Enhancements with Patches
Release Showstoppers
Stalled Tickets
New Tickets
Horde 5 Showstoppers