Opened 10 years ago

Closed 10 years ago

#7886 closed defect (fixed)

Backend crashes when invalid network request received

Reported by: Ronald Frazier <ron@…> Owned by: cpinkham
Priority: minor Milestone: 0.23
Component: MythTV - General Version: 0.22-fixes
Severity: medium Keywords:
Cc: Ticket locked: no

Description

I've discovered that mythbackend can be crashed by a network request which includes an invalid number of parameters. My example case was using the perl bindings and calling new_program with a blank starttime (which itself makes a QUERY_RECORDING TIMESLOT request to the backend with a blank starttime). However, I've discovered a number of other cases where it can happen.

The source of the bug is in mainserver.cpp. ProcessRequestWork? reads a "QStringList listline" off of the socket. It then takes the first element (listline[0]) and splits it into "QStringList tokens". The listline and tokens variables are then sent to various request handlers, some of which access array indices without first checking the length (which is how my backend crash occurred).

By the way...when listline[0] is accessed in ProcessRequestWork?, it never verifies size() != 0 either. I'm not sure if it's possible for MythSocket::readStringList to create a zero length stringlist while still returning true. If so, this is another problem that needs to be fixed.

I've tried following through the code, to see which functions access without properly checking the array size. For the tokens variable, the only problematic function I saw was HandleQueryRecording?. I'm attatching a patch that fixes that.

For the listline variable, my patch also fixes two spots in ProcessRequestWork? that were problems...in the (command == "MESSAGE") block and (command == "SHUTDOWN_NOW") block. However, there were a number of other cases where the indices weren't verified (and a few that use QStringList::at(index), which I couldn't determine if that would segfault too) and I wasn't sure of the appropriate way to fix them (should they generate an error too, or proceed as if a blank line was sent instead) or if they needed fixing at all in the case of the at() call. So here is the list of (potential) problems I found that my patch does NOT address:

ProcessRequestWork?, in the (command == "BACKEND_MESSAGE") block

HandleQueryCheckFile?

HandleSGGetFileList

HandleSGFileQuery

HandleGetNextFreeRecorder?

HandleRecorderQuery?

HandleSetChannelInfo?

HandleRemoteEncoder?

HandleGetRecorderFromNum?

HandleFileTransferQuery?

HandleMessage?

HandleFillProgramInfo?

HandleIsActiveBackendQuery?

Attachments (1)

mainserver_cpp.patch (1.4 KB) - added by Ronald Frazier <ron@…> 10 years ago.

Download all attachments as: .zip

Change History (4)

Changed 10 years ago by Ronald Frazier <ron@…>

Attachment: mainserver_cpp.patch added

comment:1 Changed 10 years ago by cpinkham

Owner: changed from Isaac Richards to cpinkham
Status: newassigned

comment:2 Changed 10 years ago by Stuart Auchterlonie

Milestone: unknown0.23

comment:3 Changed 10 years ago by cpinkham

Resolution: fixed
Status: assignedclosed

(In [23529]) Add some bounds checking to prevent a few potential master backend crashes.

Fixes #7886, but only the issues pointed out in the patch.

Note: See TracTickets for help on using tickets.