Sanitizing of filenames is probably not enough (was: GHSA-g5q8-pjqm-frc9)

[bohwaz]

I reported issue 1004379 on https://support.collaboraoffice.com/ in January 2024, where filenames containing a # (hash) would trigger an issue in Collabora convert-to API.

I was also noting this:

I'm also surprised to see the filename used to store the temporary file, as it might introduce security issues? Here it's the "#" character that creates a issue, but I'm wondering if the file name is properly escaped / protected against directory traversals?

I see that this has been fixed here: 5bd1f1d

This only restricts the use of some characters, but there might be other issues left with allowing any user-supplied filename to be stored on Collabora filesystem.

Also I see in coolwsd logs that the user-supplied filenames are also used outside of the convert-to API.

I'm worried that using the user-supplied filename may lead to path traversal, or other security (or non-security) issues.

I can see that the filename is actually used to create and retrieve files from the filesystem in /opt/cool/child-roots. This seems to be using a chroot.

I could successfully make coolwsd crash just by sending a 300+ long filename:

wsd-123910-126584 2024-05-10 18:07:11.327047 +0200 [ docbroker_002 ] WRN Crash detected, will
quarantine last version of [http%3A%2F%2Fdev.paheko.localhost%3A80%2Fwopi%2Ffiles%2F9nw1l8cz38cg] if necessary. Quarantine enabled: false, Storage available: true| wsd/DocumentBroker.cpp:652wsd-123910-126584 2024-05-10 18:07:11.327066 +0200 [ docbroker_002 ] WRN Quarantining the .upl
oading file: /opt/cool/child-roots/123910-d7b0864c/pInflbxoobbSxlRo/tmp/user/docs/AxPoLfZXBdvlnb5D/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.odt.uploading| wsd/DocumentBroker.cpp:658

Same result if I supplied an empty string for the file name.

I could also supply an arbitrary path inside the chroot, though I'm not sure if this could be exploited as I'm not a professional hacker:

wsd-123910-130305 2024-05-10 18:17:53.082154 +0200 [ docbroker_004 ] INF WOPI::GetFile downloa
ded 0 bytes from [http://dev.paheko.localhost/wopi/files/9nw1l8cz38cg/contents?access_token=&ac
cess_token_ttl=1715393872000] -> /opt/cool/child-roots/123910-d7b0864c/ouQrbzs4rTibp5t7/tmp/use
r/docs/yMutbR1g0FOt5wps/../../../../tmp/cool-JZJycaFw9SZ7uAeA/user/user/basic/script.xlc in 19m
s| wsd/Storage.cpp:1158
wsd-123910-130305 2024-05-10 18:17:53.082181 +0200 [ docbroker_004 ] INF SHA1 for DocKey [http
%3A%2F%2Fdev.paheko.localhost%3A80%2Fwopi%2Ffiles%2F9nw1l8cz38cg] of [/tmp/user/docs/yMutbR1g0F
Ot5wps/../../../../tmp/cool-JZJycaFw9SZ7uAeA/user/user/basic/script.xlc]: da39a3ee5e6b4b0d3255b
fef95601890afd80709| wsd/DocumentBroker.cpp:1176
wsd-123910-130305 2024-05-10 18:17:53.082259 +0200 [ docbroker_004 ] DBG Quarantine ctor for [
http%3A%2F%2Fdev.paheko.localhost%3A80%2Fwopi%2Ffiles%2F9nw1l8cz38cg]| wsd/QuarantineUtil.cpp:5
6
wsd-123910-130305 2024-05-10 18:17:53.082268 +0200 [ docbroker_004 ] INF TileCache ctor for ur
i [http://dev.paheko.localhost/wopi/files/9nw1l8cz38cg?access_token=&access_token_ttl=171539387
2000], modifiedTime=0], dontCache=false| wsd/TileCache.cpp:48
wsd-123910-130305 2024-05-10 18:17:53.082295 +0200 [ docbroker_004 ] INF Filesystem [/opt/cool
/child-roots/123910-d7b0864c/.] has 638612 MB free (34.3432%).| common/FileUtil.cpp:592
wsd-123910-123947 2024-05-10 18:17:53.082309 +0200 [ admin ] DBG Added admin document [http%3A
%2F%2Fdev.paheko.localhost%3A80%2Fwopi%2Ffiles%2F9nw1l8cz38cg].| wsd/AdminModel.cpp:522
wsd-123910-123947 2024-05-10 18:17:53.082329 +0200 [ admin ] INF docstats : adding a document
: ../../../../tmp/cool-JZJycaFw9SZ7uAeA/user/user/basic/script.xlc, created by : XXXX
XXX, using WopiHost : dev.paheko.localhost, allocating memory of : 137412| wsd/AdminModel.cpp:5
69

I wouldn't advise for using a whitelist of characters (eg. [a-z0-9_-]+), as this would make it hard with filenames only consisting of unicode sequences, and there might be potential issues with invalid unicode sequences. A better idea would be to use a hash of the filename, so that any potential issue cannot be exploited.

If using a hash of the filename, it would be best to also keep showing the original filename in the logs, so that the sysadmin can grep the logs for a document.

@caolanm Hi, I'm getting the crash not using the "convert-to" API but when opening a document, when the WOPI server is providing invalid filenames, eg. aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.odt

This is what my WOPI server is returning to Collabora for CheckFileInfo:

{
    "BaseFileName": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.odt",
    "UserFriendlyName": "XXXX",
    "OwnerId": 0,
    "UserId": 2,
    "Version": "b8ae79d17f0df5180a9944e5ce529ac3",
    "ReadOnly": false,
    "UserCanWrite": true,
    "UserCanRename": true,
    "DisableCopy": false,
    "UserCanNotWriteRelative": true,
    "UserExtraInfo": {
        "avatar": "http:\/\/dev.paheko.localhost\/user\/avatar\/2"
    },
    "LastModifiedTime": "2024-05-10T18:06:02+0200"
}

I tried with "convert-to" API and it falso fails with a 300+ bytes long filename:

wsd-13156-13199 2024-05-13 12:30:56.599736 +0200 [ websrv_poll ] INF #19: Client HTTP Request:
POST /lool/convert-to HTTP/1.1 Host: localhost:9980 / Accept: / / Content-Length: 18287 / Co
ntent-Type: multipart/form-data; boundary=------------------------365aa2970361df4b| net/Socket.
cpp:1190
wsd-13156-13199 2024-05-13 12:30:56.599777 +0200 [ websrv_poll ] DBG #19: Handling request: /l
ool/convert-to| wsd/COOLWSD.cpp:4262
wsd-13156-13199 2024-05-13 12:30:56.599806 +0200 [ websrv_poll ] INF #19: Post request: [/lool
/convert-to]| wsd/COOLWSD.cpp:5001
wsd-13156-13199 2024-05-13 12:30:56.600065 +0200 [ websrv_poll ] DBG Storing incoming file to:
/opt/cool/child-roots/13156-003a67cd/tmp/incoming/cool-bBlttHL3v9xdm4lK/aaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.odt| w
sd/COOLWSD.cpp:820
…
wsd-13156-15568 2024-05-13 12:30:56.601191 +0200 [ docbroker_004 ] ERR Local file URI [/opt/co
ol/child-roots/13156-003a67cd/tmp/incoming/cool-bBlttHL3v9xdm4lK/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.odt] invalid or doesn't exist.| wsd/Storage.cpp:357
wsd-13156-15568 2024-05-13 12:30:56.601242 +0200 [ docbroker_004 ] ERR loading document except
ion: Invalid URI: /opt/cool/child-roots/13156-003a67cd/tmp/incoming/cool-bBlttHL3v9xdm4lK/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.odt| wsd/DocumentBroker.cpp:2679
I could get the same issue if I provide a file name containing a single space (' ')…

[bohwaz]

Source: https://github.com/CollaboraOnline/online/security/advisories/GHSA-g5q8-pjqm-frc9

[caolanm]

For the "WRN Crash detected" part as far as I can tell there isn't actually a crash, just the file length is so long it cannot be saved due to the underlying file system limits and that error is reported thusly.

From the other report, for me:

With a local nextcloud and modifying /var/lib/nextcloud/apps/richdocuments/lib/Controller/WopiController.php to force a long name in $BaseFileName the error report can be reproduced

On the filename in general, I think we need to at least retain the suffix to help determine what filter to use for ambiguous file types. But maybe no specific need to base the rest of the filename on whatever was the original filename.

[caolanm]

But on filenames, now that I think about it a little more, then writer, calc, etc have a "File name" fields so if we replace the original filename with a pseudo one then those won't report their original filename so that's not ideal either.

[bohwaz]

Then the filename reported should be the one given by BaseFileName and not the one of the stored file on the filesystem…

I continue to think that storing arbitrary file names coming from potentially malicious users on the filesystem is a bad idea.