From: Gunnar Wrobel <p@rdus.de>
Date: Wed, 17 Nov 2010 16:30:50 +0000 (+0100)
Subject: Do not let Crypt_Blowfish die on exceptions.
X-Git-Url: https://git.internetallee.de/?a=commitdiff_plain;h=8788576ba057f7b67c1e402271cd02d701ddda39;p=horde.git

Do not let Crypt_Blowfish die on exceptions.

I'm not 100% certain this commit introduces the best way of dealing
with Crypt_Blowfish errors. As far as I can see there are four
situations that Crypt_Blowfish considers to be an error. In each of
these cases the library will unconditionally raise a PEAR error with
the mode "PEAR_ERROR_DIE". Which will unconditionally kill us.

Apparently one such situation has already been hit (commit
271ce27ceee4749c667b3d0b51be8947c915472d).

Now we are double checking input in both Horde_Secret and
Crypt_Blowfish but on the other hand we don't have to check the return
values anymore.
---

diff --git a/framework/Secret/lib/Horde/Secret.php b/framework/Secret/lib/Horde/Secret.php
index 4cfc3e2df..f6da86486 100644
--- a/framework/Secret/lib/Horde/Secret.php
+++ b/framework/Secret/lib/Horde/Secret.php
@@ -70,14 +70,14 @@ class Horde_Secret
      */
     public function write($key, $message)
     {
+        if (!is_string($message)) {
+            throw new Horde_Secret_Exception('Plain text must be a string', 0);
+        }
+
         $val = strlen($key) && strlen($message)
             ? $this->_getCipherOb($key)->encrypt($message)
             : '';
 
-        if ($val instanceof PEAR_Error) {
-            throw new Horde_Secret_Exception($val);
-        }
-
         return $val;
     }
 
@@ -92,14 +92,14 @@ class Horde_Secret
      */
     public function read($key, $ciphertext)
     {
+        if (!is_string($ciphertext)) {
+            throw new Horde_Secret_Exception('Chiper text must be a string', 1);
+        }
+
         $val = strlen($key) && strlen($ciphertext)
             ? $this->_getCipherOb($key)->decrypt($ciphertext)
             : '';
 
-        if ($val instanceof PEAR_Error) {
-            throw new Horde_Secret_Exception($val);
-        }
-
         /* Bug #9121: Data may be null padded - need to remove this
          * padding. */
         return rtrim($val, "\0");
@@ -115,6 +115,13 @@ class Horde_Secret
      */
     protected function _getCipherOb($key)
     {
+        if (!is_string($key)) {
+            throw new Horde_Secret_Exception('Key must be a string', 2);
+        }
+        if (strlen($key) > 56) {
+            throw new Horde_Secret_Exception('Key must be less than 56 characters and non-zero. Supplied key length: ' . strlen($key), 3);
+        }
+
         $idx = hash('md5', $key);
 
         if (!isset($this->_cipherCache[$idx])) {
diff --git a/framework/Secret/lib/Horde/Secret/Exception.php b/framework/Secret/lib/Horde/Secret/Exception.php
index acd034738..98aa8980a 100644
--- a/framework/Secret/lib/Horde/Secret/Exception.php
+++ b/framework/Secret/lib/Horde/Secret/Exception.php
@@ -11,6 +11,6 @@
  * @category Horde
  * @package  Horde_Secret
  */
-class Horde_Secret_Exception extends Horde_Exception_Prior
+class Horde_Secret_Exception extends Horde_Exception
 {
 }
diff --git a/framework/Secret/test/Horde/Secret/Autoload.php b/framework/Secret/test/Horde/Secret/Autoload.php
index 95d230dbc..c7ad5cc60 100644
--- a/framework/Secret/test/Horde/Secret/Autoload.php
+++ b/framework/Secret/test/Horde/Secret/Autoload.php
@@ -19,3 +19,4 @@ error_reporting(E_ALL | E_STRICT);
 
 /** Needed for PEAR_Error. */
 require_once 'PEAR.php';
+require_once dirname(__FILE__) . '/Stub/Message.php';
\ No newline at end of file
diff --git a/framework/Secret/test/Horde/Secret/Stub/Message.php b/framework/Secret/test/Horde/Secret/Stub/Message.php
new file mode 100644
index 000000000..5f0eb2970
--- /dev/null
+++ b/framework/Secret/test/Horde/Secret/Stub/Message.php
@@ -0,0 +1,20 @@
+<?php
+/**
+ * A class that pretends to be a string.
+ *
+ * PHP version 5
+ *
+ * @category   Horde
+ * @package    Secret
+ * @subpackage UnitTests
+ * @author     Gunnar Wrobel <wrobel@pardus.de>
+ * @license    http://www.fsf.org/copyleft/lgpl.html LGPL
+ * @link       http://pear.horde.org/index.php?package=Secret
+ */
+class Horde_Secret_Stub_Message
+{
+    public function __toString()
+    {
+        return 'secret';
+    }
+}
\ No newline at end of file
diff --git a/framework/Secret/test/Horde/Secret/Unit/SecretTest.php b/framework/Secret/test/Horde/Secret/Unit/SecretTest.php
index 9a1e5227e..d295d0bb3 100644
--- a/framework/Secret/test/Horde/Secret/Unit/SecretTest.php
+++ b/framework/Secret/test/Horde/Secret/Unit/SecretTest.php
@@ -78,4 +78,45 @@ class Horde_Secret_Unit_SecretTest extends PHPUnit_Framework_TestCase
         $this->assertEquals($plaintext, $secret->read($key, $secret->write($key, $plaintext . "\x00")));
     }
 
-}
+    /**
+     * @expectedException Horde_Secret_Exception
+     */
+    public function testWriteException()
+    {
+        $secret = new Horde_Secret();
+        $secret->write("\x88", array());
+    }
+
+    /**
+     * @expectedException Horde_Secret_Exception
+     */
+    public function testReadException()
+    {
+        $secret = new Horde_Secret();
+        $secret->read("\x88", array());
+    }
+
+    /**
+     * @expectedException Horde_Secret_Exception
+     */
+    public function testKeyException()
+    {
+        $secret = new Horde_Secret();
+        $secret->read(new Horde_Secret_Stub_Message(), "\x01");
+    }
+
+    /**
+     * @expectedException Horde_Secret_Exception
+     */
+    public function testLongKeyException()
+    {
+        $secret = new Horde_Secret();
+        $secret->read('012345678901234567890123456789012345678901234567890123456789', "\x01");
+    }
+
+    public function testShortKey()
+    {
+        $secret = new Horde_Secret();
+        $this->assertEquals('', $secret->read('', "\x01"));
+    }
+}
\ No newline at end of file