1167370 - TextRenderer::RenderText using uninitialized memory

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[q1]

User Agent: Mozilla/5.0 (Windows; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20150305021524

Steps to reproduce:

In Firefox 38.0.1, TextRenderer::RenderText uses uninitialized memory at gfx\layers\composite\TextRenderer.cpp line 96 et seq.

The problem is that TextRenderer::RenderText does not check the return value from DataSourceSurface::Map at line 93, thus (on failure and for some derived classes of DataSourceSurface, e.g., DataSourceSurfaceD2D) leaving untouched whatever was contained in the uninitialized variable map:

 92: DataSourceSurface::MappedSurface map;
 93: textSurf->Map(DataSourceSurface::MapType::READ_WRITE, &map);
 94:
 95: // Initialize the surface to transparent white.
 96: memset(map.mData, uint8_t(sBackgroundOpacity * 255.0f),
 97:        numLines * sCellHeight * map.mStride);
 98:
 99:  uint32_t currentXPos = 0;
100:  uint32_t currentYPos = 0;
101:
102:  // Copy our glyphs onto the surface.
  ...
115:  memcpy (map.mData + ...)

Since map can contain anything, this bug could make it possible to write data into anywhere in Firefox's address space. Depending on how RenderText is used, this bug might be exploitable to disclose sensitive data and/or cause execution of attacker-selected code. Perhaps an attacker could design a font that contains data to be written to the stack such that, when RenderText returns, it passes control to an attacker-designated function.

Comment 1

[q1]

There's also a similar bug in TextRenderer::EnsureInitialized at line 154.

Comment 2

[Kyle Fung]

Attachment: Returns if Map() fails

Comment 3

[Kyle Fung]

Attachment: Addressed issue in line 154

Comment 4

[Bas Schouten (:bas.schouten)]

Should be noted this isn't exactly sec critical as the TextRenderer isn't used without custom prefs set.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8609666 [details] [diff] [review]
Addressed issue in line 154

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

I'd have a more informative check-in comment, like "check for Map call failing" or some such.  You'd also want it to be in the format Bug xxxxx: comment.  r=reviewer".

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

This is sec-other, so I don't think we need a security approval.

Comment 7

[Kyle Fung]

Attachment: uninitialized-map-v3.patch

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20da4771e913

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20da4771e913

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> Should be noted this isn't exactly sec critical as the TextRenderer isn't
> used without custom prefs set.

If we're never going to use it we should just rip it out. If we're working toward turning it on by default some day then the fact it isn't now doesn't reduce the severity of the potential vulnerability described here (though it wouldn't be an emergency, so critical->high is acceptable).

So what's the status of this feature?

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> (In reply to Bas Schouten (:bas.schouten) from comment #4)
> > Should be noted this isn't exactly sec critical as the TextRenderer isn't
> > used without custom prefs set.
> 
> If we're never going to use it we should just rip it out. If we're working
> toward turning it on by default some day then the fact it isn't now doesn't
> reduce the severity of the potential vulnerability described here (though it
> wouldn't be an emergency, so critical->high is acceptable).
> 
> So what's the status of this feature?

Incorrect, it's used for diagnostic purposes by Mozilla engineers. But only through a pref that would essentially never run in a production environment. So it serves a clear purpose, but is unlikely to ever be used in our normal release builds by our regular users.

(Doesn't mean it isn't nice to have fixed)

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Would we want to make it only show up in nightly+aurora then?

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Bas Schouten (:bas.schouten) from comment #11)
> Incorrect, it's used for diagnostic purposes by Mozilla engineers. But only
> through a pref that would essentially never run in a production environment.
> So it serves a clear purpose, but is unlikely to ever be used in our normal
> release builds by our regular users.

Thanks for clarifying. sec-other is appropriate then