1380078 - Truncation in CFStringRefToUTF8Buffer()

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P3)

Description

[q1]

CFStringRefToUTF8Buffer() (dom\plugins\base\nsPluginsDirDarwin.cpp) truncates the result of CFStringGetMaximumSizeForEncoding() from a |CFIndex| to an |int| on 64-bit platforms. (See https://developer.apple.com/documentation/corefoundation/1542143-cfstringgetmaximumsizeforencodin ; https://developer.apple.com/documentation/corefoundation/cfindex ; https://developer.apple.com/documentation/swift/int ; https://developer.apple.com/library/content/documentation/Darwin/Conceptual/64bitPorting/MakingCode64-BitClean/MakingCode64-BitClean.html ).

The bug (line 104) does not look exploitable because the potentially-truncated buffer size |bufferLength| is used as the maximum buffer size on line 112. Still, it should be fixed.


 97: static char* CFStringRefToUTF8Buffer(CFStringRef cfString)
 98: {
 99:   const char* buffer = ::CFStringGetCStringPtr(cfString, kCFStringEncodingUTF8);
100:   if (buffer) {
101:     return PL_strdup(buffer);
102:   }
103: 
104:   int bufferLength =
105:     ::CFStringGetMaximumSizeForEncoding(::CFStringGetLength(cfString),
106:                                         kCFStringEncodingUTF8) + 1;
107:   char* newBuffer = static_cast<char*>(moz_xmalloc(bufferLength));
108:   if (!newBuffer) {
109:     return nullptr;
110:   }
111: 
112:   if (!::CFStringGetCString(cfString, newBuffer, bufferLength,
113:                             kCFStringEncodingUTF8)) {
114:     free(newBuffer);
115:     return nullptr;
116:   }
117: 
118:   newBuffer = static_cast<char*>(moz_xrealloc(newBuffer,
119:                                               strlen(newBuffer) + 1));
120:   return newBuffer;
121: }

Comment 1

[Stephen A Pohl [:spohl]]

This is a good first bug and I'm happy to mentor someone through it, if necessary. We should also remove the null check for the infallible allocation. The steps to fix this bug are:

1. Change the type of bufferLength from int to int64_t[1].
2. Remove the null check after the infallible allocation[2][3].


[1] https://dxr.mozilla.org/mozilla-central/rev/b95b1638db48fc3d450b95b98da6bcd2f9326d2f/dom/plugins/base/nsPluginsDirDarwin.cpp#104
[2] https://dxr.mozilla.org/mozilla-central/rev/b95b1638db48fc3d450b95b98da6bcd2f9326d2f/dom/plugins/base/nsPluginsDirDarwin.cpp#108-110
[3] https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation

Comment 2

[Maximilian Hunt]

I'm new to the Mozilla project, and I'd like to have a look at this. Looks like I should be able to get started.

Comment 3

[Maximilian Hunt]

I may have made a mistake. I hadn't realised this was an OSX bug and I'm not on OSX.

I wrote the patch anyway and I've submitted it for review on Mozreview, but let me know if this means I shouldn't be working on this.

Comment 4

[Maximilian Hunt]

Attachment: Bug 1380078: Fix truncation of buffer size in CFStringRefToUTF8Buffer();

On 64 bit systems CFStringGetMaximumSizeForEncoding() returns a
CFIndex (long) integer which was being converted to an int, which may
not be 64 bit. This has been changed to an int64_t so it will never
truncate.

In addition, in the same function there was a redundant null check on
the return from moz_xmalloc.  This is an infallible memory allocator,
so the check is useless and has been removed.

Review commit: https://reviewboard.mozilla.org/r/169454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169454/

Comment 5

[Stephen A Pohl [:spohl]]

Comment on attachment 8898119 [details]
Bug 1380078: Fix truncation of buffer size in CFStringRefToUTF8Buffer();

https://reviewboard.mozilla.org/r/169454/#review175030

This patch and commit message are perfect! Thank you!

Comment 6

[Stephen A Pohl [:spohl]]

(In reply to Maximilian Hunt from comment #3)
> I may have made a mistake. I hadn't realised this was an OSX bug and I'm not
> on OSX.
> 
> I wrote the patch anyway and I've submitted it for review on Mozreview, but
> let me know if this means I shouldn't be working on this.

You are welcome to fix bugs on platforms that aren't the same as your development environment. You may find it easier to fix bugs on the same platform, especially if they require local debugging and/or testing. But straightforward patches like the one in this bug can absolutely be written on other platforms.

Comment 7

[Maximilian Hunt]

Thanks Stephen. What else do I need to do to get the patch tested and pulled?

Comment 8

[Stephen A Pohl [:spohl]]

mozilla-inbound is currently closed. Your patch will land automatically as soon as it reopens, which could be any time now.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33a96b254013
Fix truncation of buffer size in CFStringRefToUTF8Buffer(); r=spohl

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/33a96b254013