5.3.0-git
2014-12-23

[#8105] sieve driver doesn't check header field validity
Summary sieve driver doesn't check header field validity
Queue Ingo
Queue Version 1.2.1
Type Enhancement
State Accepted
Priority 1. Low
Owners
Requester noethen (at) uni-paderborn (dot) de
Created 2009-03-20 (2104 days ago)
Due
Updated 2009-04-04 (2089 days ago)
Assigned 2009-03-24 (2100 days ago)
Resolved
Milestone
Patch No

History
2009-04-04 09:07:44 Jan Schneider Type ⇒ Enhancement
State ⇒ Accepted
Priority ⇒ 1. Low
 
2009-03-24 17:43:46 matthias (dot) andree (at) uni-paderborn (dot) de Comment #14 Reply to this comment
Both the maildrop and procmail script generators add the trailing
colon to the header.  Can you write a patch for lib/Script/sieve.php
to check to colons?
Whom are you asking? Me? If so, sorry, I can't write PHP and have no 
desire to.
2009-03-24 16:47:06 Matt Selsky Summary ⇒ sieve driver doesn't check header field validity
State ⇒ Feedback
Patch ⇒ No
 
2009-03-24 16:29:23 Matt Selsky Comment #13 Reply to this comment
Both the maildrop and procmail script generators add the trailing 
colon to the header.  Can you write a patch for lib/Script/sieve.php 
to check to colons?
2009-03-24 16:18:35 matthias (dot) andree (at) uni-paderborn (dot) de Comment #12 Reply to this comment
Checking for valid characters would work for me, however keep in mind 
that some drivers that do raw or regexp matching on full header lines 
(for instance, maildrop and procmail) might need the trailing colon to 
avoid matching unrelated data on corrupt messages - be sure to check 
if the corresponding output drivers in Ingo add those colons.
2009-03-24 15:43:44 Matt Selsky Comment #11 Reply to this comment
What if we just check the user supplied header for validity according 
to RFC 2822 section 2.2?



"A field name MUST be composed of printable US-ASCII characters (i.e., 
characters that have values between 33 and 126, inclusive), except 
colon."
2009-03-24 09:13:53 matthias (dot) andree (at) uni-paderborn (dot) de Comment #10 Reply to this comment
Read again what I wrote. Cyrus/timsieved is already checking the
validity of the script and returns an error message where
appropriate. If it doesn't do that for your, then your server is
broken period.
Cyrus isn't broken, as the error is reported via sieveshell, but not 
without Heiko's patch to some part of the Horde framework I do not see.



The problem remains, generation of invalid scripts. I think Ingo, 
being able to generate output for various different filter engines 
(sieve only one of them), should normalize these filters - they are 
configured by non-technical users, and expecting them to know the 
details of Sieve syntax is expecting too much anyways.
2009-03-23 19:02:35 Matt Selsky Comment #9 Reply to this comment
When I try what you describe, I get the following error message with 
Cyrus 2.3.13, Net_Sieve 1.1.6 and Ingo 1.2.x:



Error:

There was an error activating the script. The driver said: {81}script 
errors: line 18: header 'from:': not a valid header for an address test
2009-03-23 16:31:55 Jan Schneider Comment #8 Reply to this comment
Read again what I wrote. Cyrus/timsieved is already checking the 
validity of the script and returns an error message where appropriate. 
If it doesn't do that for your, then your server is broken period.
2009-03-23 15:18:02 matthias (dot) andree (at) uni-paderborn (dot) de Comment #7 Reply to this comment
BTW, see http://www.ietf.org/rfc/rfc3028.txt (RFC3028, "Sieve: A Mail 
Filtering Language") section 2.4.2.2:

    '[...]

    A header name never contains a colon.  The "From" header refers to a

    line beginning "From:" (or "From   :", etc.).  No header will match

    the string "From:" due to the trailing colon. [...]'



So Horde should make sure that either (1) the web interface rejects 
filters where custom header names contain the colon, or (2) the 
component that generates the Sieve script strips that colon.
2009-03-23 15:05:25 matthias (dot) andree (at) uni-paderborn (dot) de Comment #6 Reply to this comment
Care to explain why you're trying to shift the blame on somebody else, 
rather than reading and understanding what is presented?



Why/how this would be a Cyrus bug if (1) Horde/Ingo creates invalid 
sieve scripts through improper input validation or output generation 
(excess colon) and (2) some glue then silently drops the error message 
to the floor?



That makes two bugs in Horde here and zero in Cyrus timsieved - even 
if you cannot reproduce the "dropped error message" part (No. 2 
above), the other bug remains.
2009-03-23 14:38:57 Jan Schneider Comment #5
State ⇒ Not A Bug
Reply to this comment
That's a broken timsieved then.
2009-03-23 14:16:34 noethen (at) uni-paderborn (dot) de Comment #4 Reply to this comment
(b) doesn't happen to me, I get an error back if I create invalid
rules. Do you have the latest Net_Sieve installed?
Yes, we have installed Net_Sieve 1.1.6 stable (the current release). 
Is this function server dependent? My debugging showed, that the 
script is uploaded to the server and the server returns "ok". When i 
access the sieve file by sieveshell command i only get the list of 
valid rules, but when i load the script from the server by pear lib i 
get a pear error that the script has errors.



"Beim Aktivieren des Skripts ist ein Fehler aufgetreten. Antwort des 
Treibers: {65}script errors: line 5: header 'replay-to:': not a valid 
header"
2009-03-23 11:09:31 Jan Schneider Comment #3
State ⇒ Feedback
Reply to this comment
(b) doesn't happen to me, I get an error back if I create invalid 
rules. Do you have the latest Net_Sieve installed?
2009-03-20 15:42:22 matthias (dot) andree (at) uni-paderborn (dot) de Comment #2 Reply to this comment
To detail on the problem, (a) Ingo generates a rule that Cyrus's 
timsieved will reject (it expects header name matches without colon, 
as Heiko already stated). (b) The attempt to upload this invalid Sieve 
script fails SILENTLY, without error message, while Cyrus continues 
with the previous edition of the script. (c) the user interface in 
however presents the new filter.



The fix should be two-fold: (a) fix Ingo so it does not create invalid 
rules in the first place (stripping the trailing colon should be in 
order for user-specified header filters, otherwise it should 
complain), (b) - suggested by Heiko above - make sure to display the 
error if the Sieve script cannot be uploaded to the server.


2009-03-20 14:38:06 noethen (at) uni-paderborn (dot) de Comment #1
State ⇒ Unconfirmed
Patch ⇒ Yes
Milestone ⇒
Queue ⇒ Ingo
Summary ⇒ Driver Timsieve saves invalid sieve file without displaying error message
Type ⇒ Bug
Priority ⇒ 1. Low
Reply to this comment
Hello,



when the user adds a invalid rule - for ex. over custom header 
"replay-to:" where the colon is wrong, the user gets no feedback. The 
script is saved to the server (the server ignores the wrong filter 
rule) and the user doesn't know why the rule doesn't work.



Here is my patched function - as in fact we include the 
"INGO-Sieve-Script" from a main sieve script, so i disabled the 
activation - only the script is uploaded/installed.



    /**

      * Sets a script running on the backend.

      *

      * @param string $script    The sieve script.

      *

      * @return mixed  True on success, PEAR_Error on error.

      */

     function setScriptActive($script)

     {

         $res = $this->_connect();

         if (is_a($res, 'PEAR_Error')) {

             return $res;

         }



         if (!strlen($script)) {

             return $this->_sieve->setActive('');

         }



         $res = $this->_sieve->haveSpace($this->_params['scriptname'],

                                         strlen($script));

         if (is_a($res, 'PEAR_ERROR')) {

             return $res;

         }



         // IMT PATCHED



/*

         return $this->_sieve->installScript($this->_params['scriptname'],

                                             $script, true);

*/

          $return =  $this->_sieve->installScript($this->_params['scriptname'],

                                                      $script, false);





         // Check if the uploaded script is valid - server doesn't 
return error when script is invalid, so here i fetch the script again 
and check for errors.



         $tmp = $this->_sieve->getScript($this->_params['scriptname']);



         if (is_a($tmp, 'PEAR_ERROR'))

                 return $tmp;





         return $return;



     }