CodeSOD: Secure Mentorship

Sophia entered the software development industry through an "alternative" path. In Germany, they have a tradition of Ausbildung. Instead of going to university, you focus on on-the-job training with some classes mixed in. There are still exams and all the traditional academic trappings, but with one huge bonus: you're working closely with an Ausbilder- an experienced industry veteran who can show you the ropes.

Sophia's Ausbilder was named Tim, and at the company she was working at, he was widely hailed as a genius. He had designed their entire web system from the ground up in Java, which given that every other developer at the company only worked in Delphi, that made him a pretty unique talent. Everyone was pretty convinced he was brillant.

Sophia, who at this point was fairly novice in the industry, was excited to work with him. That lasted about five minutes.

It didn't take long to discover that the entire system was constructed around god classes that just bundled a pile of unrelated functionality into an undebuggable blob. Sophia has lots of things she wants to share, but today we want to look at one specific class, helpfully called ModuleClient:

public class ModuleClient { // ... we are at line 1091. Here comes the constructor... public ModuleClient(HttpServletRequest request, HttpSession session) { // ...some stuff String ip = request.getRemoteHost(); if ((session.getAttribute("ip") != null)) { if (!ip.equals(session.getAttribute("ip"))) { log(Level.ERROR, "session.invalidated, session.ip != request.ip : " + session.getAttribute("ip") + "!=" + ip); String tempIp = (String) session.getAttribute("ip"); int ind = tempIp.lastIndexOf('.'); tempIp = tempIp.substring(0, ind); if (!ip.startsWith(tempIp)) { log(Level.ERROR, "It is not a subnet, session will be canceled " + (tempId) + " " + session.getAttribute("ip") + "!=" + ip); session.invalidate(); } else { log(Level.ERROR, "It is a subnet, ignore error. " + tempIp) } } } } //... around 10k more lines of code}

Comments added by Sophia.

One line into this constructor, we already have an error. request.getRemoteHost() returns a hostname, not an ip- getRemoteAddr is the correct method. Now, in many cases, this may still return an IP address, but it's not guaranteed. It depends on whether the runtime host is willing to resolve hostnames, but as a best practice, if you want an address ask for an address.

So, we get an ip that isn't an ip, and check the web server's session variable to see if there is an "ip" value stored there. If there is, we check to see if it matches the one attached to this request.

This, I suspect, is an attempt at security. Assume that multiple requests that have the same session token but come from different IPs are invalid. But wait, there's more.

We split the string to cut the last segment of the IP address, e.g., 192.168.1.20 becomes 192.168.1. Then we compare the IP address attached to the request to the first three segments of the IP attached to the session- if they match, we assume they're on the same subnet.

Now, I'm no expert on networking, but that's not how subnets actually work. Or, more to the point, they're assuming that every network has a subnet mask of 255.255.255.0- which is a terrible assumption. I suspect that if they tested this code at all, they tested it using addresses from their own internal network and didn't think at all about how internet addresses actually work.

The real magic of this code is that it does nothing if session.getAttribute("ip") returns null- which, Sophia has not found anywhere in the code where the IP session attribute is getting set, which means this code actually doesn't do anything ever.

"I guess," she writes, "that is the actual security part of it."

Sophia was in this program to learn about how to do software development- which she's at least getting some great examples of what not to do.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!