Closed Bug 1577061 Opened 2 years ago Closed 2 years ago

GetCurrentWorkingDir in nsUpdateDriver may blow the stack

Categories

(Toolkit :: Application Update, defect, P1)

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 70+ fixed
firefox68 --- wontfix
firefox69 - wontfix
firefox70 + fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

(Regression)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70+r][adv-esr68.2+][adv-esr68.2+r])

Attachments

(1 file)

[Tracking Requested - why for this release]:
Security issue in update code.

static nsresult GetCurrentWorkingDir(char* buf, size_t size) {
  // Cannot use NS_GetSpecialDirectory because XPCOM is not yet initialized.
  // This code is duplicated from xpcom/io/SpecialSystemDirectory.cpp:

#if defined(XP_WIN)
  wchar_t wpath[MAX_PATH];
  if (!_wgetcwd(wpath, size)) {
    return NS_ERROR_FAILURE;
  }
  NS_ConvertUTF16toUTF8 path(wpath);
  strncpy(buf, path.get(), size);
#else
  if (!getcwd(buf, size)) {
    return NS_ERROR_FAILURE;
  }
#endif
  return NS_OK;
}

(This is only an issue for the Windows variant)

  • In the call to _wgetcwd, we're using size as the size of the destination buffer, but the true value that should be passed in here is ArrayLength(wpath) (in other words, MAX_PATH).
  • The caller is passing sizeof(workingDirPath) == MAXPATHLEN == 1024 in bytes, whereas _wgetcwd expects characters. So we call _wgetcwd claiming that the buffer can hold 1024 characters when in fact it can only hold 260.
  • In debug builds at least, it looks like _wgetcwd fills the remainder of the buffer with zeros, which results in the stack corruption.

I'm rather sick atm and might not be up for doing this review tomorrow. I suggest mhowell for the review and have cc'd him.

Flags: needinfo?(aklotz)

k, thanks.

Flags: needinfo?(aklotz)
Blocks: 1567219
Blocks: 1542830

Comment on attachment 9088609 [details]
Bug 1577061: Modify nsUpdateDriver's GetCurrentWorkingDir to accept a nsACString& argument; r=rstrong!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: It's pretty obvious what is going on from the contents of the patch, but this code only runs during an update, just prior to a restart. I would say that it would be fairly difficult to exploit.
  • 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?: This code is already exercised by xpcshell and I have already tested it manually. It is unlikely to cause regressions.
Attachment #9088609 - Flags: sec-approval?

Theoretically I think you could exploit this by creating a directory with a path string longer than MAX_PATH and then starting a Firefox instance with that path as its working directory, and then you'd get the stack buffer overrun the next time an update runs. I'm not sure how feasible that procedure is.

We're building RCs for Fx69 already, so I think this will need to ride with Fx70/68.2esr.

I should point out that, even in the absence of security issues, this could cause Firefox to crash while attempting to update itself. Not that this is significantly different from the 12 years that this bug has been present, but I could see it contributing to the update orphaning problem.

This missed the window to be able to be taken on Firefox 69. I'm giving sec-approval to land in the middle of the release cycle for 70 since it is an obvious fix. So, sec-approval+ for checkin on October 1, 2019.

Whiteboard: [checkin on 10/1]
Attachment #9088609 - Flags: sec-approval? → sec-approval+

When can this land on Nightly? The test failures that this bug induces are blocking other bugs.

Flags: needinfo?(abillings)

(To clarify: the bugs that are blocked by this one do not touch updater code, but for some reason they are affecting the generated binary in a way that makes the browser more susceptible to crashing due to this bug.)

After discussion, making this a sec-moderate. Go ahead and check it in now.

Flags: needinfo?(abillings)
Keywords: sec-moderate
Whiteboard: [checkin on 10/1]
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Comment on attachment 9088609 [details]
Bug 1577061: Modify nsUpdateDriver's GetCurrentWorkingDir to accept a nsACString& argument; r=rstrong!

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Firefox may crash during update, thus breaking updates.
  • User impact if declined: Automatic updates may be broken for some users when this issue is encountered.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Very simple patch
  • String or UUID changes made by this patch: None

Beta/Release Uplift Approval Request

  • User impact if declined: Requesting as a potential ride-along for 69 dot-release. Security issue, Firefox may crash during update, thus breaking updates.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • 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): Very simple patch
  • String changes made/needed: None
Attachment #9088609 - Flags: approval-mozilla-release?
Attachment #9088609 - Flags: approval-mozilla-esr68?

Comment on attachment 9088609 [details]
Bug 1577061: Modify nsUpdateDriver's GetCurrentWorkingDir to accept a nsACString& argument; r=rstrong!

Given that this has been a problem for 12 years, it doesn't seem like we need to rush this into a dot release instead of letting it ride with the next cycle.

Attachment #9088609 - Flags: approval-mozilla-release? → approval-mozilla-release-

Keep in mind that, while new users won't always hit this crash, the ones that do will be orphaned at the release that they are currently running at. I respect your decision, I just want to make sure that you are fully aware of the consequences.

(And these crashes do not show up in our crash reports)

Flags: needinfo?(ryanvm)

I know crashes in nsUpdateDriver.cpp show up in our crash reports. Why wouldn't this crash?

Flags: needinfo?(aklotz)

I'm not sure, but when I was reproducing this with builds that do have crash reporting enabled, they always went straight to WER.

Flags: needinfo?(aklotz)

The scenario in comment 5 doesn't strike me as a particularly common one and to my knowledge we haven't seen any reports over the years from users in the wild encountering this scenario (even if we didn't get crash reports for some reason, I would have expected someone to file a bug at some point if they were encountering a situation like this). I think letting this bake and uplifting to 68.2esr is still the reasonable course of action here.

Flags: needinfo?(ryanvm)
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
QA Whiteboard: [qa-triaged]

Hi Aaron, can you help us with some steps on how we can verify the fix on our end ?

Flags: needinfo?(aklotz)

This is really hard to do manually. We already have xpcshell tests to set up and exercise this code, so my suggestion would be to run ./mach xpcshell-test toolkit/mozapps/update/tests/unit_service_updater/marAppApplyUpdateAppBinInUseStageSuccessSvc_win.js against debug builds to verify.

Flags: needinfo?(aklotz)

Comment on attachment 9088609 [details]
Bug 1577061: Modify nsUpdateDriver's GetCurrentWorkingDir to accept a nsACString& argument; r=rstrong!

Approved for 68.2esr.

Attachment #9088609 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Based on comment 20, seems it is really hard to verify this manually. Running the mention automated test, it is not manual verification. Removing the qe-verify+ flag.

Flags: qe-verify+ → qe-verify-
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main70+][adv-main70-rollup]
Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70-rollup] → [post-critsmash-triage][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup]
Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup] → [post-critsmash-triage][adv-main70+][adv-main70+r][adv-esr68.2+][adv-esr68.2+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.