Mike Conley (:mconley) (:⚙️) Reporter
	Description • 8 years ago

This happens here: https://dxr.mozilla.org/mozilla-central/rev/8d0aadfe7da782d415363880008b4ca027686137/uriloader/prefetch/nsPrefetchService.cpp#827

See: https://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTPromiseFlatString.h#35

We ran into something like this in bug 1276013 - essentially, this pattern:

const char* buffer = NS_ConvertUTF16toUTF8(aStr).get();

results in buffer being a dangling pointer that is use-after-freeable.

One needs to fiddle a pref in order to get to this chunk of the code, so it's probably not a huge deal. Still, we probably should plug this.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 1 • 8 years ago

Is this only enabled in nightly right now? (Based on bug 1016628) Or does this also affect other branches?

	u408661 Assignee
	Comment 2 • 8 years ago

nsPrefetchService has nothing to do with the stuff in bug 1016628, this has been in the tree since 44. Again, requires a particular pref to be flipped while running to even be a potential issue, but even a potential issue bears fixing :)

	u408661 Assignee
	Comment 4 • 8 years ago

Comment on attachment 8757418 [details] [diff] [review]
Fix NS_ConvertUTF16toUTF8 usage.

Ignore the r=bz in the patch, I'll fix that up after r+ (don't want to fight bugzilla trying to get it so bz can review this).

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 8 years ago

Comment on attachment 8757418 [details] [diff] [review]
Fix NS_ConvertUTF16toUTF8 usage.

NS_ConvertUTF16toUTF8 is a class, so you could also do
NS_ConvertUTF16toUTF8 converted(aData);
But up to you.

Nit, * goes with the type, not with variable name, so char* pref

	u408661 Assignee
	Comment 6 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bdab9ae049f

	u408661 Assignee
	Comment 7 • 8 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5914f11000

	Carsten Book [:Tomcat]
	Comment 8 • 8 years ago

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

	u408661 Assignee
	Comment 9 • 8 years ago

Comment on attachment 8757418 [details] [diff] [review]
Fix NS_ConvertUTF16toUTF8 usage.

Approval Request Comment
[Feature/regressing bug #]: bug 1213443
[User impact if declined]: potential uaf security risk when changing one pref
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: low/none
[String/UUID change made/needed]: none

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 8 years ago

Comment on attachment 8757418 [details] [diff] [review]
Fix NS_ConvertUTF16toUTF8 usage.

Based on comment 2 sounds like a good idea to bring this up at least to aurora to prevent potential UAF.

	Carsten Book [:Tomcat]
	Comment 11 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/82ec5faa2397e9dd0f6c881e5bdcf773e0ba9a88

	Masatoshi Kimura [:emk]
	Comment 12 • 8 years ago

Is it possible to prevent such usage by static analysis or something?