[#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, slusarz@horde.org
Requester mtecles@biof.ufrj.br
Created 2009-02-01 (3883 days ago)
Due
Updated 2010-01-12 (3538 days ago)
Assigned 2009-02-17 (3867 days ago)
Resolved 2009-02-23 (3861 days ago)
Milestone
Patch No

Comments
mtecles@biof.ufrj.br 2009-02-01 18:12:12
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.".

Jan Schneider <jan@horde.org> 2009-02-01 23:01:26
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.

Michael Slusarz <slusarz@horde.org> 2009-02-03 03:35:18
> 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.

Chuck Hagenbuch <chuck@horde.org> 2009-02-03 03:37:14
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).

Michael Slusarz <slusarz@horde.org> 2009-02-03 03:47:28
> 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?

Chuck Hagenbuch <chuck@horde.org> 2009-02-04 21:28:28
> 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.

Chuck Hagenbuch <chuck@horde.org> 2009-02-04 21:37:06
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.

CVS Commit <cvs@lists.horde.org> 2009-02-05 21:13:14

Michael Slusarz <slusarz@horde.org> 2009-02-05 21:14:01
The {pgp/smime}.php issues should be fixed now.

CVS Commit <cvs@lists.horde.org> 2009-02-06 07:43:48

Michael Slusarz <slusarz@horde.org> 2009-02-06 07:46:15
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.

horde@immerda.ch 2009-02-07 12:31:34
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?

CVS Commit <cvs@lists.horde.org> 2009-02-10 18:47:42

CVS Commit <cvs@lists.horde.org> 2009-02-10 18:49:40

Michael Slusarz <slusarz@horde.org> 2009-02-10 18:56:46
I think I have fixed all issues now.

Chuck Hagenbuch <chuck@horde.org> 2009-02-14 20:12:31
> 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?

CVS Commit <cvs@lists.horde.org> 2009-02-17 07:32:13

Michael Slusarz <slusarz@horde.org> 2009-02-17 07:41:20
>> 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.

Chuck Hagenbuch <chuck@horde.org> 2009-02-17 19:43:06
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?

Michael Slusarz <slusarz@horde.org> 2009-02-20 05:25:40
> 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.

Chuck Hagenbuch <chuck@horde.org> 2009-02-20 12:53:47
> 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.

horde@immerda.ch 2009-02-22 14:56:22
We can confirm that the PGP problems are now fixed as well.



thanks!

samuel.wolf@wolf-maschinenbau.de 2009-03-18 20:01:21
> We can confirm that the PGP problems are now fixed as well.

>

> thanks!

Is a patch (diff) file available?

Michael Slusarz <slusarz@horde.org> 2009-03-18 20:20:36
>> 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.

ivan@spadz.com 2009-04-10 20:54:58
Based on all message from there, this is a global patch for bug 7926...



VoilĂ ...



Ivan

amg1127@cefetrs.tche.br 2009-04-13 18:10:22
> Based on all message from there, this is a global patch for bug 7926...

>

> VoilĂ ...

>

> Ivan



It seems that global patch is empty...

amg1127@cefetrs.tche.br 2009-04-13 18:12:16
Why I can't watch this ticket?



>

> It seems that global patch is empty...



kristian.lance@crc.ca 2009-04-14 14:45:50
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.



kristian.lance@crc.gc.ca 2009-04-14 14:48:02
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.



kristian.lance@crc.ca 2009-04-14 14:52:01
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.



> 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.

>



CVS Commit <cvs@lists.horde.org> 2010-01-12 23:58:53

CVS Commit <cvs@lists.horde.org> 2010-01-12 23:59:10

CVS Commit <cvs@lists.horde.org> 2010-01-12 23:59:23