[Bug]: File with content 0x30 is reproducible corrupted when encryption is enabled.

dennis-grunert commented 3 weeks ago

⚠️ This issue respects the following points: ⚠️

[X] This is a bug, not a question or a configuration/webserver/proxy issue.
[X] This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
[X] Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
[X] I agree to follow Nextcloud's Code of Conduct.

Bug description

A file with the contents 0x30 (i.e., a literal "0" (digit zero as string) without line feed) is not correctly synced to the server when (user-key) encryption is enabled. The content is lost, i.e., the data is corrupted.

Important notes:

With encryption disabled (tested on another NextCloud instance), the file is saved correctly. Therefore, this seems to be specific to encryption.
The missing line feed is not the reason since this does not happen if another single character is written to a file without line feed. Only the literal zero character 0x30 seems to trigger it. It seems that it is incorrectly checked if a file content is empty (e.g., in JavaScript, the string "0" would be falsely, but not the string "1").
Even if I could reproduce it only for this special file, there is maybe a larger problem hidden, which could lead to more data corruption in other use cases.

Steps to reproduce

Make sure that you have a NextCloud instance with activated user-key encryption (occ encryption:disable-master-key). Probably it is also reproducible with a server key (default).
Create the following files, e.g., with Linux:
- echo -n -e '\x30' > A.txt ("0")
- echo -n -e '\x30\x0A' > B.txt ("0" with LF)
- echo -n -e '\x31' > C.txt ("1")
- echo -n -e '\x30\x30' > D.txt ("00")
- echo -n -e '\x20' > E.txt (space)
- echo -n -e '\x0A' > F.txt (LF)
- echo -n -e '\x00' > G.txt (null)
Upload the files: Put the files either in the NextCloud sync directory of the client or upload them via the web UI.
Notice that file A.txt is corrupted:
- Opening the file A.txt in the browser editor of NextCloud, it doesn't show the single character but is displayed as empty.
- Downloading the file A.txt via the browser results in a zero byte file being downloaded.
- Downloading the file A.txt with the file sync of the Desktop client that doesn't had the file yet results in the error message "The downloaded file is empty, but the server said it should have been 1 B."
Notice that all other files are saved correctly.

Expected behavior

File A.txt is uploaded correctly to the server like the other files of the above test.

Installation method

None

Nextcloud Server version

29

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.1

Web server

Apache (supported)

Database engine version

MariaDB

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

Encryption is Enabled

What user-backends are you using?

[X] Default user-backend (database)
[ ] LDAP/ Active Directory
[ ] SSO - SAML
[ ] Other

Configuration report

{
    "system": {
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "***REMOVED SENSITIVE VALUE***"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "mysql",
        "version": "29.0.4.1",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbtableprefix": "oc_",
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "forcessl": true,
        "theme": "",
        "maintenance": false,
        "secret": "***REMOVED SENSITIVE VALUE***",
        "loglevel": 2,
        "trashbin_retention_obligation": "auto",
        "updater.release.channel": "stable",
        "overwrite.cli.url": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpmode": "sendmail",
        "app_install_overwrite": [
            "calendar"
        ],
        "mysql.utf8mb4": true,
        "memcache.local": "\\OC\\Memcache\\APCu",
        "default_phone_region": "DE",
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_sendmailmode": "smtp",
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "maintenance_window_start": 1,
        "preview_max_x": 1024,
        "preview_max_y": 1024
    }
}

List of activated Apps

Enabled:
  - activity: 2.21.1
  - bruteforcesettings: 2.9.0
  - calendar: 4.7.15
  - cfg_share_links: 5.1.1
  - circles: 29.0.0-dev
  - cloud_federation_api: 1.12.0
  - comments: 1.19.0
  - contacts: 6.0.0
  - dav: 1.30.1
  - encryption: 2.17.0
  - federatedfilesharing: 1.19.0
  - federation: 1.19.0
  - files: 2.1.0
  - files_downloadlimit: 2.0.0
  - files_pdfviewer: 2.10.0
  - files_reminders: 1.2.0
  - files_sharing: 1.21.0
  - files_versions: 1.22.0
  - firstrunwizard: 2.18.0
  - logreader: 2.14.0
  - lookup_server_connector: 1.17.0
  - nextcloud_announcements: 1.18.0
  - notes: 4.10.1
  - notifications: 2.17.0
  - oauth2: 1.17.0
  - password_policy: 1.19.0
  - photos: 2.5.0
  - privacy: 1.13.0
  - provisioning_api: 1.19.0
  - recommendations: 2.1.0
  - related_resources: 1.4.0
  - serverinfo: 1.19.0
  - settings: 1.12.0
  - sharebymail: 1.19.0
  - support: 1.12.0
  - survey_client: 1.17.0
  - systemtags: 1.19.0
  - text: 3.10.1
  - theming: 2.4.0
  - twofactor_backupcodes: 1.18.0
  - twofactor_totp: 11.0.0-dev
  - updatenotification: 1.19.1
  - user_status: 1.9.0
  - viewer: 2.3.0
  - weather_status: 1.9.0
  - workflowengine: 2.11.0
Disabled:
  - admin_audit: 1.19.0
  - contactsinteraction: 1.10.0 (installed 1.5.0)
  - dashboard: 7.9.0 (installed 7.0.0)
  - end_to_end_encryption: 1.15.2 (installed 1.15.2)
  - files_external: 1.21.0
  - files_trashbin: 1.19.0 (installed 1.19.0)
  - richdocuments: 8.4.4 (installed 8.4.4)
  - suspicious_login: 7.0.0
  - user_ldap: 1.20.0

Nextcloud Signing status

No errors have been found.

Nextcloud Logs

No response

Additional info

No response

susnux commented 3 weeks ago

2. On a client (here Linux), create the following files in the sync directory:

Does it make a difference if you upload using the webui?

dennis-grunert commented 3 weeks ago

Does it make a difference if you upload using the webui?

It is also reproducible when uploading the file in the web UI. I have updated the steps to reproduce above and uploaded the file bad A.txt to this comment.

A.txt

susnux commented 3 weeks ago

Can you check the nextcloud log file for any error? Especially things with "app": "encryption"

dennis-grunert commented 3 weeks ago

Can you check the nextcloud log file for any error? Especially things with "app": "encryption"

There are no logs in the <data directory>/nextcloud.log during or after upload.

I have the following encryption keys that look good at a first glance.

<data directory>/<username>/files_encryption/keys/files/A.txt/OC_DEFAULT_MODULE/<username>.shareKey
<data directory>/<username>/files_encryption/keys/files/A.txt/OC_DEFAULT_MODULE/recoveryKey_<id>.shareKey (since I have recovery enabled as suggested by the NextCloud documentation)

But I just checked the encrypted file located at

<data directory>/<username>/files/A.txt

and it ended up being just metadata without any encrypted data, see attached (I renamed it to A_encrypted,txt before uploading). As suspected, the encryption process ended without errors but it returned the wrong data.

A_encrypted.txt

I think, I found the issue: As guessed, it is because of a wrong falsely check. All relevant methods are located in class apps/encryption/lib/Crypto/Encryption.php:

encrypt() checks if the data chunk remaining to be encrypted is smaller than $this->getUnencryptedBlockSize(true), which is obvious true in our case of 1 Byte data. It then writes $data to $this->writeCache (see this line).
The write buffer is then supposed to be encrypted in end(). But there is a check in this line that only encrypts the data if !empty($this->writeCache). It is intended to check if the write cache actually contains any data.

The use of empty is wrong here since it not only returns true when the variable is not set but also if it equals to false (see the documentation). empty("0") is true, which is exactly what happens here. Instead, it should check strlen() > 0 since $this->writeCache was initiated earlier in begin() as empty string.

So it should be only this one-liner and a small unit test to fix it. I'm not a PHP developer, so I let this have someone else. Maybe this is also a candidate for the good first issue label?

susnux commented 3 weeks ago

Yes I asked because I suspected to see a Encryption Library, symmetrical encryption failed no content given, because of this line:

https://github.com/nextcloud/server/blob/86c2bd1c0ef149bbf10517e60de42c49ec452566/apps/encryption/lib/Crypto/Crypt.php#L144

dennis-grunert commented 3 weeks ago

Good catch! I guess both places need to be fixed then to be sure.

nextcloud / server