Every now and then I run training sessions for the dev teams, where we go through the small code samples and I ask audience a question - "what's wrong with this piece of code?". Sometimes it could be a trivial security blunder that leads to a SQL injection or XSS. In other cases the answer could be less obvious (e.g. a security weakness). And sometimes the question should really be "What CAN go wrong with this code" depending on certain implementation details. The purpose of these training sessions is to raise security awareness by demonstrating real security issues that I came across during my career (originating from multiple sources - code reviews, bug bounties, social media etc)
I would like to share these samples with you and hopefully together we can make Internet a safer place. Please let me know if you decide to use any of these samples as part of your own training sessions - I'd be very keen to know how it goes and to receive any feedback.
Note for security professionals - these examples are very simple. They are not meant to be hard, they are just a starting point and in most cases I seek a nearly immediate response from the audience.
"Forewarned is forearmed!"
Question 1
"SELECT ItemID, CONVERT(varchar(20), SubmitDate ,6) As SubmitDate FROM tblItem
WHERE LoginID = " & CStr(getCookie("Myapp", "iUserID")) & "
ORDER BY SubmitDate desc"
Answer 1
Potential SQL injection via getCookie("Myapp", "iUserID"). Cookies = untrusted input. Avoid constructing dynamic SQL statements (string concatenation) - this coding style often leads to SQL injections.
Also it looks like iUserID is an Integer. I always recommend (where possible) to constrain input for length, range, format, and expected data type (i.e. where we know expected type upfront). By wrapping getCookie() in either CInt() or CLng() before feeding it into the SQL statement we can essentially eliminate the risk of SQL injection.
Also it looks like iUserID is an Integer. I always recommend (where possible) to constrain input for length, range, format, and expected data type (i.e. where we know expected type upfront). By wrapping getCookie() in either CInt() or CLng() before feeding it into the SQL statement we can essentially eliminate the risk of SQL injection.
A second issue to consider (especially if iUserID is an integer) is that it might be possible to supply another user ID and bypass security controls to gain access to someone else's data.
Question 2
Code behind:
if(Request.QueryString.Get("ver") != null)
ItemVersion.Text = Request.QueryString.Get("ver").ToString();
Page:
<asp:label id="ItemVersion" runat="server"></asp:label>
Answer 2
XSS (cross-site scripting) via the "ver" parameter.
Label.Text is unfortunately unsafe - by default it sets HTML markup. An attacker can supply a value/payload similar to this: ver=<marquee>xss</marquee> or ver=<script>alert(1)</script>
Label.Text is unfortunately unsafe - by default it sets HTML markup. An attacker can supply a value/payload similar to this: ver=<marquee>xss</marquee> or ver=<script>alert(1)</script>
Question 3
From /myapp/logout.asp
' Get User ID. if found then log the user out of My App
if getCookie("Myapp", "iUserID")<>"" then
[skipped]
Set mySession = Server.CreateObject("Myapp.Session")
mySession.SessionID = getCookie("Myapp", "SessionID")
if mySession.Delete(connStrDB) then
writeCookie "Myapp", "", "iUserId"
writeCookie "Myapp", "", "sUserName"
Response.Buffer
SafeRedirect "/myapp/logout.asp?Success=True"
Response.End
[skipped]
end if
end if
Answer 3
It is possible to cause a denial of service - to log out any session. getCookie("Myapp", "SessionID") is untrusted input and the code never checks that this SessionID actually belongs to this user (iUserID). If session IDs are easy to guess (e.g. integers) then it is trivial to iterate through and log out all active users.
Question 4
sUploadedFileName = Mid(fUpload.UserFilename, InstrRev(fUpload.UserFilename, "\") + 1)
If InStr(sUploadedFileName, ".gif") <= 0 And Instr(sUploadedFileName, ".jpg") <= 0 Then
DisplayErrorPage "Only accept GIF or JPEG files, please try again"
Response.End
End If
Answer 4
The idea is to allow uploading only *.gif and *.jpg files. But it is possible to upload any file as long as it has ".jpg" or ".gif" somewhere in the name. E.g. MyEvilFile.jpg.SomeOtherText.MyExt
Weak validation of this kind often leads to hackers being able to upload executable files (or shells) - especially if uploaded "images" are accessible from the web (i.e. the Upload directory is under the web root) - e.g. http://mysite.com/UploadedImages/MyEvilFile.jpg.php
Weak validation of this kind often leads to hackers being able to upload executable files (or shells) - especially if uploaded "images" are accessible from the web (i.e. the Upload directory is under the web root) - e.g. http://mysite.com/UploadedImages/MyEvilFile.jpg.php
Question 5
sCustMediaDir = sCustHomeRoot & "\" & Replace(getCookie("Myapp", "sCustName"), " ", "_") & "\media\"
sNewFileName = sCustMediaDir & sUploadedFileName
fUpload.Form("dlgFile").SaveAs sNewFileName
Answer 5
The problem is in the way how we construct the sCustMediaDir string. sCustName is untrusted input and it can contain anything (including paths like "..\..\mypath\"). As a minimum this bug allows the attacker to rewrite files that belong to other customers (by changing the sCustName cookie value to "..\customer2"). Also if file system permissions for the web application user are weak and allow writing outside of the sCustHomeRoot directory then it could be possible to create or overwrite other files on this drive (the "\media\" part will make it less useful though)
Question 6
'if website address contains http:// strip it out
if inStr(strWebsiteAddress, "http://") > 0 then
strWebsiteAddress = Right(strWebsiteAddress, len(strWebsiteAddress) - 7)
End if
Answer 6
This attempt to strip out "http://" can be bypassed by supplying "http://http://mysite.com"
It is also worth noting that "https://" is not stripped out and potentially can be used as a bypass too.
It is also worth noting that "https://" is not stripped out and potentially can be used as a bypass too.
Question 7
http://mysite.com/embed.aspx?frameSrc=/mypath/campaign1.htm inside embed.aspx <iframe id="theFrame" name="theFrame" width="780" scrolling="auto" frameborder="no" border="0" scrolling="no" src="<% Response.Write(System.Web.HttpUtility.UrlEncode(Request.QueryString["frameSrc"])); %>" ></iframe>
Answer 7
An attempt is made to load local content (relative path) into an iframe. Unfortunately a developer here forgot that the value in frameSrc is untrusted and can be controlled by the attacker.
E.g. we can supply an external malicious page http://mysite.com/embed.aspx?frameSrc=//www.externalevilsite.com/evilpage.php which will be rendered/embedded into the web page. This approach can be leveraged in phishing campaigns etc
I would recommend to avoid referencing pages by name/URL directly and instead have a whitelist or a resource map, where each allowed page should have a corresponding ID associated with it.
E.g. if we have an internal map matching /mypath/campaign1.htm to "myresource123" then we can request it i na safe way as http://mysite.com/embed.aspx?resourceID=myresource123
E.g. we can supply an external malicious page http://mysite.com/embed.aspx?frameSrc=//www.externalevilsite.com/evilpage.php which will be rendered/embedded into the web page. This approach can be leveraged in phishing campaigns etc
I would recommend to avoid referencing pages by name/URL directly and instead have a whitelist or a resource map, where each allowed page should have a corresponding ID associated with it.
E.g. if we have an internal map matching /mypath/campaign1.htm to "myresource123" then we can request it i na safe way as http://mysite.com/embed.aspx?resourceID=myresource123
Question 8
isAdministrator = (getCookie("Myapp", "iEditedBy") = "1")
Answer 8
A classic insecure cookie handling vulnerability. The presence of a cookie iEditedBy with a value of "1" means you are an admin! And apparently these types of issues are quite common.
Question 9
' check the incoming remote address
if 0 < Instr(Request.ServerVariables("REMOTE_ADDR"), "10.11.12") then
' all OK
[skipped]
else
' not allowed
errorMessage = ERROR_PREFIX & " IP address is not allowed"
displayErrorMessage(errorMessage)
End if
Answer 9
An attempt is made to only allow IP addresses from the 10.11.12.0/24 range (from 10.11.12.0 to 10.11.12.255). Unfortunately the way this filtering is implemented will also allow IP addresses that follow this pattern: xxx.10.11.12, which is most likely undesired.
Question 10
http://www.somesite.com/Search.asp?query=SELECT+cname%2C+sname%2C+description%2C+pid%2C+picture+FROM+tblCategory+c%2C+tblSubcategory+s%2C+tblItem+i+WHERE+i.cid%3D5+AND+i.sid%3D31+AND+i.cid%3Dc.id+AND+i.sid%3Ds.id+ORDER+BY+cname%2C+sname%2C+description%2C+pid
Answer 10
This is just for the giggles. But this is a real example (real web site) that I came across a few years ago. I wouldn't even call it a SQL injection. It is more than that. These guys allow anyone to execute any SQL statement on their web site. And surprisingly they are not alone. I see this approach time and time again in the old ASP and PHP based web sites.
In fact, you can run a Google search (aka Google dork) similar to this one to see what I mean: inurl:"query="+inurl:SELECT+inurl:FROM+inurl:WHERE+inurl:"order by"
Another web site had 2 separate parameters for the "where" clause and the "order by" part of the query but the "idea" remains the same:
In fact, you can run a Google search (aka Google dork) similar to this one to see what I mean: inurl:"query="+inurl:SELECT+inurl:FROM+inurl:WHERE+inurl:"order by"
Another web site had 2 separate parameters for the "where" clause and the "order by" part of the query but the "idea" remains the same:
www.somesite.com/mypath?where_clause=+item_status_id+in+(select+status_id+from+item_statuses+where+category_id=1)+and+item_type_id+in+(1,2)&order_by_clause=ORDER+BY+create_date+DESC
Question 11
customerIDs = Convert.ToString(Request.QueryString[CustomerIDParam]);
if (customerIDs == null || !Regex.IsMatch(customerIDs, "[0-9,]+"))
Answer 11
The weakness is that this RegEx checks that we have digits or commas but it doesn’t prevent an attacker from entering other characters (like an apostrophe as an example) as long as there IS at least one digit or comma.
Once accepted these values are fed into a SQL query. A defence in depth principle dictates that we should try to defend our systems at each level. The application level is certainly capable of performing some input parameter validation. In order to fix this particular weakness we can make a regex tighter:
Once accepted these values are fed into a SQL query. A defence in depth principle dictates that we should try to defend our systems at each level. The application level is certainly capable of performing some input parameter validation. In order to fix this particular weakness we can make a regex tighter:
customerIDs = Convert.ToString(Request.QueryString[CustomerIDParam]);
if (customerIDs == null || !Regex.IsMatch(customerIDs, "^[0-9,]+$"))
Question 12
www.somesite.com/mypath?stdTextCol=&linkTextCol=&p1imageLoc=http://db1.img.somesite.com/i/123.jpg&p1thumbImageLoc=http://db2abcde01:83/i/456.jpg
Answer 12
I can see several potential issues with this request URL:
- We are allowing images to be loaded from a different domain (p1imageLoc and p1thumbImageLoc parameters). An attacker can supply their image to alter the look of the web site (and potentially use this in a phishing style attack)
- Look at the difference in the way how image server is specified in p1imageLoc and p1thumbImageLoc. In the first case this is just a normal domain name but in the second case we see an internal server name followed by a non-standard port (!!!). What happens next really depends on the implementation.
As a minimum we leak information that an attacker might find useful (as part of their reconnaissance effort). They now know that there is an internal server called db2abcde01 that runs a web service on port 83.
But the situation can actually be worse. This URL can potentially give an attacker a leg into the internal network (again - depending on how much information is actually returned back to the attacker - e.g. as part of the detailed error messages).
E.g. an attacker may try to perform a port scan by iteration through the port numbers (83, 84, 85 etc)
Or execute an admin request (db2abcde01:83/admin/SensitiveOperation) - either unauthenticated by themselves or embedding this URL somewhere waiting for a logged in person with admin privileges to inadvertently execute this request.
Or an attacker may try to find other servers on the internal network. What if they try db2abcde02? Or SuperSecretServer01?
Final words
12 questions should be enough for the first blog post of the series. I've got a lot more examples and I am sure this is not the last post of this kind. I might even try something different next time. I can post just questions (avoiding the most trivial ones) and let the audience try their "hacker" thinking and then publish the answers a week later. What do you think?
