bugfix: changed CryptPolicy to use SafeConfigParser (as it really should have all along)

* this may break parsing of some files which have "vary_rounds = 10%", that should now read "vary_rounds = 10%%".
* currently detecting that case, and falling back to old behavior + userwarning
* passlib 1.6 will let this be fatal.

6 files changed, 75 insertions, 9 deletions

diff --git a/CHANGES b/CHANGES
index 4dce9d8..7ad6576 100644
--- a/CHANGES
+++ b/CHANGES
@@ -11,6 +11,16 @@ Release History
         * added support for Cryptacular's PBKDF2 format
         * added support for using BCryptor as BCrypt backend
     
+    CryptContext
+        * interpolation deprecation:
+        
+          CryptPolicy.from_file() / .from_string() now
+          use SafeConfigParser instead of ConfigParser.
+          This may cause some existing config files containing unescaped ``%``
+          to result in errors; passlib 1.5 will demote these to warnings,
+          but any extant config files should be updated,
+          as the errors will be fatal in passlib 1.6.
+    
     Documentation
         * added quickstart guide to documentation
         
diff --git a/docs/lib/passlib.context-options.rst b/docs/lib/passlib.context-options.rst
index 8a6a19e..3792da1 100644
--- a/docs/lib/passlib.context-options.rst
+++ b/docs/lib/passlib.context-options.rst
@@ -206,7 +206,8 @@ A sample policy file:
     min_verify_time = 0.1
 
     #set some common options for all schemes
-    all.vary_rounds = 10%
+    all.vary_rounds = 10%%
+        ; NOTE the '%' above has to be escaped due to configparser interpolation
 
     #setup some hash-specific defaults
     sha512_crypt.min_rounds = 40000
diff --git a/docs/lib/passlib.context-usage.rst b/docs/lib/passlib.context-usage.rst
index 8464a1e..8609d6d 100644
--- a/docs/lib/passlib.context-usage.rst
+++ b/docs/lib/passlib.context-usage.rst
@@ -142,7 +142,8 @@ applications with advanced policy requirements may want to create a hash policy
     ;the 'vary' field will cause each new hash to randomly vary
     ;from the default by the specified %.
     pbkdf2_sha1.default_rounds = 20000
-    pbkdf2_sha1.vary_rounds = 10%
+    pbkdf2_sha1.vary_rounds = 10%%
+        ; NOTE the '%' above has to be doubled due to configparser interpolation
 
     ;applications can choose to treat certain user accounts differently,
     ;by assigning different types of account to a 'user category',
diff --git a/passlib/context.py b/passlib/context.py
index 0bd5f6a..04373eb 100644
--- a/passlib/context.py
+++ b/passlib/context.py
@@ -5,7 +5,7 @@
 from __future__ import with_statement
 #core
 from cStringIO import StringIO
-from ConfigParser import ConfigParser
+from ConfigParser import ConfigParser, SafeConfigParser, InterpolationSyntaxError
 import inspect
 import re
 import hashlib
@@ -88,6 +88,29 @@ def parse_policy_items(source):
         value = _parse_policy_value(cat, name, opt, value)
         yield cat, name, opt, value
 
+def _is_legacy_parse_error(err):
+    "helper for parsing config files"
+    #NOTE: passlib 1.4 and earlier used ConfigParser,
+    # when they should have been using SafeConfigParser
+    # (which passlib 1.5+ switched to)
+    # this has no real security effects re: passlib,
+    # but some 1.4 config files that have "vary_rounds = 10%"
+    # may throw an error under SafeConfigParser,
+    # and should read "vary_rounds = 10%%"
+    #
+    # passlib 1.6 and on will only use SafeConfigParser,
+    # but passlib 1.5 tries to detect the above 10% error,
+    # issue a warning, and retry w/ ConfigParser,
+    # for backward compat.
+    #
+    # this function's purpose is to encapsulate that
+    # backward-compat behavior.
+    value = err.args[0]
+    #'%' must be followed by '%' or '(', found: '%'
+    if value == "'%' must be followed by '%' or '(', found: '%'":
+        return True
+    return False
+
 class CryptPolicy(object):
     """stores configuration options for a CryptContext object.
 
@@ -141,10 +164,21 @@ class CryptPolicy(object):
 
         :returns: new CryptPolicy instance.
         """
-        p = ConfigParser()
+        p = SafeConfigParser()
         if not p.read([path]):
             raise EnvironmentError("failed to read config file")
-        return cls(**dict(p.items(section)))
+        try:
+            items = p.items(section)
+        except InterpolationSyntaxError, err:
+            if not _is_legacy_parse_error(err):
+                raise
+            #support for deprecated 1.4 behavior, will be removed in 1.6
+            warn("from_path(): the file %r contains an unescaped '%%', this will be fatal in passlib 1.6" % (path,), stacklevel=2)
+            p = ConfigParser()
+            if not p.read([path]):
+                raise EnvironmentError("failed to read config file")
+            items = p.items(section)
+        return cls(**dict(items))
 
     @classmethod
     def from_string(cls, source, section="passlib"):
@@ -155,10 +189,21 @@ class CryptPolicy(object):
 
         :returns: new CryptPolicy instance.
         """
-        p = ConfigParser()
         b = StringIO(source)
+        p = SafeConfigParser()
         p.readfp(b)
-        return cls(**dict(p.items(section)))
+        try:
+            items = p.items(section)
+        except InterpolationSyntaxError, err:
+            if not _is_legacy_parse_error(err):
+                raise
+            #support for deprecated 1.4 behavior, will be removed in 1.6
+            warn("from_string(): the provided string contains an unescaped '%', this will be fatal in passlib 1.6", stacklevel=2)
+            p = ConfigParser()
+            b.seek(0)
+            p.readfp(b)
+            items = p.items(section)
+        return cls(**dict(items))
 
     @classmethod
     def from_source(cls, source):
@@ -560,10 +605,16 @@ class CryptPolicy(object):
         "return policy as dictionary of keywords"
         return dict(self.iter_config(resolve=resolve))
 
+    def _escape_ini_pair(self, k, v):
+        if isinstance(v, str):
+            v = v.replace("%", "%%") #escape any percent signs.
+        return k,v
+
     def _write_to_parser(self, parser, section):
         "helper for to_string / to_file"
         parser.add_section(section)
         for k,v in self.iter_config(ini=True):
+            k,v = self._escape_ini_pair(k,v)
             parser.set(section, k,v)
 
     def to_file(self, stream, section="passlib"):
@@ -697,6 +748,9 @@ class CryptContext(object):
                     if isinstance(vr, str):
                         rc = getattr(handler, "rounds_cost", "linear")
                         vr = int(vr.rstrip("%"))
+                            #NOTE: deliberately strip >1 %,
+                            #in case an interpolation-escaped %%
+                            #makes it through to here.
                         assert 0 <= vr < 100
                         if rc == "log2":
                             #let % variance scale the number of actual rounds, not the logarithmic value
diff --git a/passlib/default.cfg b/passlib/default.cfg
index b45e048..0fa4836 100644
--- a/passlib/default.cfg
+++ b/passlib/default.cfg
@@ -8,7 +8,7 @@
 
 #TODO: need to generate min rounds for specific cpu speed & verify time limitations
 
-all.vary_rounds = 10%
+all.vary_rounds = 10%%
 
 bsdi_crypt.default_rounds = 30000
 bcrypt.default_rounds = 10
diff --git a/passlib/tests/test_context.py b/passlib/tests/test_context.py
index e86146f..650c205 100644
--- a/passlib/tests/test_context.py
+++ b/passlib/tests/test_context.py
@@ -42,7 +42,7 @@ class CryptPolicyTest(TestCase):
 [passlib]
 schemes = des_crypt, md5_crypt, bsdi_crypt, sha512_crypt
 default = md5_crypt
-all.vary_rounds = 10%
+all.vary_rounds = 10%%
 bsdi_crypt.max_rounds = 30000
 bsdi_crypt.default_rounds = 25000
 sha512_crypt.max_rounds = 50000