Closed Bug 1442318 Opened 3 years ago Closed 3 years ago

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


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 + fixed
firefox60 + fixed


(Reporter: emilio, Assigned: emilio)



(Keywords: regression, sec-high, Whiteboard: [adv-main59+])


(1 file)

mStrings.mString = converted;

calls operator= on a string that is garbage.
Attached patch PatchSplinter Review
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
Attachment #8955212 - Flags: review?(josh)
Attachment #8955212 - Flags: approval-mozilla-beta?
Blocks: 1397971
Attachment #8955212 - Flags: review?(josh) → review+
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 and fill out the resulting template.
Flags: needinfo?(emilio)
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 on attachment 8955212 [details] [diff] [review]

Adding mozilla-release flag here for 59.
Attachment #8955212 - Flags: approval-mozilla-release?
Comment on attachment 8955212 [details] [diff] [review]

[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?

Which older supported branches are affected by this flaw?

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.
Flags: needinfo?(emilio)
Attachment #8955212 - Flags: sec-approval?
Comment on attachment 8955212 [details] [diff] [review]

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.
(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.
I mean replace
> Servo_ReleaseArcStringData(&mStrings.mRustString);
> mStrings.~RustOrGeckoString();
... and then I realized that I was wrong...

Ignore that, then.
Comment on attachment 8955212 [details] [diff] [review]

sec-approval+ for trunk.
Attachment #8955212 - Flags: sec-approval? → sec-approval+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8955212 [details] [diff] [review]

Approved for Fx59rc1.
Attachment #8955212 - Flags: approval-mozilla-release?
Attachment #8955212 - Flags: approval-mozilla-release+
Attachment #8955212 - Flags: approval-mozilla-beta?
Attachment #8955212 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main59+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.