dispatchCommand serves default command with invalid prefix

[quarencia]

I have discovered what I believe to be a bug in webduino.

In lines 527 to 530 in the current code, three conditions are tested to determine if a reqeust should be responded to with the default failure command.

1. requestType == INVALID
2. strcmp to see if prefix is incorrect
3. !dispatchCommand(...)

This is done in one if statement with all three conditions OR'd. Note that in this call, dispatchCommand always executes even if the request type is INVALID and the prefix is incorrect. This is the problem.

I've encountered a logic fault with this statement where the prefix can be invalid and dispatchCommand(...) still returns true. In the particular case I had, webduino served the default root page despite receiving a incorrect (or missing) prefix. After serving the default root page Webduino then served the default failure command before calling reset() to close the client connection.

How this happened:
During stability testing of my code I noticed that the hit counter on my default page appeared to be incrementing out of sequence. I had hit it 3 times, and the next day it showed 500+ hits. After lots of debugging with sketch, webduino, and wireshark, I discovered that various windows discovery services were hitting the webduino with lots of http requests, mostly for the root. In the code statement above, the prefix test failed. However, in many cases (500+ in the course of about 30 hrs), the memory location in buffer that dispatchCommand() looks in (buff + urlPrefixLen) happened to contain "/\0" from some previous use of the memory location. dispatchCommand() sees this a request for the default root page. This meant that even with an invalid prefix, dispatchCommand() still served the default root page.

I was able to test and verify that this happened when calling either overload of processConnection(). Additionally, I was able to force the failure by making a call to to the webserver with an invalid prefix that happened to be the same length as the valid prefix. Additionally², with wireshark, I also captured several instances where "webcollage/1.135a" attacks were getting served the default root page and the default failure page (I consider webcollage an attack) as a result of this same logic fault.

My Solutions:
First and foremost, did some firewall reconfiguring on both the router and windows machines to shut down unwanted traffic.
I don't know if you have any experience with windows 7 and all of it's discovery services, but IMHO it's an unholy mess of uncessary traffic on LANs and the internet.

Second, modify webduino to change the logic. One solution in webduino is to do a memcpy() at the begginning of processConnection() to ensure that the allocated buffer is "clean". To me, that seemed like treating the symptom rather than correcting the logic. My preferred solution was to alter the test sequence so that dispatchCommand() is never called if either the request type and prefix are bad.

I tried to post my code on github for about an hour. Won't do that again unless someone can change my now very negative opinion of guthub. So here is an excerpt of my code changes. This is pretty cleaned up. My actual code now has many more debugging statements and I've also added some ip filtereing that supresses any response at all to certain domains/IPs.

/* * Original code starting at line 527 **********
if (requestType == INVALID ||
strncmp(buff, m_urlPrefix, urlPrefixLen) != 0 ||
!dispatchCommand(requestType, buff + urlPrefixLen,
(_bufflen) >= 0))
{
m_failureCmd(_this, requestType, buff, (bufflen) >= 0);
}
*/
// ** begin changes by me ****************
if(requestType == INVALID || strncmp(buff, m_urlPrefix, urlPrefixLen) != 0 )
{
m_failureCmd(_this, requestType, buff, (_bufflen) >= 0);
}
else if(!dispatchCommand(requestType, buff + urlPrefixLen,(_bufflen) >= 0))
{
m_failureCmd(_this, requestType, buff, (bufflen) >= 0);
}
// ********** end changes by me *****************