6.0.0-git
2019-04-23

[#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 2009-02-01 (3733 days ago)
Due
Updated 2010-01-12 (3388 days ago)
Assigned 2009-02-17 (3717 days ago)
Resolved 2009-02-23 (3711 days ago)
Milestone
Patch No

History
2009-04-14 14:52:01 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]
2009-04-14 14:48:02 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.
2009-04-14 14:45:50 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.
2009-04-13 18:12:16 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...
2009-04-13 18:10:22 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...
2009-04-10 20:54:58 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
2009-03-18 20:20:36 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.
2009-03-18 20:01:21 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?
2009-02-23 12:12:08 Jan Schneider State ⇒ Resolved
 
2009-02-22 14:56:22 horde (at) immerda (dot) ch Comment #22 Reply to this comment
We can confirm that the PGP problems are now fixed as well.



thanks!
2009-02-20 12:53:47 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.
2009-02-20 05:25:40 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.
2009-02-17 19:43:06 Chuck Hagenbuch Comment #19
Taken from Chuck Hagenbuch
State ⇒ Feedback
Priority ⇒ 2. Medium
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?
2009-02-17 07:41:20 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.
2009-02-17 07:32:13 CVS Commit Comment #17 Reply to this comment
2009-02-14 20:12:31 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?
2009-02-11 21:40:46 Michael Slusarz State ⇒ Resolved
 
2009-02-10 18:56:46 Michael Slusarz Comment #15 Reply to this comment
I think I have fixed all issues now.
2009-02-10 18:49:40 CVS Commit Comment #14 Reply to this comment
2009-02-07 12:31:34 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?
2009-02-06 07:46:15 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.
2009-02-06 07:43:48 CVS Commit Comment #10 Reply to this comment
2009-02-05 21:14:01 Michael Slusarz Comment #9
Priority ⇒ 3. High
Reply to this comment
The {pgp/smime}.php issues should be fixed now.
2009-02-04 21:37:06 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.
2009-02-04 21:28:28 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.
2009-02-03 03:47:28 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?
2009-02-03 03:37:14 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).
2009-02-03 03:35:18 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.
2009-02-01 23:01:26 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.
2009-02-01 18:19:36 Chuck Hagenbuch Assigned to Chuck Hagenbuch
Assigned to Michael Slusarz
Assigned to Horde DevelopersHorde Developers
State ⇒ Assigned
 
2009-02-01 18:12:12 mtecles (at) biof (dot) ufrj (dot) br Comment #1
Type ⇒ Bug
State ⇒ Unconfirmed
Priority ⇒ 1. Low
Summary ⇒ Message option "Show All Headers" causes error
Queue ⇒ IMP
Milestone ⇒
Patch ⇒ No
New Attachment: Re___horde__problem_with_showing_the_mail-headers.eml Download
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.".

Saved Queries