Closed Bug 1746139 (CVE-2022-46874) Opened 3 years ago Closed 2 years ago

Thunderbird Windows - Drag&Drop limited to 128 chars, allowing to change the file extension on drop

Categories

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

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
108 Branch
Tracking Status
thunderbird_esr91 --- wontfix
thunderbird_esr102 + fixed
firefox-esr102 108+ fixed
firefox106 --- wontfix
firefox107 --- wontfix
firefox108 + fixed

People

(Reporter: pfiatde, Assigned: mkmelin)

References

(Regressed 1 open bug)

Details

(4 keywords, Whiteboard: [reporter-external] [client-bounty-form] [verif?][win:security][post-critsmash-triage][adv-main108+][adv-esr102.6+])

Attachments

(6 files, 2 obsolete files)

Attached image Thunderbird_DragAndDropoutput.gif (obsolete) —

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.

Flags: sec-bounty?
Group: firefox-core-security → mail-core-security
OS: Unspecified → Windows 10
Product: Firefox → Thunderbird
Version: unspecified → Thunderbird 91
Type: task → defect

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

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty? → sec-bounty-

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.

Group: mail-core-security → core-security
Component: Security → Widget: Win32
Product: Thunderbird → Core
Version: Thunderbird 91 → unspecified

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(spohl.mozilla.bugs)

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.

Severity: -- → S3
Flags: needinfo?(spohl.mozilla.bugs)
Keywords: good-first-bug
Priority: -- → P3
Group: core-security → dom-core-security

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?

n-i :dveditz regarding comment 7.

Flags: needinfo?(dveditz)

(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

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.

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 ..

Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][win:security]

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.

Flags: needinfo?(dveditz) → needinfo?(mkmelin+mozilla)

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?

Assignee: nobody → nicolai
Flags: needinfo?(mkmelin+mozilla)
Status: NEW → ASSIGNED

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

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

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.

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?

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

(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.

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

Assignee: nicolai → nobody
Status: ASSIGNED → NEW

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

n-i for comment 24

Flags: needinfo?(nicolai)

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.

Flags: needinfo?(nicolai)
Attachment #9255446 - Attachment is obsolete: true

Added a new PoC Version.

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

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

Assignee: nobody → nicolai
Status: NEW → ASSIGNED
Keywords: good-first-bug
Priority: P3 → P2

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

Flags: needinfo?(cmartin)

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.

Flags: needinfo?(nicolai) → needinfo?(spohl.mozilla.bugs)

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))

Flags: needinfo?(cmartin)
Flags: needinfo?(spohl.mozilla.bugs)

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

Flags: needinfo?(mkmelin+mozilla)
Assignee: u695164 → mkmelin+mozilla
Flags: needinfo?(mkmelin+mozilla)

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.

Flags: needinfo?(u695164) → needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9269426 - Attachment is obsolete: true

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.

Attachment #9297533 - Attachment description: Bug 1746139 - Only allow filenames to be up to 255 ch. r=aleca → Bug 1746139 - Only allow filenames to be up to 85 ch. r=aleca
Target Milestone: --- → 107 Branch

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).

Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Target Milestone: 107 Branch → 108 Branch
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

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

Flags: needinfo?(mkmelin+mozilla)

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.
Flags: needinfo?(mkmelin+mozilla)
Attachment #9269188 - Flags: approval-mozilla-esr102?

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 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

Attachment #9269188 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Flags: qe-verify-
Whiteboard: [reporter-external] [client-bounty-form] [verif?][win:security] → [reporter-external] [client-bounty-form] [verif?][win:security][post-critsmash-triage]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][win:security][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][win:security][post-critsmash-triage][adv-main108+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][win:security][post-critsmash-triage][adv-main108+] → [reporter-external] [client-bounty-form] [verif?][win:security][post-critsmash-triage][adv-main108+][adv-esr102.6+]
Alias: CVE-2022-46874

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

Regressions: 1806730
Flags: sec-bounty-hof+
Regressions: 1829981
Group: core-security-release
See Also: → 1882263
Regressions: 1906455
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: