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

Closed
dragotin opened this Issue Apr 4, 2016 · 12 comments

Projects

None yet

8 participants

@dragotin
Member
dragotin commented Apr 4, 2016

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.

@dragotin dragotin added the bug label Apr 4, 2016
@rullzer
Contributor
rullzer commented Apr 4, 2016

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
Member
danimo commented Apr 4, 2016

<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
Contributor
rullzer commented Apr 4, 2016

@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 rullzer added sev2-high and removed sev3-medium labels Apr 4, 2016
@rullzer rullzer added this to the 9.0.2-next-maintenance milestone Apr 4, 2016
@rullzer
Contributor
rullzer commented Apr 4, 2016

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

@dragotin
Member
dragotin commented Apr 4, 2016

@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
Member
danimo commented Apr 4, 2016

@dragotin you are right, of course.

@cmonteroluque
Contributor

@rullzer agreed

@ckamm ckamm added a commit to ckamm/owncloud-client that referenced this issue Apr 12, 2016
@ckamm ckamm Disable validation of transmission checksums by default #4638
The server may send invalid checksums when files change on disk,
see owncloud/core#23783.

Introduces a new validateChecksumOnDownload configuration and
branding option.
f9d52e6
@ckamm ckamm added a commit to ckamm/owncloud-client that referenced this issue Apr 12, 2016
@ckamm ckamm Disable validation of transmission checksums by default #4638
The server may send invalid checksums when files change on disk,
see owncloud/core#23783.

Introduces a new validateChecksumOnDownload configuration and
branding option.
783f6d0
@ckamm ckamm added a commit to ckamm/owncloud-client that referenced this issue Apr 13, 2016
@ckamm ckamm Disable validation of transmission checksums by default #4638
The server may send invalid checksums when files change on disk,
see owncloud/core#23783.
5aecbdf
@ckamm ckamm added a commit to ckamm/owncloud-client that referenced this issue Apr 14, 2016
@ckamm ckamm Disable validation of transmission checksums by default #4638
The server may send invalid checksums when files change on disk,
see owncloud/core#23783.
352f5d2
@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
Collaborator

CC @icewind1991 since it's related to scanning.

@PVince81 PVince81 added sev1-critical and removed sev2-high labels Apr 19, 2016
@rullzer rullzer was assigned by PVince81 Apr 19, 2016
@rullzer rullzer added a commit that referenced this issue Apr 19, 2016
@rullzer rullzer When the scanner detects a file is changed clear checksum
Fixes #23782 and #23783

If the file scanner detects a changed file we clear the checksum while
we update the cache.

* Unit test added
9e3c146
@rullzer rullzer added a commit that referenced this issue Apr 19, 2016
@rullzer rullzer When the scanner detects a file is changed clear checksum
Fixes #23782 and #23783

If the file scanner detects a changed file we clear the checksum while
we update the cache.

* Unit test added
0dbcbc4
@rullzer
Contributor
rullzer commented Apr 19, 2016

PR that fixes this in #24098

@rullzer rullzer added a commit that referenced this issue Apr 20, 2016
@rullzer rullzer When the scanner detects a file is changed clear checksum
Fixes #23782 and #23783

If the file scanner detects a changed file we clear the checksum while
we update the cache.

* Unit test added
2296552
@rullzer
Contributor
rullzer commented Apr 20, 2016

Stable 9 backport in #24129

@rullzer
Contributor
rullzer commented Apr 21, 2016

Merged. Closing.

@rullzer rullzer closed this Apr 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment