MidiManagerUsb should not trust indices provided by renderer.

MidiManagerUsb should not trust indices provided by renderer.

MidiManagerUsb::DispatchSendMidiData takes |port_index| parameter. As it is
provided by a renderer possibly under the control of an attacker, we must
validate the given index before using it.

BUG=456516
Committed: https://crrev.com/5576cbc1d3e214dfbb5d3ffcdbe82aa8ba0088fc
Cr-Commit-Position: refs/heads/master@{#315303}

[yhirano, 2015-02-09 02:14:48]

yhirano@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Takashi Toyoshima, 2015-02-09 06:34:41]

lgtm

https://codereview.chromium.org/907793002/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/907793002/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_usb.cc:46: if (port_index >= output_streams_.size()) {
Can you add TODO(toyoshim) here?
I think we should check this in MidiHost, and call BadMessageReceived() to kill
the malicious renderer. But for now, MidiHost does not have information about
the max port number. So this change looks ok now.

[yhirano, 2015-02-09 08:40:33]

New patchsets have been uploaded after l-g-t-m from toyoshim@chromium.org

[yhirano, 2015-02-09 08:41:12]

https://codereview.chromium.org/907793002/diff/40001/media/midi/midi_manager_...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/907793002/diff/40001/media/midi/midi_manager_...
media/midi/midi_manager_usb.cc:46: if (port_index >= output_streams_.size()) {
On 2015/02/09 06:34:41, Takashi Toyoshima (chromium) wrote:
> Can you add TODO(toyoshim) here?
> I think we should check this in MidiHost, and call BadMessageReceived() to
kill
> the malicious renderer. But for now, MidiHost does not have information about
> the max port number. So this change looks ok now.

Done.

[yhirano, 2015-02-09 08:41:29]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-02-09 08:42:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907793002/60001

[commit-bot: I haz the power, 2015-02-09 10:42:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-09 10:42:56]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  ios_rel_device_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[yhirano, 2015-02-09 10:50:42]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-02-09 10:51:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907793002/60001

[commit-bot: I haz the power, 2015-02-09 11:02:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-09 11:02:05]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[yhirano, 2015-02-09 14:00:17]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-02-09 14:01:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907793002/60001

[commit-bot: I haz the power, 2015-02-09 15:13:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-09 15:15:12]

Patchset 4 (id:??) landed as
https://crrev.com/5576cbc1d3e214dfbb5d3ffcdbe82aa8ba0088fc
Cr-Commit-Position: refs/heads/master@{#315303}