1746139 - (CVE-2022-46874) Thunderbird Windows - Drag&Drop limited to 128 chars, allowing to change the file extension on drop

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Matthias Zoellner]

Attachment: Thunderbird_DragAndDropoutput.gif

If a long file name is drag and dropped from an email, thunderbird under windows is cutting the filename at 128 chars.
Thunderbird Version: 91.4.0 (64-Bit) under Windows 10 64bit
This allows an attacker to craft a file like:

totally_not_malicious_file_with_more_then_128_characters_definetly_no_problem_here_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.exe.pdf

Which seems to the user to be a PDF file, but after the drop is cut to .exe and therefore executable under windows. This might result in accidential execution of code by the user.
Attached there is a gif showing the process.

As under Windows there are some filetypes, like the ".SearchConnector-ms" (https://filesec.io/searchConnector-ms) which triggers automatic actions, this might have a bigger impact.

Comment 1

[Matthias Zoellner]

I just wanted to check, if this is considered as security risk or not? Or if there is any information missing.
Maybe not completly clear from the attached gif, but this of course also works when receiving an E-mail with the long attachement name.

Please let me know, if there is any information missing and if this is going to get fixed.
BR

Comment 2

[Daniel Veditz [:dveditz]]

Unfortunately Thunderbird is not part of the Mozilla security bug bounty program.

This could raise a security problem for some users, but IMHO (I'm not on the Thunderbird team) it seems relatively minor. Do people often drag attachments? I always click the "save" context menu button.

This is exacerbated by the fact that by default windows hides extensions (damn stupid thing to do!) so the user has even less chance of noticing the bad extension. I was thinking "sec-low" based on the difficulty in carrying out the attack (social engineer people to drag?) Could maybe argue for sec-moderate based on the severity of the consequence

Comment 3

[Kai Engert [:KaiE:]]

The definition for the 128 char limit seems to be here:
https://searchfox.org/mozilla-central/rev/682e5a82d403974bacb779c1db515962d946be48/widget/windows/nsDataObj.cpp#476

Comment 4

[Kai Engert [:KaiE:]]

Assigning to Widget Win32, to get attention from the right people.
Although this has been reported for Thunderbird, I'm guessing there might be drag&drop scenarios in Firefox with similar effects.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:spohl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Stephen A Pohl [:spohl]]

We have an internal need for more good-first-bugs, so adding to that list.

(In reply to Kai Engert (:KaiE:) from comment #3)

The definition for the 128 char limit seems to be here:
https://searchfox.org/mozilla-central/rev/682e5a82d403974bacb779c1db515962d946be48/widget/windows/nsDataObj.cpp#476

This comment references Windows versions prior to XP. We should investigate if this path length limitation can safely be removed.

Comment 7

[Matthias Zoellner]

Just wanted to ask, if there is a plan to change the behaviour. I would like to write a small blogpost about this, if there are no concerns?

Comment 8

[Stephen A Pohl [:spohl]]

n-i :dveditz regarding comment 7.

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to Stephen A Pohl [:spohl] from comment #6)

We have an internal need for more good-first-bugs, so adding to that list.

(In reply to Kai Engert (:KaiE:) from comment #3)

The definition for the 128 char limit seems to be here:
https://searchfox.org/mozilla-central/rev/682e5a82d403974bacb779c1db515962d946be48/widget/windows/nsDataObj.cpp#476

This comment references Windows versions prior to XP. We should investigate if this path length limitation can safely be removed.

It might no longer be 128, but there is surely still a limit. And therefore still a potential problem if the premise of this bug report is accepted - but no one has yet done. Ref https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN https://stackoverflow.com/questions/265769/maximum-filename-length-in-ntfs-windows-xp-and-windows-vista

cc: masayuki, who was involved with bug 250392

Comment 10

[Thomas D. (:thomas8)]

Just my 2 cents, the code which trims file names to whichever maximum length is acceptable to the OS should certainly never just chop off the right side, but always leave at least the last 4 characters (original file extension) intact and instead chop characters before that, which would easily fix this bug.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Wow, it's really old bug! Yes, if we should keep the length of the limit, first, we should parse the file name to split extension and file name. Then, cut the file name if too long, and append the extension with ..

Comment 12

[Daniel Veditz [:dveditz]]

Thomas is correct in comment 10 about how to handle truncation. We should be able to use MAX_PATH now (MAX_PATH - 1 ? watch out for buffer overflows and leave room for the terminating null), but whether we change it or not, there will still be a point where we have to truncate. Watch out for pathological cases where the extension itself might be super long, maybe even longer than the limit. Could always just slap something innocuous like .txt on the end if we have to truncate and the actual extension is longer than appears reasonable.

I don't know how to answer comment 7. Seems like more of a question for Thunderbird folks since their users are the ones potentially put at risk.

Comment 13

[Magnus Melin [:mkmelin]]

Looks like Thunderbird should fix the file name as needed here: https://searchfox.org/comm-central/rev/9be7ae36d358df58b0f29b9a5033e0c3e61074da/mail/base/content/mailCore.js#980
Nicolai, can you take this?

Comment 14

[Mirko Brodesser (:mbrodesser-Igalia)]

Be aware this should affect Firefox too, see bug 1746151#c11.

Comment 15

[Mirko Brodesser (:mbrodesser-Igalia)]

As mentioned above, #c10 seems the right way forward.

Comment 16

[u695164]

Some intresting parts from the callstack:

nsDataObj::GetDownloadDetails

In our case this return null as kFilePromiseDestFilename is not set in our DnD events. The aFilename is derived
succesfully over a length of 128 chars here from the source URL (in the case for this method its srcFileName).

Renaming the source URL would mean that we'd have to do a copy of the file and then overwrite the file url in our Drag and Drop event.

---

The mistake happens here

nsDataObj::GetFileDescriptor_IStreamW

Here the destination file name gets truncated at 128 length accordingly to NS_MAX_FILEDESCRIPTOR.

---

I see two solutions from here.

• Set kFilePromiseDestFilename for the events.
  A working example (I tried it out) could look like this:

  let name = attachment.name || attachment.displayName;
  
  if (name.length > 128) {
     let lastPieces = name.substr(-10);
     let newName = name.substr(0, 117) + "…" + lastPieces;
     dataTransfer.setData("application/x-moz-file-promise-dest-filename", newName)
  
  }
  

• The wcsncpy call could be changed.

I'm open for suggestions. Maybe I'd need access to bug 1746151#c11 to see if a change for the wcsncpy call is planned.

---

Some other information:

nsFlavorDataProvider.getFlavorData only gets called with a lazy dataprovider (e.g. cloudkey).

The attachment name in the UI is already visually dotted in the middle so that you are able to see the file ending.

Comment 17

[u695164]

Attachment: Bug 1746139 - Keeping filename ending intact when using windows with events like drag and drop. r=dveditz

Comment 18

[u695164]

Hey, I'm having troubles debugging getFlavourData.
For me it seems to be code that gets invoked when an user uses the FileLink support (lazy dataprovider).

Here it says it's "dataless".
#define kFilePromiseMime

It should do something but i couldn't figure out what under Windows 10 with trunk daily tb and m-c.

My current understanding is, that it is used when drag and drop an attachement sent via FileLink. See FileLink attachement for getFlavourData

One more thing:
It is definitely in the dataTransfer object, saved as an [nsXPTCStubBase] object.

Any ideas?

Comment 19

[u695164]

Attachment: FileLink attachement for getFlavourData

Comment 20

[Magnus Melin [:mkmelin]]

I think it's used when dragging an attachment (any attachment) out of Thunderbird, e.g. to the desktop

Comment 21

[u695164]

(In reply to Magnus Melin [:mkmelin] from comment #20)

I think it's used when dragging an attachment (any attachment) out of Thunderbird, e.g. to the desktop

I've set mailCore.js::messenger.saveAttachmentToFolder 3rd arg to "foo.pdf" and added a breakpoint at nsMessenger::SaveAttachmentToFolder. This method is not called in the drag and drop operation from the file system to Thunderbird or the other way around from Thunderbird to file system under Win10.

The filename wasn't set to "foo.pdf" and the breakpoint wasn't hit.
I'd really like to see where this applies before making changes.

(Well a build wihout the nsFlavorDataProvider deleted had the same behaviour. No error and the attachment was succefully drag and dropped to the filesystem.)

Just figured out that it does something. It makes it possible to get attachement from e.g. an imap source. But you can't touch the name here seems to be some safety from somewhere.

Comment 22

[u695164]

Attachment: Bug 1746139 - Sanitize file names from untrusted sources. r=mkmelin

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 24

[Matthias Zoellner]

Is here still any progress? Or will this stay like it is?
I would still like to write a mini Blogpost about this.
Let me know, if there are any concerns.
BR

Comment 25

[Stephen A Pohl [:spohl]]

n-i for comment 24

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D141941	Bug 1746139 - Keeping filename ending intact when using windows with events like drag and drop. r=dveditz	nicolai	dveditz: Resigned from review

:nicolai, could you please find another reviewer?

For more information, please visit auto_nag documentation.

Comment 27

[Matthias Zoellner]

Attachment: Thunderbird_draganddrop-2022-07-20_07.42.42.gif

Added a new PoC Version.

Comment 28

[Matthias Zoellner]

Attachment: Thunderbird_tldr-2022-07-19_21.16.46_redacted.gif

And as the Blogpost is on it's way a new, more complete PoC.

Comment 29

[Matthias Zoellner]

Just for your information: You can find the blogpost here: https://blog.syss.com/posts/tampering-with-thunderbird-attachements/

Comment 30

[Thomas D. (:thomas8)]

Chris, could you review D141941 (already reviewed by :dveditz) or connect another reviewer?

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:spohl, since the bug has high priority and recent activity, could you please find another way to get the information or close the bug as INCOMPLETE if it is not actionable?

For more information, please visit auto_nag documentation.

Comment 32

[Chris Martin [:cmartin]]

LGTM on the MAX_PATH extension. I guess it's up to the other patch to actually prevent the exploit though, since AFAICT this will only make it so the attacker needs to craft a longer file name to cause the same behavior?

(Not that I think it affects anything, but it's worth noting that MAX_PATH is not always the maximum path length on Windows -- Windows 10 can have this limit removed in the registry, and thus many Windows API functions would still allow it. And also windows allows the "\?\really\really\really\long\path" extended path syntax that can be up to 32,767 chars long. (link))

Comment 33

[Alessandro Castellani [:aleca]]

Magnus, would you be able to take this leftover and finish it?

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:spohl, since the bug has high priority and recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 35

[Magnus Melin [:mkmelin]]

Attachment: Bug 1746139 - Only allow filenames to be up to 85 ch. r=aleca

The common file name length limit is 255. On windows 255 chars, on linux 255 bytes.
In UTF-8 one char can take up up to 4 bytes, but 3 covers all normal cases.
Use a limit of 255/3 = 85 chars for file names to be safe. That will also make for more reasonable file names in general usage.

Filenames longer than 85 will turned into first 74 chars … 10 last chars.

Comment 36

[Magnus Melin [:mkmelin]]

Let's get this landed. Lando isn't letting me push it for some reason. dveditz, can you land D141941?
The information is already public since months (comment 29).

Comment 37

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/120942422c67529a29ed3a563c6b4052705295d2

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Keeping filename ending intact when using windows with events like drag and drop. r=cmartin
https://hg.mozilla.org/integration/autoland/rev/5984cee8a1c5688a06370fbcf2e5481458e12dfa
https://hg.mozilla.org/mozilla-central/rev/5984cee8a1c5

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Hi Magnus, did you want to nominate this for ESR102 uplift?

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 9269188 [details]
Bug 1746139 - Keeping filename ending intact when using windows with events like drag and drop. r=dveditz

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Moderate security issue
• User impact if declined: Long filenames can trick the user to executing a file they didn't expect to be executing.
• Fix Landed on Version: 108
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is fairly confined.

Comment 41

[Dianna Smith [:diannaS]]

Comment on attachment 9269188 [details]
Bug 1746139 - Keeping filename ending intact when using windows with events like drag and drop. r=dveditz

Approved for 102.6esr

Comment 42

[Dianna Smith [:diannaS]]

Comment on attachment 9269188 [details]
Bug 1746139 - Keeping filename ending intact when using windows with events like drag and drop. r=dveditz

https://hg.mozilla.org/releases/mozilla-esr102/rev/9d361299a3ab

Comment 43

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 44

[Rob Lemley [:rjl]]

Comment on attachment 9297533 [details]
Bug 1746139 - Only allow filenames to be up to 85 ch. r=aleca

This needs to be uplifted to comm-esr102 since the other patch was uplifted to Firefox 102.6.0.
approval-comm-esr102+ a=rjl

Comment 45

[Rob Lemley [:rjl]]

Thunderbird 102.6.1:
https://hg.mozilla.org/releases/comm-esr102/rev/52c4495b3fb11e60c9c94ef148dd8563151df385