4
\$\begingroup\$

I have a web application to manage computers. Every registered computer is locally querying the latest information and updates the database with it in a 10 minute cycle.

But the computer is only allowed to update its information and nothing else. Currently I'm identifying the computer like this:

I get its hostname from the "remote_addr" variable

string senderHostname = (Dns.GetHostEntry(Request.ServerVariables["remote_addr"]).HostName).Split('.')[0];

Then I compare the hostname of the sender computer with the hostname of the computer which should be updated.

if (senderHostname.Equals(targetHostname, StringComparison.InvariantCultureIgnoreCase)) {
 // allowed
}
else {
 // forbidden
}

If they are the same, then I'm allowing the update, otherwise not.

I know that this code is very very ugly and I would be interested in a best practice to do this.

All the code:

protected void Page_Load(object sender, EventArgs e)
{
    try
    {
        clientMachineName = (Dns.GetHostEntry(Request.ServerVariables["remote_addr"]).HostName).Split('.')[0];
    }
    catch (Exception)
    {
        clientMachineName = "n/a";
    }

    if (!String.IsNullOrEmpty(Request.QueryString["status"]) && !String.IsNullOrEmpty(Request.QueryString["value"]))
    {
        String macAddress = Request.QueryString["status"];
        String statusCode = Request.QueryString["value"];

        if (clientMachineName.Equals(getHostName(macAddress), StringComparison.InvariantCultureIgnoreCase))
        {
            ComputerDbCommunicator communicator = new ComputerDbCommunicator();
            communicator.insertIntoComputerDb("UPDATE [Computer] SET [statuscode] = " + statusCode + " WHERE [macaddress] = '" + macAddress + "'");

            communicator.closeConnections();
            communicator = null;

            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write(statusCode);
            Response.Flush();
            Response.End();
        }
        else
        {
            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write("Operation not allowed");
            Response.Flush();
            Response.End();
        }
    } 
    else if (!String.IsNullOrEmpty(Request.QueryString["status"]))
    {
        String macAddress = Request.QueryString["status"];
        String statusCode = null;
        ComputerDbCommunicator communicator = new ComputerDbCommunicator();
        SqlDataReader dr = communicator.queryComputerDb("SELECT [statuscode] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
        if (dr.HasRows)
        {
            while (dr.Read())
            {
                statusCode = dr[0].ToString();
            }
            dr.Close();

            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write(statusCode);
            Response.Flush();
            Response.End();

            communicator.closeConnections();
            communicator = null; 
        }
    }
}

protected String getHostName(String macAddress)
{
    String hostName = null;
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    SqlDataReader dr = communicator.queryComputerDb("SELECT [hostname] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
    while (dr.Read())
    {
        hostName = dr[0].ToString();
    }
    dr.Close();

    communicator.closeConnections();
    communicator = null;

    return hostName;
}
\$\endgroup\$
4
  • \$\begingroup\$ Can you add some contextual code (ideally copy-pasting the entire function or class to the bottom of your question)? \$\endgroup\$
    – Pimgd
    Jan 26, 2015 at 13:10
  • \$\begingroup\$ added the code @Pimgd \$\endgroup\$
    – soumer
    Jan 26, 2015 at 13:23
  • \$\begingroup\$ owasp.org/index.php/SQL_Injection \$\endgroup\$
    – RobH
    Jan 26, 2015 at 14:27
  • 1
    \$\begingroup\$ Have you checked the Request.UserHostName property? Never used it but it sounds like what you want. \$\endgroup\$
    – RobH
    Jan 26, 2015 at 14:33

2 Answers 2

3
\$\begingroup\$

Instead of reading the querystrings multiple times, you should consider to read them once and store them in a variable.


You should use Parameter's for your query to avoid sql injection.


Instead of Request.ServerVariables["remote_addr"]).HostName you should give Request.UserHostAddress a try.

See also: https://stackoverflow.com/q/13994582/2655508


Using a guard clause for the case that macAddress is null or empty will add readability to your code.


The selecting and updating of the statuscode should be extracted to separate methods.


After implementing the changes mentioned above (except the parameterized query) and the change @Pimgd suggested we will get

protected void Page_Load(object sender, EventArgs e)
{
    try
    {
        clientMachineName = (Request.UserHostAddress.HostName).Split('.')[0];
    }
    catch (Exception)
    {
        clientMachineName = "n/a";
    }

    String macAddress = Request.QueryString["status"];
    String statusCode = Request.QueryString["value"];

    if (String.IsNullOrEmpty(macAddress)) { return; }

    if (!String.IsNullOrEmpty(statusCode))
    {
        if (clientMachineName.Equals(getHostName(macAddress), StringComparison.InvariantCultureIgnoreCase))
        {
            UpdateStatusCode(statusCode);
            CreateResponse(statusCode);
        }
        else
        {
            CreateResponse("Operation not allowed");
        }
    }
    else
    {
        statusCode = GetStatusCode(macAddress);
        if (!String.IsNullOrEmpty(statusCode))
        {
            CreateResponse(statusCode);
        }
    }
}

protected void UpdateStatusCode(string statusCode)
{
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    communicator.insertIntoComputerDb("UPDATE [Computer] SET [statuscode] = " + statusCode + " WHERE [macaddress] = '" + macAddress + "'");

    communicator.closeConnections();
    communicator = null;
}

protected string GetStatusCode(String macAddress)
{
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    SqlDataReader dr = communicator.queryComputerDb("SELECT [statuscode] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
    if (!dr.HasRows) { return String.Empty; }

    string statusCode = String.Empty;
    while (dr.Read())
    {
        statusCode = dr[0].ToString();
    }
    dr.Close();

    communicator.closeConnections();
    communicator = null;

    return statusCode;
}

protected void CreateResponse(string response)
{
    Response.Clear();
    Response.ClearHeaders();
    Response.AddHeader("Content-Type", "text/plain");
    Response.Write(response);
    Response.Flush();
    Response.End();
}

protected String getHostName(String macAddress)
{
    String hostName = null;
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    SqlDataReader dr = communicator.queryComputerDb("SELECT [hostname] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
    while (dr.Read())
    {
        hostName = dr[0].ToString();
    }
    dr.Close();

    communicator.closeConnections();
    communicator = null;

    return hostName;
}
\$\endgroup\$
3
\$\begingroup\$

You've got some duplication in there;

Perhaps you can create a function with a string argument that will handle response writing?

protected void CreateResponse(string response){
    Response.Clear();
    Response.ClearHeaders();
    Response.AddHeader("Content-Type", "text/plain");
    Response.Write(response);
    Response.Flush();
    Response.End();
}

To me that seems far more concerning than the way in which you retrieve the hostname.

\$\endgroup\$
1
  • \$\begingroup\$ ok, that would help with the "Don't Repeat Yourself" problem. But I want a reliable way to identify the computer who is sending me the request. And not this ugly "remote_addr" parsing. \$\endgroup\$
    – soumer
    Jan 26, 2015 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.