Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync can become broken for a user, if uploading a file to the sync target which contains a single quote in the filename #11630

Open
mrjo118 opened this issue Jan 12, 2025 · 0 comments
Labels
bug It's a bug desktop All desktop platforms high High priority issues

Comments

@mrjo118
Copy link
Contributor

mrjo118 commented Jan 12, 2025

Operating system

Windows

Joplin version

3.1.24

Desktop version info

Joplin 3.1.24 (prod, win32)

Client ID: f2646541b8454befae8b217e6393f6a8
Sync Version: 3
Profile Version: 47
Keychain Supported: Yes

Revision: d581264

Backup: 1.4.2

Current behaviour

The forum thread https://discourse.joplinapp.org/t/how-to-get-rid-of-sqlite-error/42999 has highlighted that usages of the IN clause in SQL statements do not sanitise the input values. In particular, BaseItem.loadItemsByIds uses input data coming from filenames on the sync target, which means if a file containing a single quote in the filename is manually added to the sync target's Joplin directory, it will break the sync. In the case of Dropbox sync, this will not automatically be remedied by deleting or renaming such files, because the api invoked to get the delta of files on Dropbox specifies the 'include_deleted' parameter as true in the code, meaning files previously of this state will continue to be returned by the delta logic and break sync for the user. The only way to remedy this is to then completely delete their sync target and start a new profile using an export of their notes.

Initially I thought that the lack of sanitisation for sql inputs could potentially allow harmful sql injection attacks by a malicious actor, however when making attempts to do so I was unable to trick the code to execute 2 sql statements within 1 string, making the scope of harm that could be caused to the user only causing the sql to fail. Even if it were possible to execute some other sql injection attack, the scope of this would be limited, because it is a client side database. I also attempted the same attack by manipulating the contents of the sqlite db of a joplin server user, for a note shared to another joplin server user. But it appears that joplin server is more robust in this regard, in that it did not allow the sync to become broken for the user the note was shared with.

Steps to reproduce the issue:
-Create a new profile and set up file system sync
-Create a new text file and save it as hat's.txt to the sync target folder (if the file was created before the last successful sync, then the error wont occur unless you modify / touch the file first)
-Sync the Joplin client. The sync will fail with an error, which is shown below the sync button and includes the failing sql which includes hat's as an input in the IN clause, omitting the extension

Expected behaviour

Queries which make use of the IN clause could use bind parameters to protect against sql injection, however there is a limit to the number of bind parameters which you can set on a query in sqlite, and it may be too low if the user syncs a very outdated profile which has since had a lot of changes. Instead, as the usages of the IN clause primarily take ids as input values, a suggested fix would be to strip out single quotes only, which would prevent the scenario that could cause the profile to be permanently broken for the user.

Logs

2025-01-09 14:39:39: Synchronizer: [warn] There was some errors:
2025-01-09 14:39:39: Synchronizer: [warn] Error: On query {"sql":"SELECT * FROM notes WHERE id IN ('foto's','aanhangsels','Nieuwe map','9424a9c42e984a728848a8503e...
Code: SQLITE_ERROR

also:

Code: SQLITE_ERROR
Error: On query {"sql":"SELECT * FROM notes WHERE id IN ('foto's','aanhangsels','Nieuwe map','9424a9c42e984a728848a8503ea2e41f','78c67f70e36140759c78ce1f54a67a45','7c633f2c543a48c38e51af8ecd89a2f4','f6cd2ae6c4dc427086bb3ee701c8a342','830dad2ffaf043a89c386ea341af4e91','6b02710085e54561a65975e4d5ee112f','b17d031a1b6e4fbaabf156056259cc12','cb6b73492b2f4c79a004e40e991751b0','70cb4d956b094af4af3632b2a01abddc','7a2d81ab395a4584a05c55e410bb5d3c','027af472a58a4d17906c0d8747422f03','768c56f398ac4ed296ebc4ec5b68cd70','786fbcae50c04cab83ceeb26fc30ac5b','ed9ad0987cb2483e89df7788ac19554f','8471f1e753b948aa9c7801d805c8e791')","params":[]}: Error: SQLITE_ERROR: near "s": syntax error: SELECT * FROM notes WHERE id IN ('foto's','aanhangsels','Nieuwe map','9424a9c42e984a728848a8503ea2e41f','78c67f70e36140759c78ce1f54a67a45','7c633f2c543a48c38e51af8ecd89a2f4','f6cd2ae6c4dc427086bb3ee701c8a342','830dad2ffaf043a89c386ea341af4e91','6b02710085e54561a65975e4d5ee112f','b17d031a1b6e4fbaabf156056259cc12','cb6b73492b2f4c79a004e40e991751b0','70cb4d956b094af4af3632b2a01abddc','7a2d81ab395a4584a05c55e410bb5d3c','027af472a58a4d17906c0d8747422f03','768c56f398ac4ed296ebc4ec5b68cd70','786fbcae50c04cab83ceeb26fc30ac5b','ed9ad0987cb2483e89df7788ac19554f','8471f1e753b948aa9c7801d805c8e791'):
at DatabaseDriverNode.sqliteErrorToJsError (C:\Users\Rolf\AppData\Local\Programs\Joplin\resources\app.asar\node_modules@joplin\lib\database-driver-node.js:23:18)
at JoplinDatabase.sqliteErrorToJsError (C:\Users\Rolf\AppData\Local\Programs\Joplin\resources\app.asar\node_modules@joplin\lib\database.js:27:30)
at JoplinDatabase.tryCall (C:\Users\Rolf\AppData\Local\Programs\Joplin\resources\app.asar\node_modules@joplin\lib\database.js:130:32)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async Note.modelSelectAll (C:\Users\Rolf\AppData\Local\Programs\Joplin\resources\app.asar\node_modules@joplin\lib\BaseModel.js:325:28)
at async BaseItem.loadItemsByIds (C:\Users\Rolf\AppData\Local\Programs\Joplin\resources\app.asar\node_modules@joplin\lib\models\BaseItem.js:182:28)
at async Synchronizer.start (C:\Users\Rolf\AppData\Local\Programs\Joplin\resources\app.asar\node_modules@joplin\lib\Synchronizer.js:785:36)
at async timeoutCallback (C:\Users\Rolf\AppData\Local\Programs\Joplin\resources\app.asar\node_modules@joplin\lib\registry.js:125:52)

@mrjo118 mrjo118 added the bug It's a bug label Jan 12, 2025
@laurent22 laurent22 added desktop All desktop platforms high High priority issues labels Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms high High priority issues
Projects
None yet
Development

No branches or pull requests

2 participants