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

SFTP + keypair mode doesn't work any more on stable10 #28669

Closed
PVince81 opened this issue Aug 14, 2017 · 15 comments
Closed

SFTP + keypair mode doesn't work any more on stable10 #28669

PVince81 opened this issue Aug 14, 2017 · 15 comments
Assignees
Labels
p2-high Escalation, on top of current planning, release blocker regression Type:Bug
Milestone

Comments

@PVince81
Copy link
Contributor

Steps

  1. Setup stable10
  2. Setup a system-wide SFTP mount with "RSA public key" mode
  3. Copy the given key into "~/ssh/.authorized_keys"
  4. Type in the user name and other stuff, wait for refresh

Expected

Green light

Actual

Red.

Version

stable10 1f0a3b0

Log is full of:

{"reqId":"nGG4Y8OWrYTttMWrtzN1","level":3,"time":"2017-08-14T12:28:32+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"PHP","method":"GET","url":"\/owncloudee\/ocs\/v2.php\/apps\/notifications\/api\/v1\/notifications?format=json","message":"pack(): Type H: illegal hex digit - at \/srv\/www\/htdocs\/owncloudee\/lib\/composer\/phpseclib\/phpseclib\/phpseclib\/Crypt\/RSA.php#1081"}
@PVince81 PVince81 added Type:Bug p2-high Escalation, on top of current planning, release blocker regression labels Aug 14, 2017
@PVince81 PVince81 added this to the triage milestone Aug 14, 2017
@PVince81
Copy link
Contributor Author

0  phpseclib\Crypt\RSA->_parseKey() /srv/www/htdocs/owncloud/lib/composer/phpseclib/phpseclib/phpseclib/Crypt/RSA.php:1081
1  phpseclib\Crypt\RSA->loadKey() /srv/www/htdocs/owncloud/lib/composer/phpseclib/phpseclib/phpseclib/Crypt/RSA.php:1565
2  OCA\Files_External\Lib\Auth\PublicKey\RSA->manipulateStorageConfig() /srv/www/htdocs/owncloud/apps/files_external/lib/Lib/Auth/PublicKey/RSA.php:63
3  OCA\Files_External\Controller\GlobalStoragesController->manipulateStorageConfig() /srv/www/htdocs/owncloud/apps/files_external/lib/Controller/StoragesController.php:228
4  OCA\Files_External\Controller\GlobalStoragesController->updateStorageStatus() /srv/www/htdocs/owncloud/apps/files_external/lib/Controller/StoragesController.php:245
5  OCA\Files_External\Controller\GlobalStoragesController->update() /srv/www/htdocs/owncloud/apps/files_external/lib/Controller/GlobalStoragesController.php:187
6  call_user_func_array:{/srv/www/htdocs/owncloud/lib/private/AppFramework/Http/Dispatcher.php:159}() /srv/www/htdocs/owncloud/lib/private/AppFramework/Http/Dispatcher.php:159
7  OC\AppFramework\Http\Dispatcher->executeController() /srv/www/htdocs/owncloud/lib/private/AppFramework/Http/Dispatcher.php:159
8  OC\AppFramework\Http\Dispatcher->dispatch() /srv/www/htdocs/owncloud/lib/private/AppFramework/Http/Dispatcher.php:89
9  OC\AppFramework\App::main() /srv/www/htdocs/owncloud/lib/private/AppFramework/App.php:98
10 OC\AppFramework\Routing\RouteActionHandler->__invoke() /srv/www/htdocs/owncloud/lib/private/AppFramework/Routing/RouteActionHandler.php:46
11 call_user_func:{/srv/www/htdocs/owncloud/lib/private/Route/Router.php:299}() /srv/www/htdocs/owncloud/lib/private/Route/Router.php:299
12 OC\Route\Router->match() /srv/www/htdocs/owncloud/lib/private/Route/Router.php:299
13 OC::handleRequest() /srv/www/htdocs/owncloud/lib/base.php:930
14 {main}          /srv/www/htdocs/owncloud/index.php:56

It looks like phpseclib has trouble parsing keys

@PVince81
Copy link
Contributor Author

Here is an example key that fails:

$matches                         = (array[3]);
  $matches[0]                    = (string[892]) 'DEK-Info: DES-EDE3-CBC,0920080F8133E3CD2epqrFq8zuQpKk0bHothHWaVbDbdLz03liANnx1Sqbb9Q6mhFPsovjJ9BPF921LwnTCMSCT4A97d5qYcloINwg0UBK5TJNs1mRLrRs/GkKb1JYuS8LEJRsn/diUbi7Az7FkuJ1W+0m7/WzWbSHYVAhI6ihySCOBrCKejoiqKd9EOnBO1U5RIim5Z73+3dhAs5QZlavqQzEfORxmyV0MK8rmYse99bkM8n+9qpLhcTQoymeZgeHb/ilN5xvdJlk3vWqkUqWDj6V7L+PJMAfAkHaIcd98N1t8ewThBOixnLGQhQmxdJwgtm6P2/eD4+R54hZi4mpRW9rpHXiu6H6vd2p2AnLV9oR7ZrPocKnKxFFVfIbWtYZdFi0pxPdKTinPkb4ha/ZSbgVIAPYIHSnpxbjk6A2ly8PMoZBAbfaBNVcvuzpO9wYa7HOSATH8qzZf26pKo0yqFf+V5wWP0gVNNqYpyMKNRcR2fpJn2APoSoq1pL1gxF3RrSRxG5yDzl2taRzxQ5taJkJ8X1ByUkk2kkW2MBjOUlvXgtFDz6Sk+7eLXh4oL1dXH3ek5HVe0Q5aDtSynAAiKVnbiYytYIll8yUJTo4XIpU3M9MfMwrfa1XLk5zVdSQmpnvNdekJRXOXkiLshE0N++68WodqDUjqR/M0YT9bYFj5ymoDIYlRFOKq8utxvAZ5kGs3c55bLPXpCJWiBx3ga3vIKfslBf+ljSYcz1KiYR37A7oC2QvgllappNXCEt9CK+xKG3NyKDPc/Svgk3BVQ5uzZDr4jxBdV34RKasckeBXuYgi4zktRnaEc5wcBRMwnJQ==-----END RSA PRIVATE KEY-----';
  $matches[1]                    = (string[12]) 'DES-EDE3-CBC';
  $matches[2]                    = (string[869]) '0920080F8133E3CD2epqrFq8zuQpKk0bHothHWaVbDbdLz03liANnx1Sqbb9Q6mhFPsovjJ9BPF921LwnTCMSCT4A97d5qYcloINwg0UBK5TJNs1mRLrRs/GkKb1JYuS8LEJRsn/diUbi7Az7FkuJ1W+0m7/WzWbSHYVAhI6ihySCOBrCKejoiqKd9EOnBO1U5RIim5Z73+3dhAs5QZlavqQzEfORxmyV0MK8rmYse99bkM8n+9qpLhcTQoymeZgeHb/ilN5xvdJlk3vWqkUqWDj6V7L+PJMAfAkHaIcd98N1t8ewThBOixnLGQhQmxdJwgtm6P2/eD4+R54hZi4mpRW9rpHXiu6H6vd2p2AnLV9oR7ZrPocKnKxFFVfIbWtYZdFi0pxPdKTinPkb4ha/ZSbgVIAPYIHSnpxbjk6A2ly8PMoZBAbfaBNVcvuzpO9wYa7HOSATH8qzZf26pKo0yqFf+V5wWP0gVNNqYpyMKNRcR2fpJn2APoSoq1pL1gxF3RrSRxG5yDzl2taRzxQ5taJkJ8X1ByUkk2kkW2MBjOUlvXgtFDz6Sk+7eLXh4oL1dXH3ek5HVe0Q5aDtSynAAiKVnbiYytYIll8yUJTo4XIpU3M9MfMwrfa1XLk5zVdSQmpnvNdekJRXOXkiLshE0N++68WodqDUjqR/M0YT9bYFj5ymoDIYlRFOKq8utxvAZ5kGs3c55bLPXpCJWiBx3ga3vIKfslBf+ljSYcz1KiYR37A7oC2QvgllappNXCEt9CK+xKG3NyKDPc/Svgk3BVQ5uzZDr4jxBdV34RKasckeBXuYgi4zktRnaEc5wcBRMwnJQ==-----END RSA PRIVATE KEY-----';

@PVince81
Copy link
Contributor Author

php7-7.1.6-1.1.x86_64

@PVince81
Copy link
Contributor Author

Also happening with php5-5.6.31-1.1.x86_64

So it's not a PHP version thing

@PVince81
Copy link
Contributor Author

Okay, I bisected this and the problem appeared with this commit 9b88fa6 which seems to strip "\n" and "\r" from the config options.

Not sure if this one is really needed, possibly a hardening. @Peter-Prochaska

I think the private and public keys are stored with newline in the DB, maybe we should convert it to a different format before storing, like base64 ? If we do, we'll also need to convert the old keys to the new format.

@PVince81 PVince81 modified the milestones: planned, triage Aug 18, 2017
@peterprochaska
Copy link
Contributor

peterprochaska commented Aug 21, 2017

Yes, this hardening is necessary. You can inject new headers for the server and create a new request.
So we have to find a new method to store the keys correct in the DB.

@PVince81
Copy link
Contributor Author

Ok, so here's the plan:

  • modify SFTP storage backend to store the private keys in base64 form (base64 the whole key string)
  • write migration to migrate keys that were stored with the old format to the new format

Not sure yet about the migration. Easiest would be to switch to a new setting name so we can detect whether the old one is here or new one.

@PVince81 PVince81 self-assigned this Sep 18, 2017
@Matan
Copy link

Matan commented Sep 20, 2017

I've just upgraded to 10.0.3 using owncloud/server:latest docker and now getting this issue.

{"reqId":"SC9N5EqXeZFut5mX18Ow","level":3,"time":"2017-09-20T08:32:47+00:00","remoteAddr":"172.20.0.1","user":"matan.uberstein","app":"PHP","method":"PUT","url":"\/apps\/files_external\/globalstorages\/1","message":"pack(): Type H: illegal hex digit - at \/var\/www\/owncloud\/lib\/composer\/phpseclib\/phpseclib\/phpseclib\/Crypt\/RSA.php#1081"}

@PVince81
Copy link
Contributor Author

If you need a quickfix you can revert this commit 9b88fa6. Note that it might introduce back a little security hole.

@Matan
Copy link

Matan commented Sep 20, 2017

I think doing that with the docker is going to quite tricky. When do you think the fix will be released?

@Matan
Copy link

Matan commented Sep 20, 2017

I commented out lines 224-226 on lib/private/Files/External/StorageConfig.php inside the running docker. It's working now 👍 It's not ideal, but would do for now. Thanks for the info! :)

@PVince81
Copy link
Contributor Author

@Matan aiming for 10.0.4 / next month for the clean fix.

@PVince81
Copy link
Contributor Author

fix is here: #29156

you can test it in 10.0.4beta1

@Matan
Copy link

Matan commented Dec 11, 2017

I've upgraded to 10.0.4 and I can confirm the issue is resolved. 🥇

@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-high Escalation, on top of current planning, release blocker regression Type:Bug
Projects
None yet
Development

No branches or pull requests

4 participants