Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't pass IV to pycrypto when using MODE_CTR #714

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@jmh045000
Copy link

jmh045000 commented Mar 31, 2016

jhall150
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 31, 2016

Coverage Status

Coverage increased (+0.04%) to 72.636% when pulling 4752287 on jmh045000:master into 74ba014 on paramiko:master.

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Apr 25, 2016

What's the impact of this on older versions of PyCrypto? (Paramiko 1.x supports PyCrypto 2.1 and up, basically.)

From a skim, it seems like "probably nothing?" because the commit linked in #713 states that the IV was already being ignored; but it'd be nice to make sure we're not risking errors for folks on PyCrypto 2.1-2.5 or so.

@bitprophet bitprophet added this to the 1.16.2 milestone Apr 25, 2016

@bitprophet bitprophet modified the milestones: 1.16.2 / 1.17.1 / 2.0.1, 1.16.3 et al Jun 21, 2016

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Dec 6, 2016

Double checked this change & the referenced PyCrypto commit and I'm still kinda uncomfortable just-merging this. Help me out, @jmh045000?

  • What's a concrete example of the bug/exception/etc this causes?
  • What PyCrypto version or versions does it impact?
  • What happens if you run Paramiko on an older PyCrypto with this change in place?
  • Is there a non crummy way to introspect whether to pass in iv vs ""? (Which would then mean the questions about PyCrypto version become moot.)

I'd dig more myself but given 2.0+ no longer use PyCrypto I can't afford the time :) if you get back to me, happy to keep it rolling! Thanks!

@bitprophet bitprophet modified the milestones: 1.17.3 et al, 1.17.4 etc Dec 9, 2016

@mimi1vx

This comment has been minimized.

@nvgoldin

This comment has been minimized.

Copy link

nvgoldin commented Jan 31, 2017

Hitting the same issue on fedora 24 - around a week ago python2-crypto RPM was upgraded in fedora24/updates to python2-crypto.x86_64 2.6.1-13.fc24. This essentially breaks paramiko on fedora24, as v2.x RPMs are not available there. The only option is to pin down an older version of python2-crypto, but as this is a security issue, its by far not the best solution.

https://bugzilla.redhat.com/show_bug.cgi?id=1418084

@jmh045000

This comment has been minimized.

Copy link
Author

jmh045000 commented Jan 31, 2017

I plan on resolving the concerns @bitprophet has this week. Then I'll resolve the merge conflicts and hopefully it will go in.

@nvgoldin

This comment has been minimized.

Copy link

nvgoldin commented Feb 5, 2017

Just updating the same python2-crypto version just hit EPEL, so it is going to affect all el7 installations. The older versions were removed, so you can't pin down the version either.
On fedora24 - looks like paramiko v2.x is going to be pushed to updates soon.

epel - https://bugzilla.redhat.com/show_bug.cgi?id=1419312
fc24 - https://bodhi.fedoraproject.org/updates/FEDORA-2017-6bf7d0c531

nvgoldin added a commit to nvgoldin/lago that referenced this pull request Feb 6, 2017

Make paramiko v1.16 work with python2-crypto again
1. Monkeypatch the paramiko.transport.Transport._get_cipher method
to work with python2-crypto.x86_64 0:2.6.1-13.el7. Based on:
paramiko/paramiko#714

2. Explicitly require paramiko >= v2.1.1 for fc >= 24, which does not
depends on python2-crypto.

https://bugzilla.redhat.com/show_bug.cgi?id=1419312

Signed-off-by: Nadav Goldin <ngoldin@redhat.com>

nvgoldin added a commit to nvgoldin/lago that referenced this pull request Feb 6, 2017

Make paramiko v1.16 work with python2-crypto again
1. Monkeypatch the paramiko.transport.Transport._get_cipher method
to work with python2-crypto.x86_64 0:2.6.1-13.el7. Based on:
paramiko/paramiko#714

2. Explicitly require paramiko >= v2.1.1 for fc >= 24, which does not
depends on python2-crypto.

https://bugzilla.redhat.com/show_bug.cgi?id=1419312

Signed-off-by: Nadav Goldin <ngoldin@redhat.com>

nvgoldin added a commit to nvgoldin/lago that referenced this pull request Feb 7, 2017

Make paramiko v1.16 work with python2-crypto again
Monkeypatch the paramiko.transport.Transport._get_cipher method
to work with python2-crypto.x86_64 0:2.6.1-13.el7. Based on:
paramiko/paramiko#714

https://bugzilla.redhat.com/show_bug.cgi?id=1419312

Signed-off-by: Nadav Goldin <ngoldin@redhat.com>
@jmh045000

This comment has been minimized.

Copy link
Author

jmh045000 commented Feb 8, 2017

My testing script:

#!/bin/bash

rm -rf test
mkdir test
cd test

cat > paramiko_test.py << EOF
#!/usr/bin/env python2
import paramiko
import sys
try:
    ssh = paramiko.SSHClient()
    ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
    ssh.connect('<redacted>', username='pycrypto-test', password='test')
    _, stdout, _ = ssh.exec_command('uptime')
    print '\n'.join(stdout.readlines()).strip()
    sys.exit(0)
except Exception as e:
    print e
    sys.exit(1)
EOF
chmod +x paramiko_test.py

test_exec=$(readlink -m paramiko_test.py)

git clone https://github.com/dlitz/pycrypto.git &>/dev/null
git clone https://github.com/jmh045000/paramiko.git &>/dev/null
(cd paramiko ; git remote add upstream https://github.com/paramiko/paramiko.git ; git fetch upstream) &>/dev/null

pycrypto_dir=$(readlink -m pycrypto)
paramiko_dir=$(readlink -m paramiko)

for pycrypto_version in v2.1.0 v2.2 v2.3 v2.4.1 v2.5 v2.6 v2.6.1 master ; do
    for paramiko_version in v1.17.3 v1.18.1 ; do
        working_dir=pycrypto_${pycrypto_version}-paramiko_${paramiko_version}
        virtualenv $working_dir &>/dev/null || 1
        source ${working_dir}/bin/activate || 1

        ( cd $pycrypto_dir ; git checkout $pycrypto_version ) &>/dev/null
        ( cd $paramiko_dir ; git checkout $paramiko_version ) &>/dev/null
        pip install 'ecdsa<2.0,>=0.11' &> /dev/null || exit 1
        pip install ${pycrypto_dir} &>/dev/null || exit 1
        pip install -e ${paramiko_dir} &>/dev/null || exit 1

        echo "Testing with pycrypto==${pycrypto_version}, paramiko==${paramiko_version}"
        echo -n "Without patch: "
        ${test_exec}

        ( cd $paramiko_dir ; git cherry-pick 4752287a7379da61245087ee7e35635a4e42bb3f ) &>/dev/null
        echo -n "With patch   : "
        ${test_exec}
    done
done

Output:

Testing with pycrypto==v2.1.0, paramiko==v1.17.3
Without patch: 18:13:11 up  2:24,  0 users,  load average: 0.03, 0.05, 0.05
With patch   : 18:13:11 up  2:24,  0 users,  load average: 0.03, 0.05, 0.05
Testing with pycrypto==v2.1.0, paramiko==v1.18.1
Without patch: 18:13:20 up  2:24,  0 users,  load average: 0.11, 0.06, 0.05
With patch   : 18:13:20 up  2:24,  0 users,  load average: 0.11, 0.06, 0.05
Testing with pycrypto==v2.2, paramiko==v1.17.3
Without patch: 18:13:29 up  2:24,  0 users,  load average: 0.10, 0.06, 0.05
With patch   : 18:13:30 up  2:24,  0 users,  load average: 0.09, 0.06, 0.05
Testing with pycrypto==v2.2, paramiko==v1.18.1
Without patch: 18:13:38 up  2:25,  0 users,  load average: 0.08, 0.06, 0.05
With patch   : 18:13:39 up  2:25,  0 users,  load average: 0.08, 0.06, 0.05
Testing with pycrypto==v2.3, paramiko==v1.17.3
Without patch: 18:13:47 up  2:25,  0 users,  load average: 0.07, 0.06, 0.05
With patch   : 18:13:48 up  2:25,  0 users,  load average: 0.07, 0.06, 0.05
Testing with pycrypto==v2.3, paramiko==v1.18.1
Without patch: 18:13:57 up  2:25,  0 users,  load average: 0.06, 0.06, 0.05
With patch   : 18:13:58 up  2:25,  0 users,  load average: 0.06, 0.06, 0.05
Testing with pycrypto==v2.4.1, paramiko==v1.17.3
Without patch: 18:14:08 up  2:25,  0 users,  load average: 0.05, 0.05, 0.05
With patch   : 18:14:09 up  2:25,  0 users,  load average: 0.05, 0.05, 0.05
Testing with pycrypto==v2.4.1, paramiko==v1.18.1
Without patch: 18:14:20 up  2:25,  0 users,  load average: 0.04, 0.05, 0.05
With patch   : 18:14:21 up  2:25,  0 users,  load average: 0.04, 0.05, 0.05
Testing with pycrypto==v2.5, paramiko==v1.17.3
Without patch: 18:14:33 up  2:26,  0 users,  load average: 0.03, 0.05, 0.05
With patch   : 18:14:33 up  2:26,  0 users,  load average: 0.03, 0.05, 0.05
Testing with pycrypto==v2.5, paramiko==v1.18.1
Without patch: 18:14:45 up  2:26,  0 users,  load average: 0.03, 0.05, 0.05
With patch   : 18:14:46 up  2:26,  0 users,  load average: 0.03, 0.05, 0.05
Testing with pycrypto==v2.6, paramiko==v1.17.3
Without patch: 18:14:57 up  2:26,  0 users,  load average: 0.02, 0.04, 0.05
With patch   : 18:14:58 up  2:26,  0 users,  load average: 0.02, 0.04, 0.05
Testing with pycrypto==v2.6, paramiko==v1.18.1
Without patch: 18:15:09 up  2:26,  0 users,  load average: 0.02, 0.04, 0.05
With patch   : 18:15:10 up  2:26,  0 users,  load average: 0.02, 0.04, 0.05
Testing with pycrypto==v2.6.1, paramiko==v1.17.3
Without patch: 18:15:21 up  2:26,  0 users,  load average: 0.01, 0.04, 0.05
With patch   : 18:15:22 up  2:26,  0 users,  load average: 0.01, 0.04, 0.05
Testing with pycrypto==v2.6.1, paramiko==v1.18.1
Without patch: 18:15:33 up  2:27,  0 users,  load average: 0.01, 0.04, 0.05
With patch   : 18:15:34 up  2:27,  0 users,  load average: 0.01, 0.04, 0.05
Testing with pycrypto==master, paramiko==v1.17.3
Without patch: No handlers could be found for logger "paramiko.transport"
CTR mode needs counter parameter, not IV
With patch   : 18:15:46 up  2:27,  0 users,  load average: 0.01, 0.04, 0.05
Testing with pycrypto==master, paramiko==v1.18.1
Without patch: No handlers could be found for logger "paramiko.transport"
CTR mode needs counter parameter, not IV
With patch   : 18:16:06 up  2:27,  0 users,  load average: 0.01, 0.04, 0.05
@jmh045000

This comment has been minimized.

Copy link
Author

jmh045000 commented Feb 8, 2017

I now have two branches, one based on 1.17 and one based on 1.18. Do you want me to submit two PRs, one for each of those release series?

@ploxiln

This comment has been minimized.

Copy link
Contributor

ploxiln commented Feb 8, 2017

My guess is that he'll want a PR against 1.17, and he'll merge it up into 1.18 himself. You can actually change the "base branch" of this PR (from paramiko:master to paramiko:1.17) if you click the "Edit" button next to the title.

@ploxiln

This comment has been minimized.

Copy link
Contributor

ploxiln commented Feb 8, 2017

Well, nevermind, you'll need to create a new pull request anyway, because this one is from your master branch (rather than e.g. a topic branch), so it would be rather awkward to force-push a 1.17-based branch to your master branch.

Sorry for the noise ...

@jmh045000

This comment has been minimized.

Copy link
Author

jmh045000 commented Feb 8, 2017

Yea, not sure why I made it from the master branch... lazy I guess? I'll open one to 1.17 and reference this PR, and let the owner decide what to do ;)

@khenderick

This comment has been minimized.

Copy link

khenderick commented Feb 17, 2017

A new python-crypto also hit the Ubuntu repos: https://bugs.launchpad.net/ubuntu/+source/paramiko/+bug/1665565

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 19, 2017

I'm digging out my ticket backlog today, and just made myself a note to handle this & other high priority, low hanging fruit tickets tomorrow (Mondays are my official OSS days now thanks to my employer). Thanks to everyone tracking this & updating it, and apologies for the tardiness (been heads down on a private alpha of a related project I'm trying to make public ASAP).

bitprophet added a commit that referenced this pull request Feb 20, 2017

bitprophet added a commit that referenced this pull request Feb 20, 2017

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 20, 2017

Yea, the other PR against the 1.x lines is the right one to merge; this code is no longer extant in master as it reflects a post-2.0 world. Closing.

EDIT: since this is the one with all the discussion: noting explicitly that #889 has been merged into the 1.17 and 1.18 lines and will get a release today as 1.17.4 and 1.18.2.

@bitprophet bitprophet closed this Feb 20, 2017

@bitprophet bitprophet modified the milestone: 1.18.3 etc Feb 21, 2017

@guillaume-uH57J9

This comment has been minimized.

Copy link

guillaume-uH57J9 commented May 13, 2017

Is there any known workaround for this?

Thanks for the fix, unfortunately the updated packages are not available in my distribution yet
https://packages.debian.org/jessie/python-paramiko
https://packages.debian.org/stretch/python-paramiko

@mimi1vx

This comment has been minimized.

Copy link

mimi1vx commented May 13, 2017

@guillaume-uH57J9

This comment has been minimized.

Copy link

guillaume-uH57J9 commented May 13, 2017

Fair enough, I applied the fix to my install and it avoids the error message.
The connection still fails, will try to investigate as to why.

Update: it seems I am also affected by #883 because my keys are ECDSA. It finally connected when switching RSA 4096bit keys.

@blackduckx

This comment has been minimized.

Copy link

blackduckx commented Jun 7, 2017

I opened a case @redhat to get the fix included in RHEL6. I let you know when I have an update.

I hit this bug while upgrading salt-minion, that depends on python-crypto 2.6.1.

@ExaneServerTeam

This comment has been minimized.

Copy link

ExaneServerTeam commented Jul 5, 2017

@mplx mplx referenced this pull request Sep 6, 2017

Merged

ssh v2 backend support #8

rdkls added a commit to rdkls/yang-explorer that referenced this pull request Aug 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.