1837514 - Extension sanitization (.lnk, .local) bypass using pipe (|) + "Always ask you where to save files"

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P2)

Description

[haxatron1]

+++ This bug was initially created as a clone of Bug #1815204 +++

Weirdly, when "Always ask you where to save files" enabled, it is again possible to bypass filters and save lnk / local / url / scf files in Firefox.

For instance, saving a file named "test.ln||k" with "Always ask you where to save files" gets transformed to a file named "test.ln k.lnk" which is considered a .lnk extension, bypassing the filtering.

(1) Enable "Always ask you where to save files".
(2) Test the download by clicking the link using https://deluxe-remarkable-cacao.glitch.me/

Comment 1

[haxatron1]

Attachment: download.html

Comment 2

[haxatron1]

Attachment: example.txt

Comment 3

[Neil Deakin]

This seems to be the case that Windows IFileDialog.setDefaultExtension collapses any spaces that it encounters in the extension.

We could either just replace the spaces with underscores or somesuch just before calling setDefaultExtension. Alternatively we could modify SanitizeExtension to replace illegal characters in the extension with underscores instead of spaces.

Comment 4

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #3)

Alternatively we could modify SanitizeExtension to replace illegal characters in the extension with underscores instead of spaces.

This seems reasonable to me. Neil, do you have time to write this patch?

Comment 5

[Neil Deakin]

I looked into this in more detail and the issue is actually because of the line at https://searchfox.org/mozilla-central/rev/ddd233d195a5e4b1c75a972a7fd322947fa1d5e3/widget/windows/nsFilePicker.cpp#461 in AppendFilter() which strips out whitespace from the file extension. This was added by 715129 to handle an unrelated fix, so I am not sure why the whitespace is being removed.

If I remove the whitespace stripping, the extension on the default filename is correct, however it is duplicated. (test.ln k becomes test.ln k.ln k). I could add extra code to remove this duplication although this might be an edge case not worth handling.

Comment 6

[:Gijs (he/him)]

Neil, did your other patch perhaps fix this? Or if not, are you planning a separate patch here?

Comment 7

[Neil Deakin]

Yes, this was fixed by the other bug.

Comment 8

[Daniel Veditz [:dveditz]]

Which other bug? bug 1815062?

Comment 9

[Neil Deakin]

Bug 1891234