RickStrahl / Westwind.plUploadHandler

An ASP.NET base HttpHandler library to handle plUpload content on the server.
28 stars 19 forks source link

Possible directory traversal vulnerability #2

Closed rdogmartin closed 11 years ago

rdogmartin commented 11 years ago

A security company alerted me to the existence of a directory traversal vulnerability in my code, and that got me to thinking it might also exist in Westwind.plUploadHandler. Sure enough, I could repro the issue in my slightly modified version of your code with this HTTP request:

POST /dev/gs/gs/handler/upload.ashx?aid=20 HTTP/1.1 Host: localhost User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,/;q=0.8 Accept-Language: en-US,en;q=0.5 Accept-Encoding: gzip, deflate DNT: 1 Referer: http://localhost/dev/gs/default.aspx?g=task_addobjects&aid=20 Content-Length: 344 Content-Type: multipart/form-data; boundary=---------------------------152811697411241 Cookie: gsp_g_ui_tmpl_cookie=0; gsp_left-pane_g_ctl00_media=293px; gsp_right-pane_g_ctl00_mediaCR=842px; .ASPXANONYMOUS=we0eebRAzgEkAAAAOGE2M2RjNzUtMzNiOS00NmM0LWI5OTYtMTQ2YjJjN2E2MDdk0; ASP.NET_SessionId=uugjpatk3ndowm3ngzme1cpz; .ASPXAUTH=03DBD52FFA5B0B5510ECA0652334E434B97441618462774140362ABD2CB6023538F0E3DDD616B9991C768D361814E1815C3D192EDECC557C799B8E462739A9D516043CDBBA67D78053625345048769802C9C2DD6AE7FF4F3193DF24E2CA4E4567B7415328F8380C216CB8308BBAF251886BE32C6EC1DAD7C984A8B0F22840DE5; GSP=F061A15F62979145FEA5E9DD8A65DE0BC0DA194AF064773737B2362975ACF955A882D2F85AC5919BA119F70AED46DB7CB989C1A40CE5A6614DDF01EBC1DC7F7D8C5AE370180E2A65266D024C8E60B0E00C15D98CC6D629D5C8876EBC5929E7C26A2A8DC4F46AE31B052D07FA4A88FF102620254B41290FF6D797DC95FC631526 Connection: keep-alive Pragma: no-cache Cache-Control: no-cache

-----------------------------152811697411241 Content-Disposition: form-data; name="name"

......\p17oplsgg1b8m5668u7udr19rf4.txt -----------------------------152811697411241 Content-Disposition: form-data; name="file"; filename="badfile.txt" Content-Type: text/plain

Your server is now owned! -----------------------------152811697411241--

The key change from a normal upload is that I modified 'p17oplsgg1b8m5668u7udr19rf4.txt' to '......\p17oplsgg1b8m5668u7udr19rf4.txt', allowing me to store the file outside the intended destination directory. I believe one could use this to upload a file to many (most?) places on the server that the IIS app pool identity has write permission to.

RESOLUTION: I believe it's easily solvable by adding a check in plUploadBaseHandler.ProcessRequest(). Look for this line:

string fileName = Request["name"] ?? string.Empty;

Add this immediately after it:

// Check for directory traversal attack if (fileName.LastIndexOf('\') > -1 || fileName.LastIndexOf('/') > -1) { WriteErrorResponse("You do not have authorization to perform the requested action.", 403); return; }

Cheers, Roger Gallery Server Pro www.galleryserverpro.com

rdogmartin commented 11 years ago

Note: github apparently edited a key part of my post. What is showing as '......\' was originally '..slash..slash..slash'

RickStrahl commented 11 years ago

Thanks roger. Will take a look and see. I think checking the path for any existance for slashes should do it really, as the name should only be a the file ever. It should never include anything path related.

RickStrahl commented 11 years ago

So I took a look and what I saw is that the file did not upload into an invalid location for me. The display echo'd back the traversed URL though and so that was definitely problematic.

I've fixed the code now by explicitly normalizing the path when retrieving the filename both from the request content and from Request.Files. Using Path.GetFilename() to strip off any paths that might be there.

Change is implemented and checked in.

Thanks for your help, Roger.

rdogmartin commented 11 years ago

Nice fix - think I'll do the same thing in my code.