Share20OcsController no longer provides info whether public link share is password-protected

[felix-schwarz]

Steps to reproduce

1. Retrieve list of shares

Expected behaviour

For public link shares with a password, the record included share_with and share_with_displayname with values, looking like this:

  <element>
   <id>29</id>
   <share_type>3</share_type>
   <uid_owner>admin</uid_owner>
   <displayname_owner>admin</displayname_owner>
   <permissions>1</permissions>
…
   <share_with>some value</share_with>
   <share_with_displayname>some value</share_with_displayname>
…
   <name>Public link</name>
  </element>

For public link shares without password, the share_with and share_with_displayname records were not included:

  <element>
   <id>29</id>
   <share_type>3</share_type>
   <uid_owner>admin</uid_owner>
   <displayname_owner>admin</displayname_owner>
   <permissions>1</permissions>
…
   <name>Public link</name>
  </element>

Actual behaviour

The share_with and share_with_displayname tags are now always included for all public links, regardless of whether they have a password set:

  <element>
   <id>29</id>
   <share_type>3</share_type>
   <uid_owner>admin</uid_owner>
   <displayname_owner>admin</displayname_owner>
   <permissions>1</permissions>
…
   <share_with>***redacted***</share_with>
   <share_with_displayname>***redacted***</share_with_displayname>
…
   <name>Public link</name>
  </element>

Possible solutions

I believe this code segment to be responsible for this:

		} elseif ($share->getShareType() === Share::SHARE_TYPE_LINK) {
			$result['share_with'] = '***redacted***';
			$result['share_with_displayname'] = '***redacted***';
			$result['name'] = $share->getName();

If it only returned the two tags ***redacted*** if the share has a password set, it'd match past behaviour. Speculatively, the code would look something like this then:

		} elseif ($share->getShareType() === Share::SHARE_TYPE_LINK) {
			if ($share->getSharedWith()) {
				$result['share_with'] = '***redacted***';
				$result['share_with_displayname'] = '***redacted***';
			}
			$result['name'] = $share->getName();

Impact

This change in Share20OcsController currently breaks the new ios-sdk's ability to provide accurate information to the app on whether a public link share has its password set. It currently indicates this for all public link shares for server versions returning ***redacted***.

Server configuration

Reproduced using demo.owncloud.org running ownCloud 10.2.0 (stable).

[micbar]

@felix-schwarz
This comes from https://github.com/owncloud/security-tracker/issues/301
PR was: #34691

[micbar]

Seems to be a security issue.

[micbar]

@felix-schwarz I agree with your proposal. Lets make it conditional.

I will add it to the sprint.
Maybe we could add it to the Hotfix Release.

@pmaier1 @patrickjahns

[IljaN]

@felix-schwarz Not sure If I understand the proposed fix (or maybe the problem). To my understanding ShareWith() will always be empty for public links as they have no explicit recipient.

If it only returned the two tags redacted if the share has a password set, it'd match past behaviour.

Before the fix both field share_with and share_with_displayname contained the password.
31b47dc. When password was null the field was filtered out because we remove empty/null fields before sending the response.

So I propose we rather check for $share->getPassword() instead of $share->getSharedWith(). If a password is set we fill respective fields with "redacted".

[IljaN]

@micbar @felix-schwarz WIP fix here: #35544, please comment/test.

[micbar]

@IljaN Your proposal makes sense.

@michaelstingl @felix-schwarz Is there a blue ticket for this?
I remember seeing a case where the sync client shows that a pw is set, even if it is not set at the server side.

Does the fix work for this problem too?
What are the clients expecting?

[felix-schwarz]

@IljaN Thanks for the explanation! Was just best-guessing above (I'm not familiar with the internals of the OC server sources).

#35544 looks exactly like what's needed then. Thanks! Is there any easy way for me to quickly bring up an instance with these changes, so I can run the OC iOS SDK's unit tests against it?

[micbar]

@felix-schwarz
basically you need a local LAMP (or in docker)

• Git clone owncloud/core
• git checkout stable10_fix_35541
• Run make
• Run php occ maintenance:install with the needed parameters

[michaelstingl]

@michaelstingl @felix-schwarz Is there a blue ticket for this?
I remember seeing a case where the sync client shows that a pw is set, even if it is not set at the server side.

Yeah, I think I've seen something similar, but I can't find it right now.

We'll need QA with all client platforms on Monday. (maybe also adjustments in the smoke tests) /cc @jesmrec @HanaGemela

Is there any easy way for me to quickly bring up an instance with these changes, so I can run the OC iOS SDK's unit tests against it?

@felix-schwarz Just patch the apps/files_sharing/lib/Controller/Share20OcsController.php in the 10.2 Docker?

[felix-schwarz]

@michaelstingl Good idea. Tried to do this, but couldn't proceed past trying to fire up a fresh 10.2 instance.

Using this minimal docker-compose.yml …

version: '3'

services:
  owncloud:
    image: owncloud/server:latest
    restart: always
    ports:
      - 36080:8080
      - 36180:80
      - 36443:443

… the server is stuck during startup both using Docker for Mac as well as trying the same with Docker running in an Ubuntu instance. Both do not proceed past starting apache:

Creating owncloud_owncloud_1 ... done
Attaching to owncloud_owncloud_1
owncloud_1  | Creating volume folders...
owncloud_1  | Creating hook folders...
owncloud_1  | Removing custom folder...
owncloud_1  | Linking custom folder...
owncloud_1  | Removing config folder...
owncloud_1  | Linking config folder...
owncloud_1  | Writing config file...
owncloud_1  | Fixing base perms...
owncloud_1  | Fixing data perms...
owncloud_1  | Fixing hook perms...
owncloud_1  | Installing server database...
owncloud_1  | creating sqlite db
owncloud_1  | ownCloud was successfully installed
owncloud_1  | ownCloud is already latest version
owncloud_1  | Writing objectstore config...
owncloud_1  | Writing php config...
owncloud_1  | Updating htaccess config...
owncloud_1  | .htaccess has been updated
owncloud_1  | Writing apache config...
owncloud_1  | Enabling cron background...
owncloud_1  | Set mode for background jobs to 'cron'
owncloud_1  | Touching cron configs...
owncloud_1  | Starting cron daemon...
owncloud_1  | Starting apache daemon...
owncloud_1  | [Sat Jun 15 20:20:29.419408 2019] [mpm_prefork:notice] [pid 179] AH00163: Apache/2.4.29 (Ubuntu) configured -- resuming normal operations
owncloud_1  | [Sat Jun 15 20:20:29.419500 2019] [core:notice] [pid 179] AH00094: Command line: '/usr/sbin/apache2 -f /etc/apache2/apache2.conf -D FOREGROUND'

It's not possible to connect to this server, then.

This docker-compose.yml used to work fine with previous ownCloud releases.

[HanaGemela]

@micbar the issues related to password are
owncloud/client#7189
#35419

[micbar]

@micbar the issues related to password are
owncloud/client#7189
#35419

Thank you!

[HanaGemela]

Tested with desktop client 2.5.4 and server 10.2.1.RC1
When user creates a public link in the desktop client or WebUI without a password, the password checkbox is not checked in the desktop client