Quick C++ question - not sure if crazy or observant.

Myrmidon

So, I'm currently studying RTSP for fun, and I ran into a guy's program on the web entitled "RTSP Hello World." There's a portion in it I'm scratching my head over - it looks like a vulnerable piece of code, but it could be that I'm just not a very good programmer yet.

Background of my question:

A client sends an RTSP packet to this program. The packet has been unwrapped and is now stored in a buffer (it took me a couple hours to convince myself that this happened). The programmer named the char buffer "CurRequest," and used the sizeof command to determine "CurRequestSize" (an integer).

Let's say for the sake of argument that the RTSP packet is straight out of the ietf's definition of RTSP, and is their example SETUP packet.

SETUP rtsp://example.com/foo/bar/baz.rm RTSP/1.0
CSeq: 302
Transport: RTP/AVP;unicast;client_port=4588-4589

So basically at this point, that's what CurRequest contains.

The programmer then goes on to create three more variables: a global m_RtspCmdType (it's an enum that can have values like RTSP_OPTIONS or RTSP_DESCRIBE, basically our output), a boolean named 'parseSucceeded' (mostly beyond the scope of my question), and a char[200] named CmdName. Other than the last, I'm only bringing them up because they happen to be in the code snippet, not because they're important.


This is where the code comes in. The code is SUPPOSED to hunt for spaces in the RTSP packet (as a safety feature to be sure the packet can be parsed, the code just quits if it can't find a space), then populate CmdName with the entire packet up to CmdName's size (so, the first 200 characters of the packet), then finally search CmdName for an applicable command name. The code follows:

unsigned i;
for (i = 0; i < sizeof(CmdName)-1 && i < CurRequestSize; ++i)
{
char c = CurRequest[i];
if (c == ' ' || c == '\t')
{
parseSucceeded = true;
break;
}
CmdName[i] = c;
}
CmdName[i] = '\0';
if (!parseSucceeded) return false;

// find out the command type
if (strstr(CmdName,"OPTIONS") != nullptr) m_RtspCmdType = RTSP_OPTIONS; else
if (strstr(CmdName,"DESCRIBE") != nullptr) m_RtspCmdType = RTSP_DESCRIBE; else
if (strstr(CmdName,"SETUP") != nullptr) m_RtspCmdType = RTSP_SETUP; else
if (strstr(CmdName,"PLAY") != nullptr) m_RtspCmdType = RTSP_PLAY; else
if (strstr(CmdName,"TEARDOWN") != nullptr) m_RtspCmdType = RTSP_TEARDOWN;

Finally, my question:

If I were a total asshole and sent the following packet:

SETUP rtsp://OPTIONS.com/foo/bar/baz.rm RTSP/1.0
CSeq: 302
Transport: RTP/AVP;unicast;client_port=4588-4589

Wouldn't that set m_RtspCmdType to RTSP_OPTIONS, not RTSP_SETUP, effectively messing up the output?

I'm not sure if this is a coding oversight and I've caught on properly, or if I'm mistaken and have missed something - char buffers still confuse me a little.

shwaip

looks like the for loop replaces the first space with the string terminating character (\0), so strstr shouldn't look past it.

Myrmidon

Oh my god I'm an idiot. I didn't see the 'break' in the if statement - I thought it was a 'continue.' I am the worst.

I must have looked over the code a billion times and missed that. You're right, cmdname will be null terminated once the loop breaks.

Thanks shwaiples!