[Tickets #13379] Re: Discontinue eval

[noreply at bugs.horde.org]

DO NOT REPLY TO THIS MESSAGE. THIS EMAIL ADDRESS IS NOT MONITORED.

Ticket URL: http://bugs.horde.org/ticket/13379
------------------------------------------------------------------------------
  Ticket             | 13379
  Updated By         | Michael Slusarz <slusarz at horde.org>
  Summary            | Discontinue eval
  Queue              | IMP
  Version            | 6.2.0
  Type               | Enhancement
-State              | New
+State              | Feedback
-Priority           | 2. Medium
+Priority           | 1. Low
  Milestone          |
  Patch              |
  Owners             |
------------------------------------------------------------------------------


Michael Slusarz <slusarz at horde.org> (2014-07-21 18:06) wrote:

> We are in the process of deploying csp headers for horde. Through  
> that we discovered usage of js eval in horde. Especially for a  
> webmail and the associated danger of injections it would be nice if  
> horde could discontinue the use of eval (and maybe even inline  
> js/css).

Use of eval() is perfectly acceptable.  I see way too many people who  
say things like "eval() should NEVER be used".  Which is flat-out  
wrong.  eval() is no more dangerous than anything else - meaning it  
can be abused if used incorrectly.

Nobody has shown that our code is subject to any security issue.

> One particular case we see is in DimpBase>>loadPreview. It seems it  
> is only used atm to display modal dialogs from  
> horde/imp/lib/Ajax/Imple/PassphraseDialog.php.

Sure.  And what's wrong with this?

We shouldn't be loading this code at page load time, since the user  
may never use this code.  We should only run it on demand.  (This is  
the tradeoff for trying to separate the code layer from the display  
layer.)

> Then in the dynamic composer:
> new Function("t", "return t.sub(/<[^>]*>$/,  
> \"\").strip().escapeHTML()") afaik this is a static string, so why  
> even new Function?

Easier to read.  Brackets are difficult when viewing embedded in PHP code.

> Then there are some reports from  
> /imp/dynamic.php?page=message&mailbox=... that I was not able to  
> tackle yet.
>
> There might be more occurrences.
>
> If you are interested in fixing those, we can provide more data as  
> soon as we have better processing for the logs.

Realize that there is a bunch of javascript library APIs that we  
currently use that require this kind of dynamic code.  I believe  
autocomplete is an example of this.  So this can't be removed (at  
least for H5).

I'm not saying that removing eval() is not something we should strive  
for from a *design* perspective.  But I'm not sure what your  
alternative is.  There is no difference, security wise, between  
separate script files and eval'd code, as long as the eval'd code is  
properly escaped.  And I really don't want to have to lump all  
javascript code into a single bundle when it is likely that the code  
is never needed on the page.