Fix security issue in version extra calculation (fix #328) #330

lrq3000 · 2016-12-31T16:16:21Z

and add branch name in __version__ if available.

This should fix the security issue reported in #328 (thanks to @jwilk!) alias CVE-2016-10075.

TODO (but can be done later, we should merge ASAP a fix for this issue):

restore 100% coverage (need to virtualize .git folder and create 3 scenarios: no tqdm in config, head detached so commit hash directly in HEAD file, head in a branch so commit hash is in a ref file). Maybe use something like with unittest.mock.patch.dict(os.environ, PATH='/nonexistent') (courtesy of @jwilk)?

CrazyPython · 2016-12-31T16:41:27Z

tqdm/_version.py

+        # Sanitize path of ref file
+        # get full path to ref file
+        ref_file_path = os.path.abspath(os.path.join(gitdir, ref_file))
+        # check that we are in git folder (by stripping the git folder from the ref file path)


inconsistent capitalization of comments (PEP 8 recommends lowercase)

It's because the capitalized one pertains to the whole code block, whereas the lowercase pertains to only the next line (because there's not enough space to put it at the right of the code). I don't know how to make it more explicit or prettier...

CrazyPython · 2016-12-31T16:41:35Z

tqdm/_version.py

-            __version__ += '-' + cur_hash
+res = None
+scriptdir = os.path.dirname(__file__)
+gitdir = os.path.abspath(scriptdir+'/../.git')


no spaces around operator

CrazyPython · 2016-12-31T16:44:20Z

tqdm/_version.py

+    else:
+        res = res[:8]
+    # Append to version string
+    if res:


is not None can be more explicit

lrq3000 · 2016-12-31T17:04:36Z

Thanks @CrazyPython, I fixed what you spotted :)

jwilk · 2016-12-31T17:15:36Z

(Disclaimer: I haven't tested this; I only had a cursory look at the code.)
I think this is fine from security point of view.
However, if the user kept their local site-packages in a git repo, and installed tqdm there, you would be inspecting this unrelated repository, not the tqdm one. Oh well...
IMHO the desire of having super-exact __version__ doesn't justify complexity of this code. I'd just remove all the git-related stuff from _version.py. But it's not my call...
Typo: file file.

CrazyPython

An opinionated style review. This is also dearly in need of newlines, esp. between 15&16, 29&30, and 39&40.

CrazyPython · 2016-12-31T17:12:17Z

tqdm/_version.py

@@ -15,18 +11,35 @@


 # auto -extra based on commit hash (if not tagged as release)


PEP 8 dictates one less newline here

CrazyPython · 2016-12-31T17:12:48Z

tqdm/_version.py

+gitdir = os.path.abspath(scriptdir + '/../.git')
+if os.path.isdir(gitdir):
+    # Open the HEAD file
+    with open(os.path.join(gitdir,'HEAD'), 'r') as fh_head:


Missing space after comma

CrazyPython · 2016-12-31T17:13:15Z

tqdm/tests/tests_version.py

@@ -4,7 +4,8 @@
 def test_version():
    """ Test version string """
    from tqdm import __version__
-    Mmpe = re.split('[.-]', __version__)


Not a very good variable name - CamelCase and undescriptive

CrazyPython · 2016-12-31T17:14:02Z

tqdm/_version.py

-
-        if (last_release is None) or (cur_hash not in last_release):
-            __version__ += '-' + cur_hash
+res = None


res is defined here...

CrazyPython · 2016-12-31T17:15:21Z

tqdm/_version.py

+gitdir = os.path.abspath(scriptdir + '/../.git')
+if os.path.isdir(gitdir):
+    # Open the HEAD file
+    with open(os.path.join(gitdir,'HEAD'), 'r') as fh_head:


...but only used in this if statement. Either move it in (lazy evaluation) or move the checking operation out (imperative).

CrazyPython · 2016-12-31T17:16:33Z

tqdm/_version.py

+gitdir = os.path.abspath(scriptdir + '/../.git')
+if os.path.isdir(gitdir):
+    # Open the HEAD file
+    with open(os.path.join(gitdir,'HEAD'), 'r') as fh_head:


'r' is not required, you decide if that's a good idea

yeah but better be explicit :)

CrazyPython · 2016-12-31T17:17:23Z

tqdm/_version.py

-
-        if (last_release is None) or (cur_hash not in last_release):
-            __version__ += '-' + cur_hash
+res = None


You might want to rename res to result. It's more clear.

Renamed to extra.

CrazyPython · 2016-12-31T17:18:56Z

tqdm/_version.py

+    with open(os.path.join(gitdir,'HEAD'), 'r') as fh_head:
+        res = fh_head.readline().strip()
+    # If we are in a branch, HEAD will point to a file containing the latest commit
+    if 'ref:' in res:


In both of these branches, res is set. Yet above, res is set to another name. Two variables would be clearer.

I don't understand your remark, res always aim to contain the extra = the commit hash (and if available the branch name). I changed the variable name to be more explicit. It's just that if we are in a branch, we don't get the commit hash from the HEAD file, so then it's necessary to do some more processing.

CrazyPython · 2016-12-31T17:20:45Z

tqdm/tests/tests_version.py

@@ -4,7 +4,8 @@
 def test_version():
    """ Test version string """
    from tqdm import __version__
-    Mmpe = re.split('[.-]', __version__)
+    Mmpe = __version__.split()[0]  # remove part after space (branch name)
+    Mmpe = re.split('[.-]', Mmpe)  # split by dot and dash
    assert 3 <= len(Mmpe) <= 4


Is this not the same as assert len(Mmpe) in (3, 4)? People usually expect reversed syntax, e.g. 4 >= len(Mmpe) >= 3.

Yes this is the same but I disagree the reversed syntax is more common, since they are mathematical operators I expect the same convention as in mathematical notation (least value first).

lrq3000 · 2016-12-31T23:21:43Z

Thanks both for your reviews and insights. I agree, we should think about keeping this code. Note to self: - [x] check git repo name (tqdm) - [x] fix CrazyPython remarks.

jwilk · 2017-01-01T14:59:44Z

<= is waaay more common than >= in chained comparisons: https://codesearch.debian.net/search?q=%3C%3D%5Cs%2B%5Cw%2B%5Cs%2B%3C%3D+filetype%3Apython (5363 hits) https://codesearch.debian.net/search?q=%3E%3D%5Cs%2B%5Cw%2B%5Cs%2B%3E%3D+filetype%3Apython (78 hits)

lrq3000 · 2017-01-01T19:57:22Z

Wow regex in debian repo search, I did not know about that, amazing! 2017-01-01 15:59 GMT+01:00 Jakub Wilk <notifications@github.com>:

…

<= is waaay more common than >= in chained comparisons: https://codesearch.debian.net/search?q=%3C%3D%5Cs%2B%5Cw%2B% 5Cs%2B%3C%3D+filetype%3Apython (5363 hits) https://codesearch.debian.net/search?q=%3E%3D%5Cs%2B%5Cw%2B% 5Cs%2B%3E%3D+filetype%3Apython (78 hits) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#330 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABES3iIvpUk7CdK-itAUMx-U2J1MPPtoks5rN79hgaJpZM4LYlJv> .

CrazyPython · 2017-01-01T20:16:00Z

If I have time later, I might review some of the existing tqdm codebase, if you want me to.

lrq3000 · 2017-01-01T22:53:46Z

@CrazyPython yes please feel free to do so, your feedbacks are appreciated :) But it might take you a while! 2017-01-01 21:16 GMT+01:00 CrazyPython <notifications@github.com>:

…

If I have time later, I might review some of the existing tqdm codebase, if you want me to. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#330 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABES3rSO1CJgUJxnQWv3g03Fe2972a_Jks5rOAmBgaJpZM4LYlJv> .

CrazyPython · 2017-01-04T00:56:15Z

tqdm/tests/tests_version.py

    try:
-        map(int, Mmpe[:3])
+        map(int, version_parts[:3])
    except:


You want except ValueError:. This catches "errors" like SystemExit and other python internals.

sandrotosi · 2017-01-06T20:23:57Z

tqdm/_version.py

+    extra = None
+    # Open config file to check if we are in tqdm project
+    with open(os.path.join(gitdir, 'config'), 'r') as fh_config:
+        if 'tqdm' in fh_config.read():


here you're just checking if 'tqdm' is anywhere in .git/config which might not be strict enough

@sandrotosi Yep I thought the same but after reading a bit about git internals, I didn't find a better way to check if we were looking at tqdm repository. Any suggestion? Maybe looking specifically for a specific entry in config file?

i looks like you have to go thru several steps: read .git/HEAD, lookup the remote for that ref in .git/config, check the remote url in .git/config if that ends with '/tqdm.git' (as you have to consider forks and other possible checkout methods).

i still think there must be an easier way to do it, and i'd look at out other "big" projects (numpy, scipy, matplotlib, and the like) do as they probably want to tackle the same problem as you do

fix #328, Clean imports, Fix style

casperdcl · 2017-01-22T21:52:29Z

I agree that it's a bit much to go overboard with this, it was meant to be a small helper. I'm tempted to ignore coverage of the "extra" version bit.

CrazyPython mentioned this pull request Dec 31, 2016

tqdm._version: insecure use of git #328

Closed

CrazyPython reviewed Dec 31, 2016

View reviewed changes

lrq3000 added p0-bug-critical ☢ Exception rasing to-merge ↰ Imminent labels Jan 1, 2017

lrq3000 assigned casperdcl Jan 1, 2017

lrq3000 added this to the v5.0.0 milestone Jan 1, 2017

CrazyPython reviewed Jan 4, 2017

View reviewed changes

sandrotosi reviewed Jan 6, 2017

View reviewed changes

lrq3000 and others added 3 commits January 22, 2017 14:20

__version__ extra calculation: security fix, add branch name

7996430

fix #328, Clean imports, Fix style

check if in tqdm project before extra version calculation + style fixes

064aa65

minor safety, comments tidy, flake8 fixes, show coverage

4857462

casperdcl force-pushed the version-security-fix branch from 5702ac0 to 4857462 Compare January 22, 2017 14:25

coverage fixes

30a39b2

no extra on manual tag

aec3f7d

casperdcl merged commit aec3f7d into master Jan 23, 2017

casperdcl deleted the version-security-fix branch January 23, 2017 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix security issue in version extra calculation (fix #328) #330

Fix security issue in version extra calculation (fix #328) #330

lrq3000 commented Dec 31, 2016 •

edited

Loading

CrazyPython Dec 31, 2016

lrq3000 Dec 31, 2016

CrazyPython Dec 31, 2016

CrazyPython Dec 31, 2016

lrq3000 commented Dec 31, 2016

jwilk commented Dec 31, 2016

CrazyPython left a comment •

edited

Loading

CrazyPython Dec 31, 2016

CrazyPython Dec 31, 2016

CrazyPython Dec 31, 2016

CrazyPython Dec 31, 2016

CrazyPython Dec 31, 2016

CrazyPython Dec 31, 2016 •

edited

Loading

lrq3000 Jan 1, 2017

CrazyPython Dec 31, 2016

lrq3000 Jan 1, 2017

CrazyPython Dec 31, 2016

lrq3000 Jan 1, 2017

CrazyPython Dec 31, 2016

lrq3000 Jan 1, 2017

lrq3000 commented Dec 31, 2016 via email •

edited

Loading

jwilk commented Jan 1, 2017 via email

lrq3000 commented Jan 1, 2017 via email

CrazyPython commented Jan 1, 2017

lrq3000 commented Jan 1, 2017 via email

CrazyPython Jan 4, 2017

sandrotosi Jan 6, 2017

lrq3000 Jan 6, 2017

sandrotosi Jan 6, 2017

casperdcl commented Jan 22, 2017

		@@ -15,18 +11,35 @@


		# auto -extra based on commit hash (if not tagged as release)

Fix security issue in __version__ extra calculation (fix #328) #330

Fix security issue in __version__ extra calculation (fix #328) #330

Conversation

lrq3000 commented Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrq3000 commented Dec 31, 2016

jwilk commented Dec 31, 2016

CrazyPython left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrazyPython Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrq3000 commented Dec 31, 2016 via email • edited Loading

jwilk commented Jan 1, 2017 via email

lrq3000 commented Jan 1, 2017 via email

CrazyPython commented Jan 1, 2017

lrq3000 commented Jan 1, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl commented Jan 22, 2017

Fix security issue in version extra calculation (fix #328) #330

Fix security issue in version extra calculation (fix #328) #330

lrq3000 commented Dec 31, 2016 •

edited

Loading

CrazyPython left a comment •

edited

Loading

CrazyPython Dec 31, 2016 •

edited

Loading

lrq3000 commented Dec 31, 2016 via email •

edited

Loading