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

Missing signature with encryption on GDrive #22590

Closed
PVince81 opened this issue Feb 23, 2016 · 15 comments · Fixed by #22604
Closed

Missing signature with encryption on GDrive #22590

PVince81 opened this issue Feb 23, 2016 · 15 comments · Fixed by #22604

Comments

@PVince81
Copy link
Contributor

Steps

  1. Enable encryption
  2. Mount GDrive as "/gdrive"
  3. Create a subdir "/gdrive/enc"
  4. Upload a picture (1-2mb) with the web UI into "/gdrive/enc/test.jpg"
  5. Try downloading it

Expected

Download works

Actual

"Missing signature" error.

{"reqId":"aAbpqvIJEnh+9iiNvOXe","remoteAddr":"127.0.0.1","app":"webdav","message":"Exception: {\"Message\":\"Missing Signature\",\"Exception\":\"OC\\\\HintException\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/encryption\\\/lib\\\/crypto\\\/crypt.php(521): OCA\\\\Encryption\\\\Crypto\\\\Crypt->hasSignature('---------------...', 'AES-256-CTR')\\n#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/encryption\\\/lib\\\/crypto\\\/crypt.php(454): OCA\\\\Encryption\\\\Crypto\\\\Crypt->splitMetaData('---------------...', 'AES-256-CTR')\\n#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/encryption\\\/lib\\\/crypto\\\/encryption.php(364): OCA\\\\Encryption\\\\Crypto\\\\Crypt->symmetricDecryptFileContent('---------------...', '\\\\xAF\\\\xCB\\\\xE0~\\\\x92am\\\\xE8\\\\xE3O\\\\x92\\\\xBB\\\\xD6\\\\\\\\\\\\x17...', 'AES-256-CTR', 1, 0)\\n#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/files\\\/stream\\\/encryption.php(458): OCA\\\\Encryption\\\\Crypto\\\\Encryption->decrypt('---------------...', 0)\\n#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/files\\\/stream\\\/encryption.php(289): OC\\\\Files\\\\Stream\\\\Encryption->readCache()\\n#5 [internal function]: OC\\\\Files\\\\Stream\\\\Encryption->stream_read(8192)\\n#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_external\\\/3rdparty\\\/icewind\\\/streams\\\/src\\\/Wrapper.php(67): fread(Resource id #594, 8192)\\n#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_external\\\/3rdparty\\\/icewind\\\/streams\\\/src\\\/CallbackWrapper.php(88): Icewind\\\\Streams\\\\Wrapper->stream_read(8192)\\n#8 [internal function]: Icewind\\\\Streams\\\\CallbackWrapper->stream_read(8192)\\n#9 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/http\\\/lib\\\/Sapi.php(78): stream_copy_to_stream(Resource id #598, Resource id #605, '1210243')\\n#10 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(470): Sabre\\\\HTTP\\\\Sapi::sendResponse(Object(Sabre\\\\HTTP\\\\Response))\\n#11 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(248): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#12 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v1\\\/webdav.php(55): Sabre\\\\DAV\\\\Server->exec()\\n#13 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/remote.php(137): require_once('\\\/srv\\\/www\\\/htdocs...')\\n#14 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/encryption\\\/lib\\\/crypto\\\/crypt.php\",\"Line\":556,\"User\":\"root\"}","level":4,"time":"2016-02-23T11:09:50+00:00","method":"GET","url":"\/owncloud\/remote.php\/webdav\/gdrive\/enc\/test.jpg?downloadStartSecret=rakdygjfmrh08uxr"}

Note that GDrive's stream is not seekable, in case it's related somehow.

Strangely it works properly on Amazon S3, which isn't seekable either.

Versions

ownCloud 9.0 git master fae6643

@LukasReschke @schiesbn

@LukasReschke
Copy link
Member

@PVince81 Can you post the database entry that you have in the filecache for said files?

@LukasReschke LukasReschke self-assigned this Feb 23, 2016
@PVince81
Copy link
Contributor Author

MariaDB [owncloud]> select * from oc_filecache where name='test.jpg' and storage=5 \G
*************************** 1. row ***************************
          fileid: 194
         storage: 5
            path: test.jpg
       path_hash: 0412c29576c708cf0155e8de242169b1
          parent: 44
            name: test.jpg
        mimetype: 9
        mimepart: 5
            size: 1210243
           mtime: 1456231072
   storage_mtime: 1456231072
       encrypted: 1
unencrypted_size: 0
            etag: 56cc5297f0b1f
     permissions: 27
        checksum: NULL

@LukasReschke
Copy link
Member

That's caused by \OC\Files\Storage\Wrapper\Encryption::readFirstBlock which sets the header size to 1739 instead the expected value of 8912.

With the wrong header size a ton of dashes are then passed to the decryption method which fails here since dashes are not a valid cipher text.

In fact, when doing var_dump(fread($response->getBody(), 8508)); in \OC\Files\Storage\Google::fopen I get only a response length of 1739 while with a stream_get_contents I get the whole file returned.

Let me dig further…

LukasReschke added a commit that referenced this issue Feb 23, 2016
Using the Guzzle stream directly here will only return 1739 characters for `fread` instead of all data. This leads to the problem that the stream is read incorrectly and thus the data cannot be properly decrypted => 💣

This approach copies the data into a local temporary file, as done before in all stable releases as well as other storage connectors.

While this approach will load the whole file into memory, this is already was has happened before in any stable release as well. See d608c37 for the breaking change.

To test this enable Google Drive as external storage and upload some files with encryption enabled. Reading the file should fail now.

Fixes #22590
@LukasReschke
Copy link
Member

Patch is at #22604

@PVince81
Copy link
Contributor Author

@LukasReschke is the number of bytes returned by fread() consistent or variable ?
I wonder if Guzzle tries to read synchronously and sometimes the network delivers less bytes than expected. Maybe we need to tell it do used buffered mode ? (if there is such a thing)

@PVince81
Copy link
Contributor Author

I tested with a docker DAV server, and encryption works fine. (docker run -d -e USERNAME=test -e PASSWORD=test -p 8888:80 morrisjobke/webdav)

Let me double check whether we use a guzzle download stream there too.

@PVince81
Copy link
Contributor Author

Yup, see https://github.com/owncloud/core/blob/master/lib/private/files/storage/dav.php#L344

Maybe it is a network buffer thing of some sorts ?

@PVince81
Copy link
Contributor Author

Works fine on v8.2.2.

@PVince81
Copy link
Contributor Author

Interesting: I uploaded a file in v8.2.2, encrypted. Bad then we didn't have the signature.
Then upgraded to master, also cannot download the file. It's a stream thing.

@PVince81
Copy link
Contributor Author

Ok, the block size is always 1760 big in readFirstBlock even though we requested 8k.
Weiiiird.

@PVince81
Copy link
Contributor Author

I found where Guzzle does the request, here: https://github.com/owncloud/3rdparty/blob/fc0c1159f4e275186b45f8454aaa89f90718b89e/guzzlehttp/ringphp/src/Client/StreamHandler.php#L406

It's just a plain fopen on the URL... Maybe PHP has trouble with URLs and buffering ?

@PVince81
Copy link
Contributor Author

Since it's a plain fopen, I can imagine that it's not buffered...

Now if Guzzle had a way to buffer this...

@PVince81
Copy link
Contributor Author

I see Guzzle has a CachingStream, now need to find out how to use its stream wrapper to make this work.

@PVince81
Copy link
Contributor Author

Indeed, fread might return smaller packets: http://stackoverflow.com/questions/23458069/php-fread-chunk-length-not-taken-into-acount-correctly

@schiesbn @LukasReschke @icewind1991 at some point we might want/need to check all fread calls and make them loops.

Or use Guzzle's cached wrapper in more places.

LukasReschke added a commit that referenced this issue Feb 25, 2016
Using the Guzzle stream directly here will only return 1739 characters for `fread` instead of all data. This leads to the problem that the stream is read incorrectly and thus the data cannot be properly decrypted => 💣

This approach copies the data into a local temporary file, as done before in all stable releases as well as other storage connectors.

While this approach will load the whole file into memory, this is already was has happened before in any stable release as well. See d608c37 for the breaking change.

To test this enable Google Drive as external storage and upload some files with encryption enabled. Reading the file should fail now.

Fixes #22590
@lock
Copy link

lock bot commented Aug 6, 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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants