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

Some bugixes to run on for Mac OS X #28

Closed
wants to merge 1 commit into from

Conversation

wagnert
Copy link

@wagnert wagnert commented Apr 18, 2015

Hi, i tried to run the server on Mac OS X Yosemite and found some problems. After some fixes, it finally work. Very interesting project, congratulations!!!! We think about using it in our application server after some testing :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 10.36% when pulling c1df434 on wagnert:master into 9258b23 on fpoirotte:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 10.36% when pulling c1df434 on wagnert:master into 9258b23 on fpoirotte:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 10.36% when pulling c1df434 on wagnert:master into 9258b23 on fpoirotte:master.

@@ -69,9 +69,11 @@ protected function createResponse(
$kexAlgo = new $kexAlgo();
$message = \fpoirotte\Pssht\Messages\KEXDH\INIT::unserialize($decoder);

if (!$message->getQ()->isValid()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you have to change that line?

@fpoirotte
Copy link
Owner

Thanks for your words and your patch.

While the second change is obviously required (I missed a variable rename while refactoring the code), I don't understand your other change (in src/Handlers/KEXDH/INIT.php). Could you please explain why it was necessary?

@wagnert
Copy link
Author

wagnert commented May 1, 2015

Hi, when i uncomment it, i can't connect with ssh -p 22222 clicky@127.0.0.1, because i got a Call to undefined method GMP::isValid() in the console. Seems that $message->getE() doesn't return the expected instance, but a instance of GMP that doesn't has a isValid() method. I'm working on Mac OS X 10.10 by the way :)

fpoirotte added a commit that referenced this pull request May 1, 2015
I forgot to rename that variable while refactoring some code, leading to
errors about undefined variables being used.

See also #28.
@fpoirotte fpoirotte closed this in 2e4d6cd May 1, 2015
@fpoirotte
Copy link
Owner

OK, I temporarily disabled the check but plan to change the code later to include a proper fix.
As you noted, this will fix Diffie-Hellman key exchanges.

This however leaves some users vulnerable (those who use Elliptic Curve Diffie-Hellman, aka. ECDH) as invalid public keys could be used. Given that this project is mainly a toy and not intended for production use, it's a risk I'm willing to take for now.

@wagnert
Copy link
Author

wagnert commented May 1, 2015

I agree, this should only be a temporary fix, especially as it opens some vulnerabilities!

We really hope, that this project will switch to a stable and secure version as soon as possible, because it would perfectly fit to provide a secure shell for our application server. So, on the one hand, if we can provide you some help, feel free to give us a hint :) On the other hand, it'll be helpful if there'll be something like a roadmap with tasks that have to be solved!

And again, projects like this are great enrichment for the PHP ecosystem 👍

@wagnert
Copy link
Author

wagnert commented May 1, 2015

And before i forget: It'll be really helpful if you could tag this version :)

fpoirotte added a commit that referenced this pull request May 8, 2015
v0.1.1
~~~~~~

*   [#28] Temporarily fix Diffie–Hellman key exchange by disabling
    public key validation for Elliptic Curve Diffie–Hellman.
    This code will be revisited later on as it currently represents
    a possible security threat when ECDH is used.

*   Improve README (installation instruction, changelog).

*   Change the default ``pssht.xml`` so that it accepts connections
    from the same user as the one starting the server
    (prior to this change, it used an hardcoded username).
@fpoirotte
Copy link
Owner

I just released version 0.1.1 which includes this fix as well as various other improvements. Hope this makes it easier for you.

The roadmap for the project is defined as a series of milestones (see https://github.com/fpoirotte/pssht/milestones), but as you can see, the current milestone is way overdue.

@wagnert
Copy link
Author

wagnert commented May 8, 2015

Hi François,

thanks, thats awesome! I'll checkout the new version and the roadmap tomorrow :)

Cheers

Tim Wagner
Head of Development / Design
MAGENTO CERTIFIED DEVELOPER PLUS
CERTIFIED SCRUM MASTER

Telefon +49-8031-221055-0
Telefax +49-8031-221055-22
t.wagner@techdivision.com

TechDivision GmbH
Spinnereiinsel 3a
83059 Kolbermoor

MAGENTO GOLD PARTNER
TYPO3 GOLD MEMBER

http://www.techdivision.com

Am 08.05.2015 um 22:03 schrieb François Poirotte notifications@github.com:

I just released version 0.1.1 which includes this fix as well as various other improvements. Hope this makes it easier for you.

The roadmap for the project is defined as a series of milestones (see https://github.com/fpoirotte/pssht/milestones), but as you can see, the current milestone is way overdue.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants