CodeSOD: The Powerful Parent

As we've explored recently, developers will often latch onto something they pick up in one language and carry it forward with them into others. Kerry still is working with the co-worker who has" odd ideas about how things should work. At is turns out, this developer is also a Highly Paid Consultant, which we just discussed yesterday.

The latest problem Kerry found was in a display grid. It lists off a bunch of information linked to the user's account, and each entry on the grid has a little plus sign on it to display further details. What, exactly, appears on that grid is tied to your account. It's also worth noting that this service has a concept of corporate accounts- a "parent" account can administer entries for all their child accounts.

When someone clicks that little plus sign, we need to fetch additional details, regardless of whether or not they're a corporate parent account or not. So how exactly does the C# controller do this?

 long? acctId = null // if the user is a corporate dealer fetch claims without using an account Id if (User.IsInRole("Parent_User") || User.IsInRole("Parent_User_2")) { acctId= null; } else { acctId= fetchAcctId(); } Model model = _service.fetchDetails(itemId, acctId);

The first thing that's worth noting is that we already have the account ID value- we needed it to display the grid. So now we're fetching it again" unless you're one of the parent user roles, which we make null. It was already null on the first line, but we make it null again, just for "clarity". The comment tells us that this is intentional, though it doesn't give us any sense as to why. Well, why do we do that? What does that fetchDetails method actually do?

 public DetailViewModel fetchDetails(long itemId, int? acctID) { DetailViewModel results = new DetailViewModel(); try { using (var ctx = new DBContext()) { DetailViewInternal detailData = ctx.DetailViewInternals.SingleOrDefault( x => x.itemID == itemId && (x.FacilityID == acctID || acctID == null) ); results.CustomerFirstName = detailData.FirstName; results.CustomerLastName = detailData.LastName; ...snip out a lot of copying of info... } catch (Exception ex) { _log.Debug($"Error getting detail.. exception {ex.Message}"); } return results; }

The key logic is the lambda that we use to filter: x => x.itemID == itemId && (x.FacilityID == acctID || acctID == null)

For "normal" users, their acctID must match the FacilityID of the data they're trying to see. For "parent" users, it doesn't, so we pass a null to represent that. We could, of course, pass a flag, which would make sense, but that's clearly not what we're doing here. Worse, this code is actually wrong.

Specifically, a "parent" account has a specific list of children- they actually have a set of acctIDs that are valid for them. With this approach, a maliciously crafted request could actually pass an itemID for a different company's data (by guessing ID values, perhaps?) and fetch that data. The "parent" privileges give them permission to see anything if they're clever.

What makes this worse is that the grid code already does this. Instead of reusing code, we reimplement the authorization logic incorrectly.

With that in mind, it's barely worth pointing out that there's no null check on the query result, so the results.CustomerFirstName = detailData.FirstName line can throw an exception, and our exception just gets logged in debug mode, without any of the stack trace information. I'm pointing it out anyway, because there's not many things more annoying than a useless log message. This also means that the exception is swallowed, which means there's no way for the UI to present any sort of error- it just displays an empty details box if there are any errors. Enjoy puzzling over that one, end users!

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!