Closed Bug 1296391 Opened 4 years ago Closed 4 years ago

Latent truncation in gfxFontUtils::RenameFont() could cause buffer overrun

Categories

(Core :: Graphics: Text, defect)

48 Branch
Unspecified
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: q1, Assigned: jfkthame)

Details

(Keywords: sec-want, Whiteboard: [post-critsmash-triage][adv-main51-] btpp-active)

Attachments

(1 file)

gfxFontUtils::RenameFont() (gfx\thebes\gfxFontUtils.cpp) can experience an integer overflow. If it does, it allocates a buffer too short for the data that it writes into it. The data written is a font name table.

This bug appears to be latent because there appear to be no callers that use a font name long enough (>= 0x8000 characters) to trigger it. [1]

The bug is in line 984:


 965: nsresult
 966: gfxFontUtils::RenameFont(const nsAString& aName, const uint8_t *aFontData, 
 967:                          uint32_t aFontDataLength, FallibleTArray<uint8_t> *aNewFont)
 968: {
 969:     NS_ASSERTION(aNewFont, "null font data array");
 970:     
 971:     uint64_t dataLength(aFontDataLength);
 972: 
 973:     // new name table
 974:     static const uint32_t neededNameIDs[] = {NAME_ID_FAMILY, 
 975:                                              NAME_ID_STYLE,
 976:                                              NAME_ID_UNIQUE,
 977:                                              NAME_ID_FULL,
 978:                                              NAME_ID_POSTSCRIPT};
 979: 
 980:     // calculate new name table size
 981:     uint16_t nameCount = ArrayLength(neededNameIDs);
 982: 
 983:     // leave room for null-terminator
 984:     uint16_t nameStrLength = (aName.Length() + 1) * sizeof(char16_t); 

 ...in which |nameStrLength| receives the result of the RHS, truncated to 16 bits. Thus, if |aName().Length + 1| > 0x8000, it receives a result that is too small. This value is then used to calculate |nameTableSize|...

 985: 
 986:     // round name table size up to 4-byte multiple
 987:     uint32_t nameTableSize = (sizeof(NameHeader) +
 988:                               sizeof(NameRecord) * nameCount +
 989:                               nameStrLength +
 990:                               3) & ~3;
 991:                               
 992:     if (dataLength + nameTableSize > UINT32_MAX)
 993:         return NS_ERROR_FAILURE;
 994:         
 995:     // bug 505386 - need to handle unpadded font length
 996:     uint32_t paddedFontDataSize = (aFontDataLength + 3) & ~3;
 997:     uint32_t adjFontDataSize = paddedFontDataSize + nameTableSize;
 998: 

 ...which eventually is used to allocate storage for the font data in |aNewFont|:

 999:     // create new buffer: old font data plus new name table
1000:     if (!aNewFont->AppendElements(adjFontDataSize, fallible))
1001:         return NS_ERROR_OUT_OF_MEMORY;
1002: 

The code then copies the old font data into the new storage...

1003:     // copy the old font data
1004:     uint8_t *newFontData = reinterpret_cast<uint8_t*>(aNewFont->Elements());
1005:     
1006:     // null the last four bytes in case the font length is not a multiple of 4
1007:     memset(newFontData + aFontDataLength, 0, paddedFontDataSize - aFontDataLength);
1008: 
1009:     // copy font data
1010:     memcpy(newFontData, aFontData, aFontDataLength);
1011:     
1012:     // null out the last 4 bytes for checksum calculations
1013:     memset(newFontData + adjFontDataSize - 4, 0, 4);
1014:     

...followed by the new name table, which overruns the buffer on lines 1039-42, which copy the *untruncated* number of characters |aName.Length()| :

1015:     NameHeader *nameHeader = reinterpret_cast<NameHeader*>(newFontData +
1016:                                                             paddedFontDataSize);
1017:     
1018:     // -- name header
1019:     nameHeader->format = 0;
1020:     nameHeader->count = nameCount;
1021:     nameHeader->stringOffset = sizeof(NameHeader) + nameCount * sizeof(NameRecord);
1022:     
1023:     // -- name records
1024:     uint32_t i;
1025:     NameRecord *nameRecord = reinterpret_cast<NameRecord*>(nameHeader + 1);
1026:     
1027:     for (i = 0; i < nameCount; i++, nameRecord++) {
1028:         nameRecord->platformID = PLATFORM_ID_MICROSOFT;
1029:         nameRecord->encodingID = ENCODING_ID_MICROSOFT_UNICODEBMP;
1030:         nameRecord->languageID = LANG_ID_MICROSOFT_EN_US;
1031:         nameRecord->nameID = neededNameIDs[i];
1032:         nameRecord->offset = 0;
1033:         nameRecord->length = nameStrLength;
1034:     }
1035:     
1036:     // -- string data, located after the name records, stored in big-endian form
1037:     char16_t *strData = reinterpret_cast<char16_t*>(nameRecord);
1038: 
1039:     mozilla::NativeEndian::copyAndSwapToBigEndian(strData,
1040:                                                   aName.BeginReading(),
1041:                                                   aName.Length());
1042:     strData[aName.Length()] = 0; // add null termination
    

[1] gfxFontUtils::RenameFont() appears to be called by only gfxGDIFontList::MakePlatformFont() and gfxDWriteFontList::MakePlatformFont(), both of which use a name generated by gfxFontUtils::MakeUniqueUserFontName(), which generates names much too short to trigger this bug.
OS: Unspecified → Windows
Group: core-security → gfx-core-security
Flags: sec-bounty?
Oops, "can experience an integer overflow" should read "can truncate an integer result". I take full responsibility for using that template....
As pointed out in comment 0 note [1], gfxFontUtils::RenameFont is a Gecko-internal helper function that is not called with arbitrarily-long names that an attacker could control and use to trigger an overflow. It is only called with limited-length names that are generated within Gecko itself. Therefore, there is no actual risk here; the theoretical overflow situation described cannot actually be reached. 

On principle, I suppose it would be good to add a precautionary length-check in RenameFont, as humungously-long font names might well cause other problems as well; but this is not an actual security bug in Gecko as it stands.
As noted above, the way this is used in Gecko there is currently no risk of encountering this situation, but it seems wise to include a length check here just in case we ever change how we're using the method.
Attachment #8786324 - Flags: review?(jmuizelaar)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8786324 - Flags: review?(jmuizelaar) → review+
Marking this sec-want, as it's just a precautionary fix/code-cleanup for a latent issue that is not exposed or accessible in any way in current code, so no higher rating seems relevant.
Keywords: sec-want
https://hg.mozilla.org/integration/mozilla-inbound/rev/5830a5488348c2eb891774f33968d12c2ac8d276
Bug 1296391 - Check length of name string before attempting to build a new name table. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/5830a5488348
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: sec-bounty? → sec-bounty+
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage] btpp-active
Whiteboard: [post-critsmash-triage] btpp-active → [post-critsmash-triage][adv-main51-] btpp-active
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.