[rfc][icedtea-web] Malformed messages from IcedTeaPluginRequestProcessor (PR539)

[Andrew Azores]

On 10/10/2013 05:02 PM, Andrew Azores wrote:
> On 10/10/2013 04:48 AM, Jiri Vanek wrote:
>> On 10/09/2013 08:02 PM, Andrew Azores wrote:
>>> Hi,
>>>
>>> PR539 was reopened with a comment that the applet at [1] no longer 
>>> works. Looking through the
>>> provided logs, an index out of bounds exception is occurring at line 
>>> 53 of
>>> GetMemberPluginCallRequest.java. This happens because the 
>>> parseReturn method expects its String
>>> param to be of the form "reference 28 JavaScriptToString 90", but 
>>> occasionally it can occur that the
>>> param is instead of the form "reference 30 JavaScriptToString ", 
>>> missing the second numeric value.
>>> This happens because of _getString and _getMember in 
>>> IcedTeaPluginRequestProcessor.cc. If
>>> data->call_successful is false in either of these functions, then 
>>> data->result is never modified
>>> from the value it held before data was passed into the functions, 
>>> which means it is an empty string.
>>> The data->call_successful struct member is set by a call to 
>>> NPNetscapeFuncs so I'm not entirely sure
>>> why it is coming back false in these cases.
>>>
>>> It's certainly not an expected situation for these messages to be 
>>> malformed, which is especially
>>> evident looking around the codebase. Many other places do similar 
>>> work as done in
>>> GetMemberPluginCallRequest.parseReturn(String), where 
>>> message.split(" ") is called and the length of
>>> the resulting array never validated. Adding a length validation to 
>>> parseReturn seems to fix this
>>> particular bug (the applet loads at least, but I'm not sure what 
>>> else this bug may be affecting) but
>>> doesn't address the issue of the message being malformed in the 
>>> first place. It seems like the
>>> expected behaviour at some point in the past would be for these 
>>> messages instead to have "null" or
>>> "void":
>>>
>>>          if (!"null".equals(args[3]) && !"void".equals(args[3]))
>>>
>>> appended to them rather than the empty string, but that does not 
>>> seem to be the case now. Appending
>>> this to the data->result string is easy enough, it can just be done 
>>> in a new else-clause for
>>> data->call_successful in _getString and _getMember. As far as I'm 
>>> able to tell this would be the
>>> earliest feasible place to do the addition. It could also be done in
>>> IcedTeaPluginRequestProcessor::sendString or ::sendMember, but these 
>>> call _getString and _getMember
>>> respectively through IcedTeaPluginUtilities::callAndWaitForResult 
>>> anyway. The patch takes the
>>> approach of adding it to _getString and _getMember. Other functions 
>>> in the file, such as _eval and
>>> _call, append "0" when call_successful is false, so this approach 
>>> makes the most sense to me. I
>>> don't see any problems that this might cause and running the 
>>> reproducers showed no new failures either.
>>>
>>> ChangeLog:
>>> Fix array index out of bounds due to malformed plugin message (PR539)
>>> * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc: (_getMember, 
>>> _getString) append "null" to
>>> result when call is unsuccessful
>>>
>>> [1] 
>>> https://www.bankid.no/Hjelp-og-nyttige-verktoy/Nyttige-verktoy/Test-din-BankID/
>>>
>>> Thanks,
>>>
>>
>>
>> What about any test on this? It woould deserve at least C++ test, or 
>> mock test or reproducer (in best case all three :)
>>
>> Otherwise give sense to me.
>>
>>
>> J.
>
> Took me a while to figure out how to make a JavaScript object that 
> doesn't respond to toString(), but I got it. Attached both patches again.
>
> Note that this reproducer will currently fail whether or not you apply 
> the fix patch. Note that commit 420d72e5cee7, the one I proposed be 
> backed out a while ago, is a blocker on this bug as well. If you "hg 
> backout -r 420d72e5cee7" and then apply the reproducer patch, it will 
> fail in the manner I intend it to. Then when you apply the fix, the 
> reproducer will pass.
>
> Thanks,
>

Ping?

Thanks,

-- 
Andrew A