[stable8.2] Heal unencrypted file sizes at download time

[PVince81]

Whenever the unencrypted size is -1 or equal to the encrypted size,
interpret this as an error and enable a repair routine inside the
encryption stream wrapper.

The repair routine will count how many bytes of decrypted content was
returned and then update the cache when closing the stream.

This fix is very specific to encryption but has the advantage of not impairing (slowing down) the regular download cases.

@MorrisJobke @icewind1991 @schiesbn

• backport approval (after testing)
• add unit tests

Fixes #21751 but only for encryption

[mention-bot]

By analyzing the blame information on this pull request, we identified @schiesbn, @jknockaert and @DeepDiver1975 to be potential reviewers

[jknockaert]

I understand this is part of a mechanism that fixes inconsistencies in the database when they get noticed? Sounds like an excellent idea. I will have a look at it over the weekend.

[PVince81]

For every test, manually update the "size" column in the database using the real file size from disk (in the data folder)

• TEST: with small files (less than 8kb)
• TEST: with big files (more than 8kb)
• TEST: with files that have 6192 bytes (the exact unencrypted block length)
• TEST: web UI repair
• TEST: Webdav repair
• TEST: sync client download behavior
• TEST: smash

[PVince81]

• TODO: we should probably also propagate the file size to the parent folders @icewind1991 ?

[MorrisJobke]

I tested this on two broken instances and it fixed the issue there. The clients are syncing again (and by this fixing the broken entries) and also the web UI works again.

• TEST: with small files (less than 8kb)
• TEST: with big files (more than 8kb)
• TEST: with files that have 6192 bytes (the exact unencrypted block length)
• TEST: web UI repair
• TEST: Webdav repair
• TEST: sync client download behavior
• TEST: smash

[MorrisJobke]

This is a 👍 from my side.

[MorrisJobke]

Just got some feedback - it works in in IE/Edge but not in Firefox or Chrome (also not on the 2nd or 3rd try). I guess we need to check this again.

[PVince81]

Hopefully not some caching issue?
Were you able to see the actual request in the network console, whether it returned the correct content-length ?

[MorrisJobke]

Were you able to see the actual request in the network console, whether it returned the correct content-length ?

I just asked for some curl responses.

[MorrisJobke]

Were you able to see the actual request in the network console, whether it returned the correct content-length ?

I also asked for the network console output.

[MorrisJobke]

Content-Length: 984680

curl download said:

961k

Seems to be correct.

[MorrisJobke]

@PVince81 mentioned that this could also be an off-by-one error.

[PVince81]

Ok, I was able to reproduce the bug by uploading a file that has exactly 8191 bytes.
When trying to heal it, the content-length comes out as 6126, which is wrong.

[PVince81]

Looks like my tentative optimization is wrong. For some reason the size of $result is bigger than unencrypted block size ?!! I'll remove the optimization and make it always use strlen, it's slower but safer. Considering that we only repair once, the slowness might be fine.

[PVince81]

@jknockaert

1. so far there are some cases where the "size" column contains the encrypted size instead of the encrypted ones. In other cases the "encrypted" flag isn't set. It seems that it's due to people trying to restore encrypted files from backup and using occ files:scan afterwards. see Make it possible to restore individual files/folders from backup (also with encryption) #22096
2. a background job isn't feasible because such job doesn't have access to people's passwords + encryption keys to be able to decrypt the files and find out their real sizes

[jknockaert]

The proposed fix will make things worse when it is called in the context of a http-range request: the size will be off without possibility to detect it later on; so basically the oc file system corrupts. Upon opening the encrypted file and detecting an inconsistency the correct unencrypted filesize should be determined using the getFileSize procedure I implemented in #11659 (which was never released before encryption 2.0 arrived). This implementation is efficient and robust. As for how to detect an inconsistency there are multiple ways: either a test which is evaluated upon every file opening, or maybe a flag set in a background process (but still then the flag value needs to be evaluated upon opening). In case the unencrypted filesize is updated a flag should be set to write it back to the database upon closing the stream.

[PVince81]

@jknockaert I'm worried about performance or potential slow downs, that's why we're trying to do it on the fly in a single-pass. Regarding range requests, maybe in that specific situation the repairing code should simply be disabled.

Does getFileSize require re-reading the full file or is it able to somehow compute the correct size without performance impact ?

[PVince81]

• TODO: look into potential range request issue

[jknockaert]

@PVince81 getFileSize (which would have to be updated for compatibility with encryption 2.0) doesn't require re-reading the full file for seekable (encrypted) streams.
The whole encryption stream wrapper was designed under the assumption that the unencrypted file size is known upon opening the file. I would prefer a one-off performance penalty if that ensures integrity.
The corruption would happen upon every case where the file is read incompletely or an fseek is issued before closing; range requests are just an example of what triggers such behaviour.

[PVince81]

A one-shot penatly is acceptable, but before doing so we should detect whether a repair is needed in the first place. And only if a repair needs to be done, then do it once.

Would it work if we modified this PR to call getFileSize() + fix the cache entry at the beginning as soon as the discrepancy is detected: https://github.com/owncloud/core/pull/22008/files#diff-7e61c5b95e4e1be3406a3959ae86d2d2R262 ? Or alternatively, at the end.

[PVince81]

@jknockaert as usual, thanks a lot for your advice 😄

[jknockaert]

@PVince81 Sounds like a good idea. So upon stream opening:

1. evaluate need to repair
2. getFileSize
3. update cache entry (and $this->unencryptedSize)
4. proceed as if nothing happened
   getFileSize is no longer in the codebase; I'm not sure where and how to re-implement it (but I'd be happy to review it once it's there).

[PVince81]

@schiesbn can you comment on this ?

[jknockaert]

While I am thinking of the topic, I wonder if we should add an option somewhere to do a file scan and fix/report encryption issues. Such a scan would check for unencrypted files, incorrect file sizes, but also for the presence of all required keys. There's now far too much opportunities for corrupting the encryption process, eg. by php timeouts, without the possibility to validate. The scan would then fix what is fixable (access to private keys needed), and report everything else. I'm also thinking of the recovery keys here; the idea introduced in #18951 touches this concern. But maybe we should open a separate ticket for this.

[PVince81]

@jknockaert I thought about this too. Unfortunately occ files:scan on the CLI doesn't have the user's passwords, so unable to compute and fix unencrypted sizes. The most it could do is set the "encrypted" flag and maybe the size to "-1" to trigger this PR's logic, if "encrypted" was 0 initially.

[jknockaert]

@PVince81 What about adding this to the user settings page? There's already an encryption related section where the user can activate the recovery key functionality.

[PVince81]

@jknockaert there is a risk to run into timeouts if the PHP call takes too long.

[jknockaert]

@PVince81 OK, so the test would suffer from the same corruption that it tries to detect.

[PVince81]

So far I still think this is the best solution: #22008 (comment)

[PVince81]

@schiesbn any update on this critical issue ?

[schiessle]

I think the easiest solution is not to try to fix the unencrypted size in fseek. Otherwise it will become to complex for the corner case we want to fix. See my last commit.

Otherwise code from @PVince81 looks good and works as expected 👍

[PVince81]

note for @jknockaert we decided to disable the repair routine when fseek was detected.

[PVince81]

Forward port here #22567

[jknockaert]

No this is not good. The entire wrapper is designed under the assumption that the unencrypted filesize is correctly known upon access. That's why I first had to merge #11659 (to determine the unencrypted filesize without using the wrapper) before I could rewrite the wrapper in #14383. The Encryption 2.0 wrapper is conceptually identical to #14383.
Let me just give an example where the current PR goes wrong. Imagine a request for a file with a wrong size. Then an fseek happens searching from the back of the file. Because the filesize is wrong (whatever it is set to), it will result in invalid results (which may translate in file corruption).
Fixing the unencrypted file size upon opening allows to handle everything properly.

[jknockaert]

@schiesbn What's complex about writing a procedure to determine unencrypted size and write it to the filecache? It would fit in nicely together in one place in the code and in a place where it is called deterministically (when opening the file). The current PR has code spread out over diverse subroutines which are called by low level php code in a fairly unpredictable manner (remember the php bug we hit in #16452).

[schiessle]

@jknockaert after thinking once more about it I think you are right... Please have a look at this PR #22579. Is this the solution you had in mind?

[PVince81]

Obsoleted by the better solution #22579 which doesn't require two downloads

[lock]

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.