1816694 - Firefox is requesting SeTcbPrivilege hundreds of times a day

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Mike Kaply [:mkaply]]

We had a report from an enterprise customer that Firefox produces approximately 800 messages a day via Microsoft Windows security auditing with requests to SeTcbPrivilege. This is not creating a security problem per say, but the volume of messages (multiplied by the number of users) is overwhelming their logs.

A similar bug was reported on Chrome:

https://bugs.chromium.org/p/chromium/issues/detail?id=1038507

but based on input from the customer, the volume is much higher on Firefox.

Comment 1

[Bob Owen (:bobowen)]

I reproduced this for Firefox from the instructions in the Chrome issue.
Then I tried to correlate the events with Process Monitor logs.

I saw a small number that looked like they might have been down to COM.

The vast majority seem to be down to this call to SetNamedSecurityInfoW.
Stepped over that call in windbg to prove it is the source and also that the function is failing anyway.
Not clear why it is failing at this point it would appear that we should have the required permissions. The error is:
0xc000007c - An attempt was made to reference a token that doesn't exist. This is typically done by referencing the token associated with a thread when the thread is not impersonating a client.

It is an opt-out part of the function via SkipNtfsAclReset option, so I suspect callers might not require or expect it.

The callers I have seen for this that hit the failing SetNamedSecurityInfoW call are:

• IOUtils::CopyOrMoveSync here, which is presumably from various JS sources.
  :barret - I guess you might be best able to comment on this one
• CacheFileIOManager::DoomFileInternal here
  :valentin - perhaps you might be able to help on this one

Comment 2

[Valentin Gosu [:valentin] (he/him)]

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

I reproduced this for Firefox from the instructions in the Chrome issue.
Then I tried to correlate the events with Process Monitor logs.

I saw a small number that looked like they might have been down to COM.

The vast majority seem to be down to this call to SetNamedSecurityInfoW.

This was added in https://hg.mozilla.org/mozilla-central/rev/da65cb02909d425d21d1ee623831c8524ac86c6e
From what I understand it's so that the files have the same ACL permissions as the parent directory.

Stepped over that call in windbg to prove it is the source and also that the function is failing anyway.
Not clear why it is failing at this point it would appear that we should have the required permissions. The error is:
0xc000007c - An attempt was made to reference a token that doesn't exist. This is typically done by referencing the token associated with a thread when the thread is not impersonating a client.

That's interesting. For me I don't see this call failing, but something still logs a Sensitive Priviledge use to the Event Vewer:

A privileged service was called.

Service:
	Server:	Security
	Service Name:	-

Process:
	Process ID:	0x830
	Process Name:	C:\Program Files\Firefox Nightly\firefox.exe

Service Request Information:
	Privileges:		SeTcbPrivilege

What happens if you delete your cache2 folder? Does the call still fail?

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Chromium WONTFIXED this because for them it originates from ReplaceFile so they consider it a Windows bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=1038507#c5

Though from looking through our code, we use that (ReplaceFile) in very few places and the relevant failing calls might actually be able to avoid it, at the potential of not copying ACL and maybe introducing other strange problems?

Not convinced we should spend much time on this.

Comment 4

[Mike Kaply [:mkaply]]

Is there any explanation for why we hit this so much more than Chrome?

Comment 5

[Barret Rennie [:barret] (they/them)]

I don't really have a lot of insight into the usage of SetNamedSecurityInfoW in nsLocalFile::CopySingleFile except that CopySingleFile is called recursively when copying an entire directory, which would explain the high volume of calls if we are copying large directories.

Comment 6

[Bob Owen (:bobowen)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #2)
...

Stepped over that call in windbg to prove it is the source and also that the function is failing anyway.
Not clear why it is failing at this point it would appear that we should have the required permissions. The error is:
0xc000007c - An attempt was made to reference a token that doesn't exist. This is typically done by referencing the token associated with a thread when the thread is not impersonating a client.

That's interesting. For me I don't see this call failing, but something still logs a Sensitive Priviledge use to the Event Vewer:

You're right it isn't failing, I was thrown by the last error.
I guess that last error might well be getting set when whatever check is happening that causes the audit logging.
Clearly it recovers from that though.
I'll try and dig a little deeper to see what's happening.

Comment 7

[Bob Owen (:bobowen)]

Having dug a little deeper, it is a NtSetSecurityObject syscall that is causing the SeTcbPrivilege request.
I've looked a little into that call, but haven't managed to uncover much more.

The uses that are triggering this would appear to moving files we've created around in the profile directory, so it is likely that we will already be inheriting permissions.
So, I think that the best solution is to test this first.
We already have the DACL, so it should be straightforward.

Comment 8

[Bob Owen (:bobowen)]

Attachment: Bug 1816694: Don't call SetNamedSecurityInfoW if permissions are already inherited. r=mhowell!

This avoids a syscall and also prevents a lot of unnecessary SeTcbPrivilege
requests which may be audited by security software.

Comment 9

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=a26ee2163746690379cd2e90b8e77c5a1b44b715

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c800cd3be29e
Don't call SetNamedSecurityInfoW if permissions are already inherited. r=mhowell

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/c800cd3be29e

Comment 12

[Bob Owen (:bobowen)]

[Tracking Requested - why for this release]:
Given that we are in the second half of the cycle I'm guessing that we won't uplift to Beta.
However, given that this was raised as an issue for some security monitoring software, we may well want to take this for ESR102.
ESR115 is a few months away.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I'll track this for possible ESR uplift next cycle, but it would be great if we could get some verification from the reporter that this is working better either from Nightly now or after 112 rides to Beta the week after next.

Comment 14

[Mike Kaply [:mkaply]]

I've reached out to the customer to ask them to verify.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I don't think we're going to hear back from the customer unfortunately, but this has baked for a month without known side effects anyway, so we might as well go forward with the request. Molly, can you please request ESR approval on this since Bob won't be back until next week? Thanks!

Comment 16

[Molly Howell (she/her) (no longer active)]

Comment on attachment 9319622 [details]
Bug 1816694: Don't call SetNamedSecurityInfoW if permissions are already inherited. r=mhowell!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This bug causes a large amount of spam in certain logs generated by auditing tools commonly used in enterprise contexts.
• User impact if declined: This bug seems likely to cause churn by making important security logs much more difficult to use.
• Fix Landed on Version: 112
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch ought to have no end user-visible impact whatsoever, and the fact that it's been landed for a month with no related reports would seem to tell us that that is indeed the case.

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9319622 [details]
Bug 1816694: Don't call SetNamedSecurityInfoW if permissions are already inherited. r=mhowell!

Approved for 102.10esr

Comment 18

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/90f1ea058531