occ files:scan does not invalidate checksums if file changed on disk.

[dragotin]

Steps to reproduce

1. Create a file on client
2. Wait until it is synced.
3. Check that it has a checksum in the database, which was sent from the client.
4. Edit the file on the server file system, which is not supported and forbidden as you should know.
5. Feel bad about yourself and think occ files:scan --all will fix it.
6. Start occ files:scan --all as you are an experienced admin for ownCloud
7. See that the checksum of the file has not changed and was not invalidated 👎

Expected behaviour

When a files mtime on disk is changed, the checksum on the server needs to be recalculated. If that can not be done, it needs at least to be invalidated. The occ file scan must compare the mtime of the file and the one stored in the db and wipe the checksum if they do not match. In addition there should be a switch to wipe all checksums, if they can not be recalculated.

Having a wrong checksum for a file is a critical problem as the client will complain about a corrupted file on download .

Server configuration

Operating system:
Linux
Web server:
apache
Database:
mysql
PHP version:
5.5.14
ownCloud version: (see ownCloud admin page)
9.0.x, master branch, 15833d9
Updated from an older ownCloud or fresh install:

Where did you install ownCloud from:
git
Are you using external storage, if yes which one: local/smb/sftp/...
no external storage
Are you using encryption: yes/no
no encryption
Are you using an external user-backend, if yes which one: LDAP/ActiveDirectory/Webdav/...
no user-backend.

[rullzer]

Yes agreed this must be handled properly.
I think the safest thing would even be to just invalidate all checksums on file scan. As we have (without recalculating) no guarantees the data is unchanged...

[danimo]

<sysadmin_hat>Safe? Yes. Expected: no. Scanning has become the default solution when something is wrong. Invalidating checksums by default makes this impossible, as it means resynch with all clients. Not something you want with a considerable amount of users and devices.</sysadmin_hat>

[rullzer]

@danimo well if we don't want to calculate checksums on the server this is kind of playing with fire...

I guess the 'second' safest thing to do it compare mtime + size...

[rullzer]

Raised to sev2 as this has to be fixed for 9.0.2 from my POV. @cmonteroluque

[dragotin]

@danimo no, invalidating the checksums would not mean to resync to all clients, as we use the ETags to decide upon syncing as you remember.

I think that there is no alternative to invalidating all checksums on rescan as, with this bug, we can not trust any checksum that is stored in the server db, as long as it is not validated.

[danimo]

@dragotin you are right, of course.

[ghost]

@rullzer agreed

[moscicki]

You should NOT to send any checksums in the reply headers (GET) if you are unable to guarantee that these checksums are correct. So only send checksums if you are sure they are correct by design. So I think you should disable sending any checksums until you fix this issue in the server.

[PVince81]

CC @icewind1991 since it's related to scanning.

[rullzer]

PR that fixes this in #24098

[rullzer]

Stable 9 backport in #24129

[rullzer]

Merged. Closing.

[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.