Repository: line/line-bot-sdk-python
Pull request #13 (2016-10-18 16:41:32)

hmac.compare_digest is not in earlier version python2.7.7
earlier version python2.7.7
hmac doesn't support compare_digest
so I implement workaround for this. :)

be-hase: Hi, @charsyam 
Thanks for your feedback.  :smile:
But str comparing is not good because of timing attack.
( https://en.wikipedia.org/wiki/Timing_attack ) 

OK. In the near future, I will fix this issue and release 1.0.2 version.
darjeeling: maybe @charsyam can get hint here https://github.com/python/cpython/blob/2.7/Modules/operator.c#L289
charsyam: Hi, @be-hase 

could you review this? I implemented safe_compare_digest func to refer tscmp
be-hase: Oh... thanks for your patch. 👍 
okdtsk: One more thing, we adopt Google style docstring but yours is reST format. Can you rewrite your function's docstring as Google one? 

Google python style guide
https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments

This SO question will help you :)
http://stackoverflow.com/a/24385103
okdtsk: Why does not use `len2` directly when comparing to `len1`?
And the line `if len1 != length` can be rewritten `else` statement under `if len1 == length`, which will have same meaning, same cost to avoid time at track and more readable.
charsyam: @okdtsk Good Catch, I will apply your review to use len2 directlry
and I have a question about docstring.
python webhook.py and other code just follow reST format not Google Style.
could you please check this?
be-hase: I'm sorry reply late.

> docstring
Yes, we use reST format. Sorry @okdtsk 😢 

This feature is sensitive, so I want a test code.
okdtsk: @charsyam @be-hase 
Ah sorry I checked previous code. Please ignore my comment about docstring. 😭 
be-hase: And...
I think that we can simplify like below.

```
def compare_digest(target1, target2):
    if len(target1) != len(target1):
        return False

    result = 0
    for x, y in zip(target1, target2):
        result |= ord(x) ^ ord(y)

    return result == 0
```
charsyam: @be-hase Yes, I also worry about that. How we can test it well?
because I think this verify consuming time is equals.
charsyam: @be-hase and We can't just return False when length is different.
(It violates timeing attack.)
be-hase: @charsyam 
At least, I want to check like a below. (very simple checking 😄 )
https://github.com/line/line-bot-sdk-python/blob/master/tests/test_webhook.py#L37-L46

LINE Signature is hashed by SHA-256 algorithm, so bytes length must be 32bytes.
Then we just need constant-time-compare.

(Example)
For the same reason, Django provide as below.
https://github.com/django/django/blob/master/django/utils/crypto.py#L80-L105


Patch:
diff --git a/linebot/utils.py b/linebot/utils.py
index 8dac56b..cc09699 100644
--- a/linebot/utils.py
+++ b/linebot/utils.py
@@ -45,3 +45,25 @@ def to_camel_case(text):
     """
     split = text.split('_')
     return split[0] + "".join(x.title() for x in split[1:])
+
+
+def safe_compare_digest(val1, val2):
+    """safe_compare_digest method.
+
+    :param val1: string or bytes for compare
+    :type val1: str | bytes
+    :param val2: string or bytes for compare
+    :type val2: str | bytes
+    """
+    if len(val1) != len(val2):
+        return False
+
+    result = 0
+    if PY3 and isinstance(val1, bytes) and isinstance(val2, bytes):
+        for i, j in zip(val1, val2):
+            result |= i ^ j
+    else:
+        for i, j in zip(val1, val2):
+            result |= (ord(i) ^ ord(j))
+
+    return result == 0
diff --git a/linebot/webhook.py b/linebot/webhook.py
index 51fb30b..0cbf746 100644
--- a/linebot/webhook.py
+++ b/linebot/webhook.py
@@ -32,7 +32,33 @@ from .models.events import (
     PostbackEvent,
     BeaconEvent
 )
-from .utils import LOGGER, PY3
+from .utils import LOGGER, PY3, safe_compare_digest
+
+
+if hasattr(hmac, "compare_digest"):
+    def compare_digest(val1, val2):
+        """compare_digest method.
+
+        :param val1: string or bytes for compare
+        :type val1: str | bytes
+        :param val2: string or bytes for compare
+        :type val2: str | bytes
+        :rtype: bool
+        :return: result
+        """
+        return hmac.compare_digest(val1, val2)
+else:
+    def compare_digest(val1, val2):
+        """compare_digest method.
+
+        :param val1: string or bytes for compare
+        :type val1: str | bytes
+        :param val2: string or bytes for compare
+        :type val2: str | bytes
+        :rtype: bool
+        :return: result
+        """
+        return safe_compare_digest(val1, val2)
 
 
 class SignatureValidator(object):
@@ -64,8 +90,8 @@ class SignatureValidator(object):
             hashlib.sha256
         ).digest()
 
-        return hmac.compare_digest(
-            signature.encode('utf-8'), base64.b64encode(gen_signature)
+        return compare_digest(
+                signature.encode('utf-8'), base64.b64encode(gen_signature)
         )
 
 


Test patch:
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 1ce5b96..0e0c534 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -16,7 +16,7 @@ from __future__ import unicode_literals, absolute_import
 
 import unittest
 
-from linebot.utils import to_camel_case, to_snake_case
+from linebot.utils import to_camel_case, to_snake_case, safe_compare_digest
 
 
 class TestUtils(unittest.TestCase):
@@ -26,6 +26,15 @@ class TestUtils(unittest.TestCase):
     def test_to_camel_case(self):
         self.assertEqual(to_camel_case('hoge_bar'), 'hogeBar')
 
+    def test_safe_compare_digest_true(self):
+        self.assertTrue(safe_compare_digest('/gg9a+LvFevTH1sd7', '/gg9a+LvFevTH1sd7'))
+
+    def test_safe_compare_digest_false_same_size(self):
+        self.assertFalse(safe_compare_digest('/gg9a+LvFevTH1sd7', '/gg9a+LvFevTH1sd8'))
+
+    def test_safe_compare_digest_false_different_size(self):
+        self.assertFalse(safe_compare_digest('/gg9a+LvFevTH1sd7', '/gg9a+LvFevTH1sd78'))
+
 
 if __name__ == '__main__':
     unittest.main()