1811852 - (CVE-2023-25738) Firefox crashes on Windows when printing a page (sometimes)

Status: RESOLVED FIXED

(Toolkit :: Printing, defect)

Description

[Mark]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/109.0

Steps to reproduce:

Visit a long URL, such as: https://crm1.mozilla.com/survey.php?firstname=Lisa&lastname=Smith&assignedto=Town&postcode=NE1%202AE&telephonehome=07777754&email=[e-mail]&address1=13%20Front%20Street&city=Newcastle&source=Search%20Engine%20(Google,%20Yahoo%20etc)&surveydate=2023-01-25T14:00

(I've used fake data above, but can provide a real URL if you need).

Then click print, print dialogue box shows up, then firefox crashes.

Actual results:

Browser crashes fully. All windows and tabs close.

Expected results:

Browser should print the page.

Comment 1

[Andrew McCreight [:mccr8]]

That didn't crash for me on Mac OS, but I wouldn't be surprised if the print preview code is different on different OSes.

Are you on Windows? Could you please go to about:crashes and post the crash-stats.mozilla.org here? Thanks. This probably isn't a security issue, which maybe we can tell from the crash report.

Comment 2

[Mark]

Hi the report is here: https://crash-stats.mozilla.org/report/index/082e9494-99c5-4752-b225-edcb90230123#tab-details

Comment 3

[Mark]

(In reply to Andrew McCreight [:mccr8] from comment #1)

That didn't crash for me on Mac OS, but I wouldn't be surprised if the print preview code is different on different OSes.

Are you on Windows? Could you please go to about:crashes and post the crash-stats.mozilla.org here? Thanks. This probably isn't a security issue, which maybe we can tell from the crash report.

Yes on Windows 10, latest updates.

This has happened on multiple versions of Firefox from as far back as 2 years ago.

I can't see crash stats, but I can post the report here: https://crash-stats.mozilla.org/report/index/082e9494-99c5-4752-b225-edcb90230123#tab-details

Comment 4

[Andrew McCreight [:mccr8]]

Thanks.

Wow, that's not what I was expecting at all. We're crashing inside TraceProtoAndIfaceCache. Very weird.

Comment 5

[Mark]

(In reply to Andrew McCreight [:mccr8] from comment #4)

Thanks.

Wow, that's not what I was expecting at all. We're crashing inside TraceProtoAndIfaceCache. Very weird.

Indeed! An interesting one. Is it the url length or special characters maybe?

Comment 6

[Andrew McCreight [:mccr8]]

Bob, any chance you could take a look? I moved this to DOM bindings, but I guess it is still most likely the issue originates in printing somehow. Thanks.

Comment 7

[Andrew McCreight [:mccr8]]

Can you reproduce this issue without any extensions enabled? The URL there isn't all that long in the grand scheme of things, and this crash doesn't appear to be very common, so we want to figure out if there's something unique about your setup that might cause this issue so we can fix it. Thanks.

Comment 8

[Andrew McCreight [:mccr8]]

I guess I'll move this back to printing for now. Sorry about my indecisiveness.

Comment 9

[Bob Owen (:bobowen)]

(In reply to Andrew McCreight [:mccr8] from comment #6)

Bob, any chance you could take a look? I moved this to DOM bindings, but I guess it is still most likely the issue originates in printing somehow. Thanks.

I'm not really sure I can help much at the moment.
While it appears to be triggered by printing, there isn't even any printing specific code on the stack when it crashes.
We could probably do with someone who understands the interactions of GC and the tracing, to work out what type of thing might be causing this.
Even then given where it is crashing, I guess the issue will be in JS code rather than the native printing code.

In addition, I don't seem to be able to reproduce.

Comment 10

[Mark]

(In reply to Bob Owen (:bobowen) from comment #9)

(In reply to Andrew McCreight [:mccr8] from comment #6)

Bob, any chance you could take a look? I moved this to DOM bindings, but I guess it is still most likely the issue originates in printing somehow. Thanks.

I'm not really sure I can help much at the moment.
While it appears to be triggered by printing, there isn't even any printing specific code on the stack when it crashes.
We could probably do with someone who understands the interactions of GC and the tracing, to work out what type of thing might be causing this.
Even then given where it is crashing, I guess the issue will be in JS code rather than the native printing code.

In addition, I don't seem to be able to reproduce.

For what it's worth. The page I'm trying to print is a flat HTML document. No javascript on the page at all (I presume you mean JS in firefox and not the page being printed).

How do I start FF with all plugins disabled?

Comment 11

[Andrew McCreight [:mccr8]]

This page describes how to start in Troubleshooting mode, which will hopefully will do the trick.

Comment 12

[Mark]

(In reply to Andrew McCreight [:mccr8] from comment #11)

This page describes how to start in Troubleshooting mode, which will hopefully will do the trick.

Hi all, crashed again in troubleshooting mode with all plugins disabled: https://crash-stats.mozilla.org/report/index/f006003a-94fc-458d-8498-176b70230124

Comment 13

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

The bug has a crash signature, thus the bug will be considered confirmed.

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Mark from comment #12)

Hi all, crashed again in troubleshooting mode with all plugins disabled: https://crash-stats.mozilla.org/report/index/f006003a-94fc-458d-8498-176b70230124

Thanks. The stack in that crash is actually printing code so that makes a little more sense.

Comment 15

[Andrew McCreight [:mccr8]]

One of the crashes with that signature has the comment: "firefox crashes when I use my default printer. It pretended to print once (nothing is in the print queue.) I got it to use another printer, but when I switched back to default it crashed after the print preview. I restarted and it crashed when I hit ctrl+p. I didn't even get a preview. This has been happening for a couple days now."

Maybe flipping the printer back and forth got it into a broken state or something?

Comment 16

[Daniel Veditz [:dveditz]]

I can't see crash stats, but I can post the report here

That's extremely odd—that's a public URL and should load just fine. Are you on a network with content filtering that for some reason considers that a "bad site"? Do you get any kind of error when you try to load that URL? Or does it just act like that site doesn't exist?

Comment 17

[Daniel Veditz [:dveditz]]

The mozilla::dom::ProtoAndIfaceCache::PageTableCache::Trace crash has happened at a low level for a long time on both linux, android, and windows (32 crashes in 6 months). There are no comments on any of these crashes.

The memcpy | nsPrintSettingsWin::CopyDevMode crash is higher volume, only on Windows, and started with Firefox 106 (95 crashes in 3 months). Lots of user comments about printing. CopyDevMode is inlined (which crash-stats didn't handle in the past) and called by SetDevMode so I looked at that to see about signature changes. Crashes at [@ memcpy | nsPrintSettingsWin::SetDevMode ] affect Firefox 105 but not 106 which bolsters the argument for signature drift. But it can't be simply the crash-stats inline change because lower versions (especially Firefox 102 ESR releases) continue to crash at the same SetDevMode line, and I would have expected a mix of 105 and 106 versions in both signatures on either side of the change-over date.

At least one of the comments said it happened once during print-preview, and these days it looks like pages can trigger that directly with window.print(). The print function itself is old, but it used to require an additional step from the user to get to the preview. More convenient (less clicking) for the user, but does mean this code is reachable if this crash turns out to be a vulnerability.

The memcpy() part of the signature is certainly scary. All the crashes are READs so I think we calculated a size that was too large and read off the end of the source data. We only get here if we successfully allocated the too-large destination buffer so that doesn't look exploitable if I'm correct. I didn't dig too deeply, but it looks like the two values we use to calculate the size come from a Windows API structure. Did we forget to check for error responses at some earlier point and are using bogus/uninitialized values? Is there anything more dangerous we could end up doing with those values that would be even more dangerous if we managed to avoid crashing at this spot?

Comment 18

[Daniel Veditz [:dveditz]]

I think the length of the URL is a red herring. most of the ones I could see in crash-stats (non-public data) were a reasonable length.

Comment 19

[Mark]

(In reply to Daniel Veditz [:dveditz] from comment #16)

I can't see crash stats, but I can post the report here

That's extremely odd—that's a public URL and should load just fine. Are you on a network with content filtering that for some reason considers that a "bad site"? Do you get any kind of error when you try to load that URL? Or does it just act like that site doesn't exist?

No content filtering. Just a standard site and standard internet connection. The site exists and is always online.

Comment 20

[Mark]

(In reply to Daniel Veditz [:dveditz] from comment #18)

I think the length of the URL is a red herring. most of the ones I could see in crash-stats (non-public data) were a reasonable length.

The reason I mentioned this, is that if I remove the string after the URL, the page prints without error.

Could it be an encoding issue or mismatch from FF to the printspooler? (sorry, not an expert in this area, just hypothesising).

Comment 21

[Bob Owen (:bobowen)]

OK that latest crash report makes much more sense, I'll take a look.

Comment 22

[Bob Owen (:bobowen)]

Hi Mark, would you let me know what printer and if possible printer driver you are using?

Comment 23

[Bob Owen (:bobowen)]

It looks like aInDevMode->dmDriverExtra is either wrong or garbage here.

So, we need to be checking that here.
Although I think we could safely set dmDriverExtra to zero and just copy the standard DEVMODEW data.

We do allocate twice as much as it tells us for the DEVMODEW, because of similar code in chrome, but clearly if dmDriverExtra is garbage that's not going to help us here.

Hmm, I wonder if a driver is adding on to dmDriverExtra (and maybe dmSize) instead of setting it.
So, we should probably set the array held by devmodeStorageWLock to zeros before we call ::DocumentPropertiesW.

It would be nice if I could reproduce with a specific driver.
Either way these would be sound changes.

Comment 24

[Mark]

(In reply to Bob Owen (:bobowen) from comment #22)

Hi Mark, would you let me know what printer and if possible printer driver you are using?

It's a Xerox VersaLink C405 latest driver, but the crash has happened for at least 2 years. I don't know if Xerox have updated their driver in that time.

Comment 25

[Mark]

(In reply to Bob Owen (:bobowen) from comment #23)

It looks like aInDevMode->dmDriverExtra is either wrong or garbage here.

We do allocate twice as much as it tells us for the DEVMODEW, because of similar code in chrome, but clearly if dmDriverExtra is garbage that's not going to help us here.

Oddly enough, my work around was to copy the link, paste it in to chrome and print from there, as it printed without issue in chrome.

Comment 26

[Mark]

Just an update, I installed the generic PPD driver from here: https://www.support.xerox.com/en-us/product/versalink-c405/downloads?platform=win10&category=drivers&language=en_GB&attributeId=

Generic PPD

The driver package contains the Generic PPD to be installed using the Windows Add Printer Wizard.

    Released: 04/16/2019
    Version: 5.662.0.0
    Size: 455.50 KB
    Filename: VersaLink_C400_5.662.0.0_PPD_English.exe
    Tags: PPD, V3 Driver

During the install I added the printer via IP address in the setup. It's now installed as an "MS Publisher Color Printer".

It prints without any crashes now.

So you may be right in it being a driver issue.

Comment 27

[Bob Owen (:bobowen)]

Thanks, I couldn't reproduce with the Xerox VersaLink C405 driver, but I'm pretty sure I've worked out what is going on.
We do check the return from ::DocumentPropertiesW, but we don't clear the array in mDefaultDevmodeWStorage, so next time round we think we've already read the details and use the uninitialized data in mDefaultDevmodeWStorage.

Patch incoming.

Comment 28

[Bob Owen (:bobowen)]

Attachment: Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio!

Comment 29

[Mark]

(In reply to Bob Owen (:bobowen) from comment #27)

Thanks, I couldn't reproduce with the Xerox VersaLink C405 driver, but I'm pretty sure I've worked out what is going on.
We do check the return from ::DocumentPropertiesW, but we don't clear the array in mDefaultDevmodeWStorage, so next time round we think we've already read the details and use the uninitialized data in mDefaultDevmodeWStorage.

Patch incoming.

Awesome to see this bug get squished live! Thanks everyone :)

Comment 30

[Bob Owen (:bobowen)]

Comment on attachment 9314548 [details]
Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The issue is fairly obvious from the patch, but we don't know of a way to exploit it.
  In addition to trigger the issue would require a printer driver to return an error while retrieving the DEVMODEW.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Pretty unlikely, if printer drivers were returning incorrect sizes they'd probably already be causing problems.
• Is Android affected?: No

Comment 31

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9314548 [details]
Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio!

Approved to land if you and relman are okay uplifting this late in cycle.

Comment 32

[Andrew McCreight [:mccr8]]

I searched for Windows crashes with InitWithInitializer in the proto signature, and I found two other signatures which look like the same issue, and they are much more common.

Comment 33

[Bob Owen (:bobowen)]

Comment on attachment 9314548 [details]
Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio!

Beta/Release Uplift Approval Request

• User impact if declined: Intermittent browser crashes when printing.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple new size checks and clearing of a structure on failure to prevent subsequent re-use.
  Obviously, It would be a good idea to include some manual printing regression tests if those are not normally part of the Beta and RC testing.
• String changes made/needed: None
• Is Android affected?: No

Comment 34

[Pascal Chevrel:pascalc (PTO until April 26)]

Note that ESR is affected so we would need a request for this branch as well (and make sure that the patch does apply to this older codebase).
Thanks.

Comment 35

[Bob Owen (:bobowen)]

(In reply to Pascal Chevrel:pascalc from comment #34)

Note that ESR is affected so we would need a request for this branch as well (and make sure that the patch does apply to this older codebase).
Thanks.

Coming up, just didn't think it was worth it if we weren't going to take it. :-)
It applies to all branches.

Comment 36

[Bob Owen (:bobowen)]

Comment on attachment 9314548 [details]
Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: Intermittent browser crashes when printing.
• Fix Landed on Version: 111
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple new size checks and clearing of a structure on failure to prevent subsequent re-use.
  Obviously, It would be a good idea to include some manual printing regression tests.

Comment 37

[Pascal Chevrel:pascalc (PTO until April 26)]

Bob, feel free to land, I'll uplift in out last beta that builds tomorrow.

Comment 38

[Bob Owen (:bobowen)]

(In reply to Pascal Chevrel:pascalc from comment #37)

Bob, feel free to land, I'll uplift in out last beta that builds tomorrow.

OK thanks.

Comment 39

[Bob Owen (:bobowen)]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55ccf3adadb5c4b4bd679d366afbe090410d5d37
Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio

Comment 40

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

https://hg.mozilla.org/mozilla-central/rev/55ccf3adadb5

Comment 41

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9314548 [details]
Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio!

Approved for our last 110 beta, thanks.

Comment 42

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/581bd69d544a

Comment 43

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9314548 [details]
Bug 1811852: Clear default DEVMODEW storage on failure, so that we try again next time. r=emilio!

Approved for ESR 102.8.0, thanks.

Comment 44

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-esr102/rev/e74b2a8d649e

Comment 45

[Catalin Sasca, Desktop QA [:csasca]]

We tried reproducing the crash but we were unable to do so on two different Windows 10 systems and two different printers set as default (HP OfficeJet 6950 and HP DeskJet 3700) with the provided link in Comment 0 (seems like it isn't working anymore and may be why we are unable to reproduce it).

Mark, can you please look over the builds where the fix is implemented and see that the issue is no longer reproducible ? Here's the link for Nightly 111.0a1, for Beta 110.0b9 and for ESR102. Thank you!

Comment 46

[Mark]

(In reply to Catalin Sasca, QA [:csasca] from comment #45)

We tried reproducing the crash but we were unable to do so on two different Windows 10 systems and two different printers set as default (HP OfficeJet 6950 and HP DeskJet 3700) with the provided link in Comment 0 (seems like it isn't working anymore and may be why we are unable to reproduce it).

Mark, can you please look over the builds where the fix is implemented and see that the issue is no longer reproducible ? Here's the link for Nightly 111.0a1, for Beta 110.0b9 and for ESR102. Thank you!

I can't I'm afraid. I've uninstalled that printer and installed a generic driver under "MS Publisher Color Printer" as I needed to actually print!

Do you think it was the driver that was bugged and causing the crash?

Comment 47

[Bob Owen (:bobowen)]

(In reply to Mark from comment #46)
...

Do you think it was the driver that was bugged and causing the crash?

It wasn't necessarily that the driver had a bug, but if the call to get the settings failed then it triggered the issue in our code.
It's possible that the generic driver is less likely to fail, because it doesn't communicate with the printer.
I imagine it will mean that some device specific settings on the printer may not work.
That would be more likely to affect the system dialog for the printer than our print preview I would guess.

Comment 48

[Mark]

(In reply to Bob Owen (:bobowen) from comment #47)

(In reply to Mark from comment #46)
...
That would be more likely to affect the system dialog for the printer than our print preview I would guess.

Hmm. I could print to PDFs and other devices that show the print preview dialogue. It was just when selecting that installed printer, it would flash up for half a second then full browser crash.

Comment 49

[Bob Owen (:bobowen)]

(In reply to Mark from comment #48)

(In reply to Bob Owen (:bobowen) from comment #47)

(In reply to Mark from comment #46)
...
That would be more likely to affect the system dialog for the printer than our print preview I would guess.

Hmm. I could print to PDFs and other devices that show the print preview dialogue. It was just when selecting that installed printer, it would flash up for half a second then full browser crash.

Sorry, I think I made that comment confusing.
Each printer installed (including the ones that print to file) has a printer driver associated with it and it is possible that the device specific one for your Xerox VersaLink C405 communicates with the printer while retrieving its settings and sometimes that can fail, which triggers this bug in Firefox code.
By installing the generic driver it might mean that the communication doesn't happen, because it no longer communicates with the printer and only uses the standard settings structure.

I was just pointing out that having the generic driver installed might mean that the system print dialog is missing some device specific settings, because the device specific driver might normally interact with that dialog to show extra settings.
I don't think it would affect the Firefox Print Preview, because we only use the standard print settings structure that all devices have to return.

Hopefully, once the next version of Firefox rolls out next Tuesday, you will be able to reinstall the device specific driver.
The fix is already on Beta, Nightly and Extended Support Release.
If you were able to test this it would be great.
The Beta build by default installs to the same location as Release Firefox, so I would select a custom install and change the folder/directory to "Mozilla Firefox Beta".
When you launch it, it should create a new separate profile for that installation.

Comment 50

[Mark]

(In reply to Bob Owen (:bobowen) from comment #49)

(In reply to Mark from comment #48)

(In reply to Bob Owen (:bobowen) from comment #47)

(In reply to Mark from comment #46)
...
That would be more likely to affect the system dialog for the printer than our print preview I would guess.

Hmm. I could print to PDFs and other devices that show the print preview dialogue. It was just when selecting that installed printer, it would flash up for half a second then full browser crash.

Sorry, I think I made that comment confusing.
Each printer installed (including the ones that print to file) has a printer driver associated with it and it is possible that the device specific one for your Xerox VersaLink C405 communicates with the printer while retrieving its settings and sometimes that can fail, which triggers this bug in Firefox code.
By installing the generic driver it might mean that the communication doesn't happen, because it no longer communicates with the printer and only uses the standard settings structure.

A was just pointing out that having the generic driver installed might mean that the system print dialog is missing some device specific settings, because the device specific driver might normally interact with that dialog to show extra settings.
I don't think it would affect the Firefox Print Preview, because we only use the standard print settings structure that all devices have to return.

Hopefully, once the next version of Firefox rolls out next Tuesday, you will be able to reinstall the device specific driver.
The fix is already on Beta, Nightly and Extended Support Release.
If you were able to test this it would be great.
The Beta build by default installs to the same location as Release Firefox, so I would select a custom install and change the folder/directory to "Mozilla Firefox Beta".
When you launch it, it should create a new separate profile for that installation.

Ah ok, will check Tuesday when it's live. Thanks for your help!

Comment 51

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt