share receiver can rename a file in a locked folder by using the lock token of the owner

[individual-it]

Steps to reproduce

1. create a folder with a file
2. share the folder to an other user
3. as owner lock the folder
4. as receiver find the locktoken by PROPFIND
5. as receiver rename the file by a MOVE request and giving the locktoken of the owner curl -u uu2:uu2 -X MOVE "http://localhost/owncloud-core/remote.php/dav/files/uu2/folder/file.txt" -H "Destination: http://localhost/owncloud-core/remote.php/dav/files/uu2/folder/renamed.txt" -H "If: (<opaquelocktoken:8eccec3d-9c12-49e0-8ad0-54c6a5169c9d>)" -v

Expected behaviour

HTTP response 423
File is not renamed

Actual behaviour

HTTP response 403
File is renamed

After sabre/dav 4.20:
HTTP response 201
File is renamed
(the behavior at least becomes consistent, even if it is wrong!)

Logs

Web server error log

ownCloud log (data/owncloud.log)

{"reqId":"jOOLALr1P5HctlMUkOnb","level":0,"time":"2019-01-31T08:06:19+00:00","remoteAddr":"127.0.0.1","user":"uu2","app":"webdav","method":"MOVE","url":"\/owncloud-core\/remote.php\/dav\/files\/uu2\/folder\/file.txt","message":"Exception: HTTP\/1.1 403 Forbidden: {\"Exception\":\"Sabre\\\\DAV\\\\Exception\\\\Forbidden\",\"Message\":\"\",\"Code\":0,\"Trace\":\"#0 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/WildcardEmitterTrait.php(96): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\LockPlugin->beforeUnlock('files\\\/uu2\\\/folde...', Object(OC\\\\Lock\\\\Persistent\\\\Lock))\\n#1 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Locks\\\/Plugin.php(347): Sabre\\\\DAV\\\\Server->emit('beforeUnlock', Array)\\n#2 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Locks\\\/Plugin.php(311): Sabre\\\\DAV\\\\Locks\\\\Plugin->unlockNode('files\\\/uu2\\\/folde...', Object(Sabre\\\\DAV\\\\Locks\\\\LockInfo))\\n#3 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/WildcardEmitterTrait.php(96): Sabre\\\\DAV\\\\Locks\\\\Plugin->afterUnbind(*** sensitive parameters replaced ***)\\n#4 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(648): Sabre\\\\DAV\\\\Server->emit('afterUnbind', Array)\\n#5 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/WildcardEmitterTrait.php(96): Sabre\\\\DAV\\\\CorePlugin->httpMove(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#6 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(464): Sabre\\\\DAV\\\\Server->emit('method:MOVE', Array)\\n#7 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(241): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#8 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(309): Sabre\\\\DAV\\\\Server->start()\\n#9 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/apps\\\/dav\\\/lib\\\/Server.php(299): Sabre\\\\DAV\\\\Server->exec()\\n#10 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/apps\\\/dav\\\/appinfo\\\/v2\\\/remote.php(31): OCA\\\\DAV\\\\Server->exec()\\n#11 \\\/home\\\/artur\\\/www\\\/owncloud-core\\\/remote.php(175): require_once('\\\/home\\\/artur\\\/www...')\\n#12 {main}\",\"File\":\"\\\/home\\\/artur\\\/www\\\/owncloud-core\\\/apps\\\/dav\\\/lib\\\/Connector\\\/Sabre\\\/LockPlugin.php\",\"Line\":110}"}

[individual-it]

other operations on items e.g. create file / put data do also work as the receiver by giving the owners lock token.
that makes the whole locking a voluntary system

[ownclouders]

GitMate.io thinks the contributors most likely able to help are @ownclouders, and @PVince81.

Possibly related issues are #34337 (receiver cannot rename a locked share), #2816 (By renaming a shared folder the old folder is kept), #2170 (By renaming a shared folder is not shared any more), #29554 (overwriting a file with a folder and a folder with a file needs better error reporting), and #34300 (empty lock-token when unlocking public link results in an exception).

[PVince81]

so far we've been following https://tools.ietf.org/html/rfc4918

but that spec doesn't define any restrictions to users

I found this proposal that extends that: https://tools.ietf.org/html/draft-reschke-webdav-locking-05#section-2.3

Submitting a lock token provides no special access rights. Anyone
can find out anyone else's lock token by performing lock discovery.
Locks MUST be enforced based upon whatever authentication mechanism
is used by the server, not based on the secrecy of the token values.

and https://tools.ietf.org/html/draft-reschke-webdav-locking-05#section-8.2.1

I'll check how far Sabre supports this...

In any case, I think we should protect access to only the lock owner, like we do for UNLOCK.

[PVince81]

after discussing with @DeepDiver1975 and others, we agreed that locking is not a security thing but convenience. so the server will not enforce access in case a lock token is "abused".

we'll keep this as is for the current release and might reconsider in future releases.

moving to backlog

[phil-davis]

Note: I updated the "Actual Behavior" in the original post. The receiver now gets a "success" 201 HTTP status, as well as the file is renamed. So the behavior is now consistent (HTTP status and actual action match).

What needs to be decided now is if this is a bug or a feature. If a bug, then it needs to be scheduled to be fixed. If a feature, then we need to adjust the tests so that we expect the current behavior as correct.

@micbar who can decide.