Sunday, March 13, 2016

What is wrong with this code?

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. 

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();
<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>

Question 3

From /myapp/logout.asp
' Get User ID. if found then log the user out of My App
if getCookie("Myapp", "iUserID")<>"" then
  Set mySession = Server.CreateObject("Myapp.Session")
  mySession.SessionID = getCookie("Myapp", "SessionID")
  if mySession.Delete(connStrDB) then
   writeCookie "Myapp", "", "iUserId"
   writeCookie "Myapp", "", "sUserName"
   SafeRedirect "/myapp/logout.asp?Success=True"
  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"
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.

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://"
It is also worth noting that "https://" is not stripped out and potentially can be used as a bypass too.

Question 7

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 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

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
 ' not allowed
 errorMessage = ERROR_PREFIX & " IP address is not allowed"
End if

Answer 9

An attempt is made to only allow IP addresses from the range (from to 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

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:,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:
       customerIDs = Convert.ToString(Request.QueryString[CustomerIDParam]);
       if (customerIDs == null || !Regex.IsMatch(customerIDs, "^[0-9,]+$"))

Question 12

Answer 12

I can see several potential issues with this request URL:

  1. 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)
  2. 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 try a different URI scheme (ftp://, file://, telnet:// or even svn:// ;) )

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?

Thursday, March 3, 2016

The case of slow API connections and TCP retransmission

For years I've been a big fan of Mark Russinovich's "The case of" blog posts. So I decided to do a similar post this time. A couple of months ago my team was troubleshooting an issue related to slow responses from a 3rd party API. This particular API is located in the US and our code runs in Australia. Typically we saw response times of a few hundreds of milliseconds (which includes time to establish connection, round-trip to a different continent and back plus processing time). Everything worked well until suddenly one day our monitoring systems picked up a significant increase in request processing time. It looked like this:

message time elapsed time (ms)
12/18/2015 10:15:31.938 +1100 10085
12/18/2015 10:15:24.107 +1100 10114
12/18/2015 10:15:17.490 +1100 9924
12/18/2015 10:15:11.704 +1100 9991
12/18/2015 10:15:05.796 +1100 9953
12/18/2015 10:14:50.723 +1100 9964
12/18/2015 10:14:49.815 +1100 9911
12/18/2015 10:14:40.021 +1100 10140
12/18/2015 10:14:29.147 +1100 10151
12/18/2015 10:14:28.646 +1100 9937

Everything still worked fine but instead of sub-second responses we saw requests taking 9-10 seconds to complete. Further investigation was required. We performed the usual troubleshooting steps but still could not figure out what was going on there. We had to go deeper and deeper in our analysis - eventually all the way to the network packet capture. In fact, it's the packet capture that gave us the first hint of what the problem was. We saw a lot of the TCP retransmissions.

Two things were clear for us now.
Firstly, we noticed that only the SYN packets had delivery problems and had to be retransmitted. SYN packet is the first packet of a 3 packet "handshake" used to establish a TCP/IP connection. We saw that once the connection was established there were no more retransmissions during the session/data transfer.

Secondly, we could see where all those extra seconds were coming from!
See how there is a 3 seconds difference between the initial SYN packet (packet 22921) and the retransmission in line 23003 which is 3 seconds after (103.53… and then 106.54…)

And then we retransmit again 6 seconds later (packet 23087).
After that the connection is finally established but we’ve just lost 3+6=9 seconds during the TCP handshake.

Another interesting observation was that when we retransmit for the second time (packet 23087) we remove the ECN and CWR flags.

We performed several packet captures and it became clear that our SYN packets were not reaching  the destination and we had to retransmit them (or their SYN/ACK packets were not reaching us)

SYN packet retransmission (at least on Windows) by default works like this:

“The retransmission timer is initialized to three seconds when a TCP connection is established. However, it is adjusted on the fly to match the characteristics of the connection by using Smoothed Round Trip Time (SRTT) calculations as described in RFC793. The timer for a given segment is doubled after each retransmission of that segment. By using this algorithm, TCP tunes itself to the normal delay of a connection”

This is where we get 3 seconds (initial retransmission delay) plus 6 seconds (3 seconds doubled for the second retransmission).

Also given that “Max SYN Retransmissions” is set to 2, the system will only retransmit the SYN packet twice hence the ~9 seconds delay we see in the worst cases. The initial retransmission timer value is set in the "Initial RTO" parameter (see the screenshot below). To test this theory we decided to change this value from 3 seconds to 2 seconds:

This can be achieved by running this command:
netsh int tcp set global initialRto=2000

Once this change went live, straight away we saw request processing time decreasing from 9-10 seconds down to ~6 seconds. We knew we were on the right track.

Another suspicious finding (as mentioned above) was that most of the SYN packets with the ECN and CWR flags were dropped while SYN packets without these flags were going through.

ECN (Explicit Congestion Notification) is an interesting protocol extension defined in the RFC 3168. In the TCP/IP world the standard way for the receiver to "notify" sender of network congestion is to drop packets. This behaviour obviously can have a significant impact on the overall network performance. ECN (when supported and negotiated by both ends) allows signalling/notification of network congestion to happen without dropping packets.

Windows had ECN for TCP support since Windows Server 2008 and Vista (but it was disabled by default). But it is enabled in Windows 2012. (Linux passively supports ECN - will negotiate if asked by the other end)

ECN support has improved significantly since the introduction 15 years ago but apparently some issues still exist.

The next step for us was to try to disable ECN to see if this was the culprit.
ECN capability can be turned off by executing this command:

netsh int tcp set global ecncapability=disabled

Once this change was applied, all TCP Retransmissions disappeared and request processing time was back to a few hundred milliseconds.

We contacted the API vendor and they reassured us that their end had proper ECN support. The fact that not all of the SYN packets with ECN flag had this issue (but most of them), led us to believe that we saw a "Path-dependent connectivity dependency" as described on slide 6. This is also indirectly supported by the fact that some of the BGP routes changed roughly around the same time when we started experiencing this issue.

We were happy to see this issue resolved. Hope this blog post will help someone in a similar situation.

Keywords: Max SYN Retransmissions, maxsynretransmissions, slow connection, ECN, ecncapability, TCP retransmission