Changes have been made in Git for this ticket:

Bug #7926: Fix URL escaping

http://git.horde.org/diff.php/imp/message.php?rt=horde-git&r1=bd397c0db622da9eeb9859ce9c112dca19d3a4b2&r2=0f7499464586494165c1ebae472bf1b812c83c4d

Changes have been made in Git for this ticket:

Bug #7926: Fix [un]escaping of URLs

http://git.horde.org/diff.php/imp/docs/CHANGES?rt=horde-git&r1=a97cf3d0875a23ab5e05aee1f7b13793d4f7a4a0&r2=5f9f6c660f8576cd79308caabcca45bf5eed9f6e
http://git.horde.org/diff.php/imp/pgp.php?rt=horde-git&r1=d06f5dbc9d3a8328b25ce2f866ab6b5fedcb66b9&r2=5f9f6c660f8576cd79308caabcca45bf5eed9f6e
http://git.horde.org/diff.php/imp/smime.php?rt=horde-git&r1=d06f5dbc9d3a8328b25ce2f866ab6b5fedcb66b9&r2=5f9f6c660f8576cd79308caabcca45bf5eed9f6e

Changes have been made in Git for this ticket:

Bug #7926: Make sure we decode before re-encoding.

http://git.horde.org/diff.php/imp/pgp.php?rt=horde-git&r1=ee6ff322e0a8a5aa91dbffd0bda962ce51427d03&r2=7ed72077f83a9ecb54f92e92afb2aff87b40a414
http://git.horde.org/diff.php/imp/smime.php?rt=horde-git&r1=ee6ff322e0a8a5aa91dbffd0bda962ce51427d03&r2=7ed72077f83a9ecb54f92e92afb2aff87b40a414

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

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

Why I can't watch this ticket?

It seems that global patch is empty...

Based on all message from there, this is a global patch for bug 7926...



Voilà...



Ivan

CVS does not easily allow this to be done.  The individual patches can 
all be found in this ticket, however.

Is a patch (diff) file available?

We can confirm that the PGP problems are now fixed as well.



thanks!

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.

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.

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?

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.

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

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?

I think I have fixed all issues now.

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

Changes have been made in CVS for this ticket:

http://cvs.horde.org/diff.php/imp/lib/Crypt/PGP.php?rt=horde&r1=1.90.2.23&r2=1.90.2.24&ty=u
http://cvs.horde.org/diff.php/imp/lib/Crypt/SMIME.php?rt=horde&r1=1.45.2.23&r2=1.45.2.24&ty=u
http://cvs.horde.org/diff.php/imp/pgp.php?rt=horde&r1=2.79.6.20&r2=2.79.6.21&ty=u
http://cvs.horde.org/diff.php/imp/smime.php?rt=horde&r1=2.48.4.15&r2=2.48.4.16&ty=u

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?

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.

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

The {pgp/smime}.php issues should be fixed now.

Changes have been made in CVS for this ticket:

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

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.

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?
If we already do that in those files, that seems fine.

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?

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

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.

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.

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