1442318 - mozilla::css::URLValueData::GetUTF16String is unsound.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

mStrings.mString = converted;

calls operator= on a string that is garbage.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1397971
[User impact if declined]: Memory safety issue.
[Is this code covered by automated tests?]: No, but will be once the dependent bug lands.
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: one-liner fixing memory safety issue.
[String changes made/needed]: none

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Is this really 59 only? Your patch isn't for trunk? It seems like this code affects 58 and higher.

This bugs needs sec-approval to land. Please mark sec-approval? on the patch per https://wiki.mozilla.org/Security/Bug_Approval_Process and fill out the resulting template.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Trunk is now 60. Wontfixing this for 58 since we don't have any dot releases in the works and are a week and a half away from 59 release.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8955212 [details] [diff] [review]
Patch

Adding mozilla-release flag here for 59.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8955212 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
not too easily, but it's obvious to spot the problem.

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?
57+

If not all supported branches, which bug introduced the flaw?
Bug 1397971

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8955212 [details] [diff] [review]
Patch

Review of attachment 8955212 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSValue.cpp
@@ +2911,4 @@
>      nsDependentCSubstring rust = GetRustString();
>      nsString converted = NS_ConvertUTF8toUTF16(rust);
>      Servo_ReleaseArcStringData(&mStrings.mRustString);
> +    new (&mStrings) RustOrGeckoString(converted);

It might be cleaner to also use
> mStrings.~RustOrGeckoString();
for the previous line, I guess, so that we have a pair of dtor and ctor which tells reader that we are replacing the content.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> It might be cleaner to also use
> > mStrings.~RustOrGeckoString();
> for the previous line, I guess, so that we have a pair of dtor and ctor
> which tells reader that we are replacing the content.

Given that line does nothing, I'm not sure if it would help or add complexity... But sure, can do that.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

I mean replace
> Servo_ReleaseArcStringData(&mStrings.mRustString);
with
> mStrings.~RustOrGeckoString();
... and then I realized that I was wrong...

Ignore that, then.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8955212 [details] [diff] [review]
Patch

sec-approval+ for trunk.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

remote: View your change here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb1132d56be5f2237a3c5cf26f90a8323838fe4

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/5cb1132d56be

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8955212 [details] [diff] [review]
Patch

Approved for Fx59rc1.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c65305d4880f (FIREFOX_59b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/96e8880e3d86