As longtime readers know, I am also a software developer (we can’t call them engineers in Canada for legal reasons).
I took over a complex codebase last year in a field I’m unfamiliar with, and have been working on features and improvements since then. This week I decided that it was time to rename a bunch of objects because I didn’t like the previous naming convention as it was confusing.
Tonight while testing a new feature, I accidentally clicked on the wrong thing and the application crashed. I inspected the stack trace and discovered that a method was missing. This was of course impossible because I had used a well-known Visual Studio extension to refactor my code and after all, it compiled and ran.
Upon closer inspection, it turns out that there was what seemed like an innocuous piece of code that I’d dismissed previously because the same Visual Studio extension told me that it is never used. 0 references. It said it right there.
One of the things I changed during this refactoring process was taking twenty distinct objects (numbered 1..20) and put them in a generic List<T>. What I had not counted on was that the “unused” code was actually called through .NET Reflection. Another method was building a string based on a database value, then invoking that as a method name in the code. Since I’d renamed a bunch of things, the code was failing because the method name was no longer there (it was one of the 20 objects I’d changed into a list). This code relies on what is effectively SQL injection.
It works like this: the database fetches a list of rows, and each row contains the name of a matching method in the code. It doesn’t take much effort to change that value in the database, because there’s literally a screen to do that in the app. The code performs a small string manipulation on that value to build a method name. For the sake of discussion, let’s call one example GetVehicles(). The database value is called “Vehicles” and this code slaps the word “Get” in front of that and … just runs it.
There’s nothing stopping a user from changing “Vehicles” to “Customer” or “User”. Yes the app will crash, but the great thing about .NET code is the stack trace. You can even dump out the memory if you want.
Please, if you’re writing code, don’t rely on MethodInfo.Invoke using values from a database table without fully vetting the inputs. This is just bad news. Database parameterization won’t save you.
Share your SQL injection stories in the comments below.
Photo by Alexandre Debiève on Unsplash.
Wow, this is, like, reverse SQL injection 😅
That is a scary thing to deal with. Glad nobody on the dev team I work with does things like that.
A former developer did something… fun? The code was designed as 3 different solutions. Solution 1 was a DLL that had shared methods used by solutions 2 and 3. Solution 2 was an Android app and solution 3 was a windows app. So far so good. If a shared thing (such as “validate username” to ensure the user was an AD user and had permissions to run the app), it went in the DLL and the Android app and windows app would both call the same function in the DLL. So if the company decided to change the domain name (which happened!), you update the DLL and copy it over and you are golden.
The developer decided at one point that a function that existed ONLY in the Android app would be beneficial to both. So, rather than porting the function over, they just took the DLL created by the android app and made it a reference to Solution 1’s DLL. Hit compile and things are good… all 3 apps compile fine… They could compile it over and over all 3 apps and no problems came up so into source control it goes.
Flash forward a year-ish and the developer, who was the sole support person for the apps and dll, is no longer with the company. I pull it out of source control (without the bin folder because why put waste into git, right?), I load up the DLL and hit compile and it fails because it is missing a reference. Do some digging and think to myself “oh, the android app must not use this DLL because the DLL needs the DLL from the android app.”. So check that code out, hit compile and… fails. It is missing the other DLL. Circular reference problem if you try to put all 3 projects into a single solution. Find the location of the live files, copy the DLL’s to a shared location and change the references and now we can compile each no problem and do bug fixing, but the code needs to be refactored… a lot.
As an aside to that, the code shouldn’t have made it into git with a circular reference problem like that. The code reviewer should have flagged it and said nope. Not entirely sure how that got past the code reviewer (which was likely me), but my guess is that there were a LOT of code changes (1000’s of lines of code changed) and it just got missed.
TL;DR version – don’t make circular references in your code. Ever. Future developers will hate your code if you do.
Comments are closed.