Truncation in CFStringRefToUTF8Buffer()

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: q1, Assigned: max, Mentored)

Tracking

({good-first-bug})

54 Branch
mozilla57
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment)

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: }
Group: core-security
Priority: -- → P3
Flags: needinfo?(spohl.mozilla.bugs)
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
Mentor: spohl.mozilla.bugs
Flags: needinfo?(spohl.mozilla.bugs)
Keywords: good-first-bug
Whiteboard: [good first bug][lang=c++]
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.
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 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!
Attachment #8898119 - Flags: review?(spohl.mozilla.bugs) → review+
(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.
Assignee: nobody → max
Status: NEW → ASSIGNED
Thanks Stephen. What else do I need to do to get the patch tested and pulled?
mozilla-inbound is currently closed. Your patch will land automatically as soon as it reopens, which could be any time now.
Pushed by spohl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33a96b254013
Fix truncation of buffer size in CFStringRefToUTF8Buffer(); r=spohl
https://hg.mozilla.org/mozilla-central/rev/33a96b254013
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.