hmac.compare_digest is not in earlier version python2.7.7 #13

charsyam · 2016-10-18T16:41:32Z

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

be-hase · 2016-10-19T03:13:32Z

Hi, @charsyam
Thanks for your feedback. 😄
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 · 2016-10-19T03:39:04Z

maybe @charsyam can get hint here https://github.com/python/cpython/blob/2.7/Modules/operator.c#L289

charsyam · 2016-10-19T04:45:07Z

Hi, @be-hase

could you review this? I implemented safe_compare_digest func to refer tscmp

be-hase · 2016-10-19T05:38:44Z

Oh... thanks for your patch. 👍

be-hase · 2016-10-19T05:38:55Z

linebot/webhook.py

-        return hmac.compare_digest(
-            signature.encode('utf-8'), base64.b64encode(gen_signature)
-        )
+        if hexversion >= 0x020707F0:


Would you like to use if hasattr(hmac, "compare_digest"): ?
I think that it is readable.

charsyam · 2016-10-19T05:52:25Z

linebot/webhook.py

+    return ret == 0
+
+
+compare_digest = safe_compare_digest


@be-hase how about this approach?
I applied hasattr and made function reference as compare_digest.

okdtsk · 2016-10-19T05:57:25Z

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 · 2016-10-19T06:07:45Z

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 · 2016-10-19T06:26:56Z

@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?

darjeeling · 2016-10-19T06:41:40Z

linebot/webhook.py

+    if len1 == len2:
+        left = str1
+        ret = 0
+


why not else ?

In your hint. There is an answer. so I avoid to use else :)
/* don't use else here to keep the amount of CPU instructions constant,
* volatile forces re-evaluation */

be-hase · 2016-10-19T06:56:39Z

I'm sorry reply late.

docstring

Yes, we use reST format. Sorry @okdtsk 😢

This feature is sensitive, so I want a test code.

okdtsk · 2016-10-19T06:58:09Z

@charsyam @be-hase
Ah sorry I checked previous code. Please ignore my comment about docstring. 😭

be-hase · 2016-10-19T07:21:52Z

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 · 2016-10-19T07:31:28Z

@be-hase Yes, I also worry about that. How we can test it well?
because I think this verify consuming time is equals.

charsyam · 2016-10-19T07:32:08Z

@be-hase and We can't just return False when length is different.
(It violates timeing attack.)

be-hase · 2016-10-19T08:07:45Z

@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

be-hase · 2016-10-19T09:29:47Z

linebot/utils.py

+def safe_compare_digest(val1, val2):
+    """safe_compare_digest method.
+
+    :param str or bytes val1: string or bytes for compare


OK, looks good to me.

Sorry..., How about this ?

:param val1: string or bytes for compare :type val1: str | bytes

ref:
https://github.com/line/line-bot-sdk-python/blob/master/linebot/api.py#L79-L83

And Have you signed up ICLA?
( https://feedback.line.me/enquete/public/919-h9Yqmr1u )
If you have completed, I will merge and release 1.0.2

Thanks 😄

fix indent implement tscmp for preventing timing attack remove semicolon apply flake8 apply flake8 refactoring using hasattr refactoring using len2 directly add test and refactoring add test and refactoring change docstring

charsyam · 2016-10-19T09:38:46Z

@be-hase I singed ICLA and update your review for changing docstring Thanks

charsyam force-pushed the bugfix/hmac-compare-digest-for-under-python277 branch from 85fe814 to 276e67c Compare October 18, 2016 16:47

charsyam force-pushed the bugfix/hmac-compare-digest-for-under-python277 branch 4 times, most recently from f44920c to 305adf6 Compare October 19, 2016 04:42

be-hase reviewed Oct 19, 2016

View reviewed changes

charsyam force-pushed the bugfix/hmac-compare-digest-for-under-python277 branch from 305adf6 to f590147 Compare October 19, 2016 05:48

charsyam commented Oct 19, 2016

View reviewed changes

charsyam force-pushed the bugfix/hmac-compare-digest-for-under-python277 branch from f590147 to 9450c09 Compare October 19, 2016 06:27

darjeeling reviewed Oct 19, 2016

View reviewed changes

charsyam force-pushed the bugfix/hmac-compare-digest-for-under-python277 branch 2 times, most recently from af51cb1 to 9827e6d Compare October 19, 2016 08:57

be-hase reviewed Oct 19, 2016

View reviewed changes

fix hmac.compare_digest under python277

29759f7

fix indent implement tscmp for preventing timing attack remove semicolon apply flake8 apply flake8 refactoring using hasattr refactoring using len2 directly add test and refactoring add test and refactoring change docstring

charsyam force-pushed the bugfix/hmac-compare-digest-for-under-python277 branch from 9827e6d to 29759f7 Compare October 19, 2016 09:34

be-hase merged commit b763069 into line:master Oct 19, 2016

be-hase mentioned this pull request Oct 19, 2016

Release 1.0.2 #14

Merged

hmac.compare_digest is not in earlier version python2.7.7 #13

hmac.compare_digest is not in earlier version python2.7.7 #13

Uh oh!

Conversation

charsyam commented Oct 18, 2016

Uh oh!

be-hase commented Oct 19, 2016

Uh oh!

darjeeling commented Oct 19, 2016

Uh oh!

charsyam commented Oct 19, 2016

Uh oh!

be-hase commented Oct 19, 2016

Uh oh!

be-hase Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

charsyam Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

okdtsk commented Oct 19, 2016

Uh oh!

okdtsk commented Oct 19, 2016

Uh oh!

charsyam commented Oct 19, 2016

Uh oh!

darjeeling Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

charsyam Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

be-hase commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

okdtsk commented Oct 19, 2016

Uh oh!

be-hase commented Oct 19, 2016

Uh oh!

charsyam commented Oct 19, 2016

Uh oh!

charsyam commented Oct 19, 2016

Uh oh!

be-hase commented Oct 19, 2016

Uh oh!

be-hase Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

charsyam commented Oct 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

be-hase commented Oct 19, 2016 •

edited

Loading