Saturday 4 May 2013

Follow-up to "Quick Code Puzzle" article

I'm a bit disappointed that I only got three responses to my code puzzle the other day, but so be it. At least Matt, Dale and Winston will get a beer next time I see them. Thanks guys: I really do appreciate you taking the time to join in.

Bruce: cheers for your response, but you didn't actually post a solution, so you weren't in the running for a beer!

To recap, the problem was to write this function:

boolean function isWithinWebroot(required string fileSystemPath){
    // provide code here

With the following guidance:

If the tested path passes the test, it will then be used as the base path to serve file(s), so it must not be hackable. Please note that the value passed into the function comes from user input.
The reason why I was asking this is because I was writing a little widget to emulate the directory browsing web servers can be configured to do when browsing to a directory, eg:

Directory Listing For /shared/git/blogExamples/quiz/ - Up To /shared/git/blogExamples

FilenameSizeLast Modified
   functions.cfc1.3 kbSat, 04 May 2013 11:43:36 GMT
   manualtests.cfm1.3 kbSat, 04 May 2013 11:43:10 GMT
   puzzle.cfm1.1 kbSat, 04 May 2013 10:53:24 GMT
   testAdam.cfc0.1 kbSat, 04 May 2013 11:10:16 GMT
   testDale.cfc0.1 kbSat, 04 May 2013 11:18:54 GMT
   testMatt.cfc0.1 kbSat, 04 May 2013 11:17:58 GMT
   testWinston.cfc0.1 kbSat, 04 May 2013 11:18:58 GMT
   tests.cfc1.9 kbSat, 04 May 2013 11:31:12 GMT

Apache Tomcat/7.0.23

I needed to do this because I'm running PHP on the built-in web server on my work PC, and the build-in web server doesn't have this feature and I needed to emulate it. It's all detailed yesterday's article.

Whilst writing my CFML code, I wrote my version of isWithinWebroot() in such a way that the URL was hackable to allow access to directories and files outside the web root, which is "naughty".

I worked out a (seemingly) foolproof way of doing it, but it took some head-scratching and in the meantime I posted the puzzle article to see how other people would handle it, to get inspiration. I finally got around to looking at the code today (I did not look at it at the time, as I did not want my own efforts to be influenced by the other "competitors").

Here's the code for all four solutions (ie: including my own one), wrapped up in a CFC ready to be unit tested:

And here's the unit test suite I contrived to try to defeat all of them (which I could). Note I summarise these tests below, so don't bother poring over them if you don't want to. That said, they're quite simple:

Those tests are all "abstract", and require extending CFCs to actually work, I have created four sub CFCs thus:

That's testAdam.cfc, and I have similar CFCs for testing matt, dale and winston too. It means I can add generic tests to tests.cfc, and each of matt(), dale(), winston() and adam() will be tested.

Here's a summary of the tests and the results:

Test Example Valid? Reason for test Notes
testCurrentFilePathC:\apps\adobe\ColdFusion\10\cfusion\wwwroot\shared\git\blogExamples\quiz\tests.cfcyesJust in case a FILE path defeated the function
testCurrentDirectoryPathC:\apps\adobe\ColdFusion\10\cfusion\wwwroot\shared\git\blogExamples\quiz\yesThis is just the baseline test
testCurrentDirectoryPathNoTrailingSlashC:\apps\adobe\ColdFusion\10\cfusion\wwwroot\shared\git\blogExamples\quizyesJust in case the tests relied on a certain format
testWebrootC:\apps\adobe\ColdFusion\10\cfusion\wwwroot\yesJust in case the function failed right in the web root
testWebrootNoTrailingSlashC:\apps\adobe\ColdFusion\10\cfusion\wwwrootyesJust in case the function failed right in the web root
testCfRootC:\apps\adobe\ColdFusion\10\cfusionnoDefinitely an invalid directorywinston() errors on this
testRootC:\noAgain, definitely invalidwinston() errors on this
testValidRelativeDirectoryPathC:\apps\adobe\ColdFusion\10\cfusion\wwwroot\shared\git\blogExamples\quiz\..yesValid but not obvious
testInvalidRelativeDirectoryPathC:\apps\adobe\ColdFusion\10\cfusion\wwwroot\..noInvalid but not obvious. And the case that initially tripped me upDefeats matt(), dale() and winston()
testFakeDirectoryPathC:/fake/fake/fake/fake/fake/fakenoI figured this would defeat Matt's code due to a logic error in it (*)Defeats matt()
testHackC:\windows\C:\apps\adobe\ColdFusion\10\cfusion\wwwroot\noThis is a bit mean, but it defeats Dale's code (**)adam() errors, defeats dale()

(*) the logic error in Matt's code is that he just checks the number of sub-directories being more than those of the web root, which is not a very robust approach.
(**) Dale checks for the presence of the web root in the passed-in path, but doesn't check that it comes at the beginning of the passed-in path.

So all of the functions worked pretty well (matt's more by coincidence than anything else though, I'm sorry to say), and covered the obvious possibilities well. None of them were perfect, and all of them could be defeated after a fashion. I kinda don't mind that my function will actually error if it's fed garbage... garbage in, garbage out. But it should have occurred to me to error trap file operations. That was slack of me.

I'm gonna give Dale a bonus beer for being the most robust of them (I think a false positive is better than an error for the sake of this exercise).

So that's one beer for each of Matt and Winston; two for Dale. I think at least Matt is going to be at cf.Objective() to claim his? Youse other fellas will have to wait, but hit me up if/when we cross paths!

Cheers all. I enjoyed this exercise.

If anyone else fancies submitting other solutions, pls do.