925031 - TextInputHandler.mm:3841:5 [-Warray-bounds] array index 1 is past the end of the array (which contains 1 element)

Status: RESOLVED INCOMPLETE

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

Our code:

    InterfaceTypeList supportedServices;
    supportedServices[0] = kTextServiceDocumentInterfaceType;
    supportedServices[1] = kUnicodeDocumentInterfaceType;
    ::NewTSMDocument(2, supportedServices, &mPluginTSMDoc, 0);

The definition for InterfaceTypeList:

    typedef OSType InterfaceTypeList[1];

Thus supportedServices[1] is an invalid assignment. I wonder if this ever shows up in crash stats.

Comment 1

[Josh Aas]

Marking this security sensitive to be safe.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

I think I'm originally responsible for that code, so I'll take it.

The fix should be very straightforward.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Still (re)building my patch.

But I've found that, at least in recent clang builds, we're saved by the skin of our teeth from having this bug cause any crashes.

PluginTextInputHandler::ActivatePluginTSMDocument() (which has this bug) is only compiled in 32-bit mode.  Furthermore supportedServices is its only stack variable -- which (on the face of it) makes it likely there's extra room in the space set aside for stack variables, and that writing one element beyond the end of this array won't stomp on anything else.

I've confirmed (using gdb's disassemble) that's the case in recent clang builds:  Altogether 20 bytes are provided for stack variables, and 8 bytes are provided for supportedServices -- which is exactly what's needed.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

(I checked this in a recent mozilla-central nightly, which is an opt build without symbols stripped.)

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

The syntax is a bit weird, so I'm doing a set of Mac tryserver builds before I ask for a review.  Clang wouldn't let me C-cast an OSType* to an InterfaceTypeList, but had no problems with this syntax.  The resulting assembly code also looked correct.

I'll ask Josh for a review if/when the tryserver builds turn out OK.  The results will eventually be available here:

https://tbpl.mozilla.org/?tree=Try&rev=2b8afda65006

Comment 6

[Josh Aas]

Comment on attachment 815140 [details] [diff] [review]
Fix

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

::: widget/cocoa/TextInputHandler.mm
@@ +3836,5 @@
>      // more than one kind of input (say Hiragana and Romaji).  This is what
>      // the OS does when it creates a TSM document for use by an
>      // NSTSMInputContext class.
> +    OSType holder[2];
> +    InterfaceTypeList& supportedServices = (InterfaceTypeList&) holder;

Better to use static_cast if nothing is preventing you from doing so.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1

Apparently the rules for static_cast are stricter than those for C-style casts -- I found I couldn't get static_cast to work with my previous patch.  So I rewrote it.  Along the way I found you still get the compiler's warning if you try to set (*supportedServices)[1], so I set holder[1] (and holder[0]) instead.

Clearly Apple's definition of InterfaceTypeList (which makes its size one element) is a design flaw -- Apple's own code uses InterfaceTypeList arrays with more than one element.  But it's not hard to work around.  And in any case the Text Input Manager APIs are now obsolescent.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 815416 [details] [diff] [review]
Fix rev1

Oops, uploaded wrong patch.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2

Actually it makes better sense to use reinterpret_cast.

Comment 11

[Markus Stange [:mstange]]

Does this do what it's supposed to do? If you ignore the types, you basically pass *holder to NewTSMDocument, and not holder (without the deref)... or is C++ just that confusing?

Comment 12

[Josh Aas]

Comment on attachment 815455 [details] [diff] [review]
Fix rev2

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

I think Markus is right, it's incorrect. The definition for InterfaceTypeList is:

typedef OSType InterfaceTypeList[1];

And the definition for NewTSMDocument is:

extern OSErr NewTSMDocument(SInt16 numOfInterface, InterfaceTypeList supportedInterfaceTypes,
                            TSMDocumentID * idocID, SRefCon refcon)  

So,

InterfaceTypeList* supportedServices = reinterpret_cast<InterfaceTypeList*>(holder);

should be:

InterfaceTypeList supportedServices = reinterpret_cast<InterfaceTypeList>(holder);

Very confusing code.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Actually I think my original patch was correct, but what Josh suggests would be less confusing ... if it works.

Note that 'holder' and '&holder' are synonyms, and that I passed '*supportedServices' to NewTSMDocument().

A big raspberry to Apple for its horrible definition of InterfaceTypeList :-(

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Josh's suggestion doesn't compile.  Let me see what else I can come up with.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

This bug isn't urgent -- we're not crashing because of it.  And I still haven't been able to come up with a solution that's both correct and "easy to read".  So I'm going to put this off for a while, then (hopefully) come back to it later with a fresh outlook.

Comment 16

[Andrew McCreight [:mccr8]]

It doesn't sound like this is really exploitable, but feel free to adjust the rating as needed.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Shall we unmark this as a security bug?

We actually don't crash.  And this bug is very easily discovered, since it spawns a warning.

Comment 19

[Andrew McCreight [:mccr8]]

Sounds reasonable to me.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 21

[Marco Castelluccio [:marco]]

Sorry, there was a problem with the detection of inactive users. There was no action for a while so I won't revert the change, but feel free to reassign it to yourself in case you're still planning to work on this.

Comment 22

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

This is left over from when I was a Mozilla employee. I'll never get back to it.

Comment 24

[Stephen A Pohl [:spohl] (OOO until 3/25)]

There is no trace of this code left in tree anymore.