1830186 - Dangling Pointer in GridLines::SetLineInfo

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect)

Comment 1

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1830186 - Use local variable instead of temporary in GridLines::SetLineInfo

SafeElementAt takes a reference to a default value. In this case, a temporary
array is constructed and passed as this reference. If this default value is
used, it is returned by reference. However, after the SafeElementAt call that
variable has gone out of scope. This will cause crashes or reading arbitrary
memory if the storage for that argument is written to.

Fortunately this is only a possibility when using the dev tools, and seems to
not actually be an issue currently, but this is a potential security bug.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Emily McDonough [:alaskanemily] from comment #1)

Fortunately this is only a possibility when using the dev tools, and seems to
not actually be an issue currently, but this is a potential security bug.

Calling sec-moderate per above^

Comment 3

[Emily McDonough [:alaskanemily]]

Comment on attachment 9330499 [details]
Bug 1830186 - Use local variable instead of temporary in GridLines::SetLineInfo

Security Approval Request

• How easily could an exploit be constructed based on the patch?: There is no known method to exploit this. At the very least, it would require the user opening the dev tools on a document using grid layout.
• 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?: Everything since Firefox 59
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This should apply cleanly to all supported branches.
• How likely is this patch to cause regressions; how much testing does it need?: The change should be safe, and is unlikely to cause any regressions. Because no actual exploit or crash is known, no testing can validate it.
• Is Android affected?: Yes

Comment 4

[Emily McDonough [:alaskanemily]]

Comment on attachment 9330499 [details]
Bug 1830186 - Use local variable instead of temporary in GridLines::SetLineInfo

Beta/Release Uplift Approval Request

• User impact if declined: Potential crashes or possibly a security issue when using dev tools on content with grid layout. No example of this crash has been found, but it is still a possibility.
• 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): The change is extremely simple, and is at worst just a tiny refactor.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 5

[Emily McDonough [:alaskanemily]]

Comment on attachment 9330499 [details]
Bug 1830186 - Use local variable instead of temporary in GridLines::SetLineInfo

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The change is very small and prevents a potential security flaw.
• User impact if declined: Potential crashes or possibly a security issue when using dev tools on content with grid layout. No example of this crash has been found, but it is still a possibility.
• Fix Landed on Version: 114 pending sec approval
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is extremely simple, and is at worst just a tiny refactor.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: toy program to illustrate why this is an issue (proving that the temporary doesn't get lifetime-extended)

For folks like me who look at the old code and think to themselves, "hey wait, doesn't C++ say that temporaries get lifetime-extended if you capture them in a const reference something something": that didn't actually happen in this case.

Here's a trivial program (a simplified version of the actual code involved here) to prove this. If I compile with g++ and run this program, I get the following output:

In Logger constructor for 0x7fff48c2260f
Entering GetMeALogger
In Logger destructor for 0x7fff48c2260f
After the call to GetMeALogger
In DoStuff for 0x7fff48c2260f

Note that the destructor does get invoked right after the call to GetMeALogger, i.e. the temporary variable does not get its lifetime extended, and the subsequent usage (DoStuff) is potentially crashy/risky.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Emily McDonough [:alaskanemily] from comment #3)

• Which older supported branches are affected by this flaw?: Everything since Firefox 59

[adding detail] specifically, this was introduced by this change for bug 1423378, I think, where we switched from copying the about-to-be-destructed-temporary into a local variable [which is fine] to capturing it in a reference [which is not-fine]:
https://hg.mozilla.org/mozilla-central/rev/60d0c815b98047ab945bf8b7d2c235bc260ba8ac#l1.12

Once we're ready to open this bug up (once the patch has reached all supported releases), we should probably mark it as a regression from bug 1423378.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9330499 [details]
Bug 1830186 - Use local variable instead of temporary in GridLines::SetLineInfo

Given that this is a sec-moderate, it doesn't technically require sec-approval to land. That said, I do appreciate you checking anyway given how late we are in the cycle. I'm OK uplifting this to Beta & ESR ahead of our RC builds next week given the very low risk of the patch, so go ahead and land this.

Comment 9

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

Use local variable instead of temporary in GridLines::SetLineInfo r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a6dc276798192dc33f392b55de4abcd4ebf7e603
https://hg.mozilla.org/mozilla-central/rev/a6dc27679819

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9330499 [details]
Bug 1830186 - Use local variable instead of temporary in GridLines::SetLineInfo

Approved for 113.0rc1 and 102.11esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/96af71f51c28
https://hg.mozilla.org/releases/mozilla-esr102/rev/50b18e01ff34