User Input In Sheep’s Clothing mvc,, code 0 comments suggest edit

We all know that it is bad bad bad to trust user input. I don’t care if your users are all ascetic monks in a remote monastery, do not trust their input. However, user input often likes to put on sheep’s clothing and disguise itself as something else entirely, such as the case with ViewState.

Another example of this is highlighted in the latest entry of his excellent series of ASP.NET MVC tips. In this post, Stephen Walther writes about how cookie values and server variables can be passed as parameters to action methods.

Immediately, commenters understably asked whether this was safe or not. One person went so far as to call this a security hole in controller actions.

However, to be extremely nitpicky, the security implication isn’t in passing server variables this way. That’s perfectly safe. The security implication is in trusting the values passed to an action method in the first place. If your action method makes decisions with security implications based on assuming that these values are accurate, then you have a potential security problem.

Keep in mind, many of these values can be spoofed with or without ASP.NET MVC. Many of the server variables should never be trusted no matter how you access them, whether via this technique or a call to Request.ServerVariables["variable_name"].

In fact, right there near the top of the MSDN documentation for the IIS Server Variables, it warns against trusting these values:

Some server variables get their information from HTTP headers. It is recommended that you distrust information in HTTP headers because this data can be falsified by malicious users.

In the same way, in a typical configuration for ASP.NET MVC, the parameter values for action methods come directly from the user in the form of the URL or Request parameters. This makes sense after all, since the whole point of a controller in the MVC pattern, according to Wikipedia, is to:

Processes and responds to events, typically user actions, and may invoke changes on the model.

The parameters to an action method generally correspond to user input. It’s really asking for trouble to have parameters in an action method that you consider to be anything but user input.

In the end, I don’t consider this a security flaw so much as a security lure. This is the type of thing that might tempt someone to do the wrong thing and trust these values. We will review this particular case and consider not passing in server variables into action methods, but doing so doesn’t really solve the fundamental issue. There are other ways to pass data to an action method (defaults on a route) that a developer might be tempted to trust (don’t trust it!).

Whether we change this or not, the fundamental issue is that developers should never trust user input and developers should always treat action parameter values as user input.

Tags: aspnetmvc , security

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.



18 responses

  1. Avatar for Clive Chinery
    Clive Chinery July 8th, 2008

    This article is an excellent wake-up call!

  2. Avatar for Troy Goode
    Troy Goode July 8th, 2008

    Hi Phil,
    Great explanation. In my opinion this is a "security hole" because server variables that have been [b]historically safe[/b] from user input (such as SERVER_NAME & LOGON_USER) [b]no longer are[/b] if accessed in this manner.
    The other server variables (such as POST_METHOD) also become considerably easier to modify (e.g.: you could do it from your iPhone), but as you said, they were always user input in the end anyway.

  3. Avatar for Troy Goode
    Troy Goode July 8th, 2008

    Oh and I fail at HTML. Evidently BlogEngine has polluted my mind with BBCode. =)

  4. Avatar for Francois Ward
    Francois Ward July 8th, 2008

    I'm the one who had posted the original comment after Stephen's story. At first, I was reconsidering hitting submit, because as you pointed out, a lot of the so called "server variables" are really coming from the request (let say, the browser's user agent), and can be spoofed, thus it isn't a security issue.
    But the reason I still posted it, was exactly what Troy Goode mentionned: many of the server variables come from the -server-, and -cannot- be spoofed (well, not by a client anyway). And now, using this method, they can.
    I honestly keep getting the impression that ASP.NET MVC is remaking a ton of mistakes that were the reason ASP.NET gained popularity in the first place over PHP and whatsnot. Remember the days where you could spoof any PHP variable from query string, because on most install they were mapped that way by default? This isn't -nearly- as bad, but its the same idea. As with PHP's formerly default configuration (its not anymore for a freagin reason!), programmers won't (and shouldn't!) need to worrie about the implementation unless they explicitely request to do so.
    In ASP.NET's web forms, with request validation at on, a user cannot use javascript to insert an extra element in a listbox, select it, and submit the form: the listbox is a wrapped control, and I shouldn't have to worrie about it. If however, I explicitely disable request validation, or use a normal SELECT element, then it is under my control, and I have to worrie about it. If I use an SQLCommand object with SqlParameters, I shouldn't have to worrie about SQL Injection. If I make my own strings, concatenated, then I should.
    So, to follow with this...If I use your wrapped version of the server variables, I should only have to worrie about server variable security (which ones are server, which ones are client), not that a cookie could overlap a server-side variable.
    May a suggest an Attribute based approach or something?
    Foo([Server]string HTTP_WHATEVER, [Cookie]string somestuff, [Auto] more stuff)
    That way if I want to shoot myself in the foot, I have to explicitely pick Auto, and it is self documenting to boot!

  5. Avatar for Troy Goode
    Troy Goode July 8th, 2008

    Attributes are an interesting way to tackle it, Francois. I just thought of another way:
    Modify the order of operations of ActionInvoker such that parameters are populated by Server Variables first (rather than going straight for the QueryString).
    That way this technique remains and is just as "safe" as accessing Request.ServerVariables["x"] today.

  6. Avatar for Francois Ward
    Francois Ward July 8th, 2008

    Indeed, that would work too. My gripe with the auto-mapping of -different types- of input remains even without the security issue. It just increase the surface area for possibilities of overlapping like crazy. You have to make sure POST, query string, cookies -and-server variable names never overlap? So not only you have to (as always) worrie about a malicious user not messing up the request, you -also- have to make sure no one else in your team did. So adding a totally unrelated page written by a totally different programmer has a -much- higher chance (the chance is always there regardless, but it is just multiplicated) of having a side effect.
    The automatic injection of parameters is really, really sweet. I love it. I just dislike having to worrie about collisions to such a large extent. Poor QA trying to figure out why a page that worked perfectly well yesterday doesn't today, even though it wasn't modified whatsoever, etc :)
    ASP.NET MVC isn't the Web Client Software Factory... it will be one of the default Visual Studio template eventually... people who eventually use it will do so after reading the marketing driven "magic" tutorials on the ASP.NET web site that try to show as much as possible that .NET does everything for you. Thats going to create one big, big mess, not unlike how InProc session and application recycling still does, to this day.

  7. Avatar for haacked
    haacked July 8th, 2008

    @Troy I disagree with your "historically safe" comment. It's still "historically safe" to access Request.ServerVariables["LOGON_USER"]. We haven't changed that.
    For example, you can also access this value like so: Request["LOGON_USER"]. Should we consider that a security hole as well now?
    I ?would consider it a hole in your code if your code chose to do that and trust that value. But why would you access it in that way when it's widely known there's a completely safe manner in accessing that value?
    We can only protect the developer from him/her self so much. At some point, the developer has to take responsibility for code that he or she writes.
    Again, I'm not saying we won't look into this, as there's also no good reason to pass SERVER_VARIABLES into an action method as far as I can think of right now. I'm just saying I don't think the reasoning here is fully valid to call it a security "hole" as opposed to "lure".
    As for the PHP comparison, this is a very different situation. PHP resulted in changing variable values. This is only about mapping parameter values. Parameters to an action method are explicitly user input, whereas a variable you're using in your code is definitely not.
    As for the idea of using attributes to fine tune how the invoker maps requests to parameters, definetly something we're looking into already.

  8. Avatar for Francois Ward
    Francois Ward July 8th, 2008

    Sorry, I may not have been clear with my comparison with PHP. The problem I was refering to wasn't how the mapping was made, it was the conceptual model behind it.
    If you map stuff from various sources directly to variables, it is not readily obvious where the value came from. In this case, you have multiple sources, of different safety levels, mapped at the exact same place, and conflicting with each other. It is not intuitive what could overlap what and how. The request accessor is already a step above since someone has to explicitely invoke it and you'll easily be able to see it in the debugger. This is a bit too magical.
    And don't worrie, I definately understand how you must be feeling reading the comments... "Why the hell would a programmer use this feature for security sensitive code?!" is not very far from things the rest of us tells ourselves a lot "Why the -hell- would the accountant open 2 browser windows on the same page and edit the same record at the same time, and spam refresh and repost the form 16 times over!" :) Its still going to be the norm, not the exception.

  9. Avatar for Troy Goode
    Troy Goode July 8th, 2008

    I'm not interested in arguing the semantics of a hole versus a lure with you, so I'll gladly concede the point and call it a "lure."

    For example, you can also access this value like so: Request["LOGON_USER"].

    While some part of my brain was aware you could do this, I've never accessed Server Variables in this way and have never seen it suggested that you do so. Whether or not I think Request["LOGON_USER"] is a security hole is immaterial, but if I had my druthers: no, that would not work that way.
    Moving to ASP.Net MVC will change the way developers do some things they have done for a long time. How many MVC developers are going to continue accessing querystring parameters via Request.QueryString[""]? In some actions, sure, but for the most part most developers will be accessing it via the action parameters.

    We can only protect the developer from him/her self so much. At some point, the developer has to take responsibility for code that he or she writes.

    Absolutely, but surely you can see the line of thinking that might lead a developer to make a security mistake here:
    1) I used to get QueryString parameters via Request.QueryString[""]
    2) Now I usually get QueryString parameters from my action parameters.
    3) I used to get ServerVariables via Request.ServerVariables[""]
    4) Now I CAN get ServerVariables from my action parameters as well, so maybe that is how I'm SUPPOSED to.

    Again, I'm not saying we won't look into this...

    I'm glad to hear it. I'm confident you guys will come up with the appropriate solution.
    I hope you haven't viewed this an attack on the MVC framework or on your team. I truly enjoy working with ASP.Net MVC and am looking forward to seeing what you guys put out in the next release.

  10. Avatar for Scott Hanselman
    Scott Hanselman July 8th, 2008

    Foo([Server]string HTTP_WHATEVER, [Cookie]string somestuff, [Auto] more stuff)
    is a really cool idea.

  11. Avatar for HB
    HB July 8th, 2008

    Does that mean all data found in Server Variables can be modified by the user? Like for example REMOTE_HOST (or whichever one has the IP Address in it).
    If they aren't all dangerous, is there a list somewhere that has those that can be trusted?

  12. Avatar for haacked
    haacked July 9th, 2008

    @HB REMOTE_HOST can be spoofed as far as I know. I'm not sure if there's an authoritative list.
    @Scott Yeah, we've had something like that in mind for a while, but not sure if we'll do it for v1.
    @Troy - Step one is to dissuade devs from that line of thinking in the first place. ;) Step one prime (in parallel) is for us to remove the possibility of this line of thinking in this case.
    The larger point still stands, we can make this change (after discussing it, we probably will), but that doesn't mean that it's suddenly safe to trust action method parameters.

  13. Avatar for Troy Goode
    Troy Goode July 9th, 2008

    I don't disagree. =)

  14. Avatar for GuyIncognito
    GuyIncognito July 10th, 2008

    Wait a second... how exactly can REMOTE_ADDR be tampered with?
    I understand that anything that is submitted via an HTTP header during a GET/POST/PUT/DELETE/HEAD request could be set to whatever the 'bad guy' wanted (for example HTTP_USER_AGENT or HTTP_COOKIE).
    Why on earth would a web server (IIS, Apache, etc) allow the requesting client to specify it's own IP address at will?
    A malicious user could "forge" an IP packet with whatever IP address she wanted, but as far as I know the response (and any "secret" information) would go back unsolicited to that spoofed IP (if it existed). While IP spoofing is fun and profitable for DOS attacks, it's not like in the movies or those silly crime TV shows. Unless I'm wrong... then we're all doomed and putting ecommerce, patient data, and financial information on the web is incredibly stupid on all of our parts. I've been both wrong and stupid before so please correct me... :)

  15. Avatar for Michael Morton
    Michael Morton July 11th, 2008

    @GuyIncognito: You may not need the response. A simple "forged" POST to a "secured" form could be enough to create a login, delete data, and/or generally make a mess of things.

  16. Avatar for GuyIncognito
    GuyIncognito July 11th, 2008

    How many fake IP addresses would the attacker have to make up?
    2^32 (well, plus removing the ~300 million addresses for private/multi-cast) is sort of a really big number?
    Is that sort of along the lines of guessing a GUID or session ID? :)

  17. Avatar for Michael Morton
    Michael Morton July 11th, 2008

    @GuyIncognito: No, if someone coded a site with an authentication "bypass" based upon the IP, chances are (and I've seen this before) they did it so that users internal to the company can access the site without authentication. Taking into account that there is a limited number of private IP ranges (10.x, 172.16.x, 192.168.x), and making a reasonable assumption that the developer of this site coded it to allow ranges of these private IPs, you would probably only have to try a handful of IPs before finding one that would work.
    In any case, the best way to be secure is to not make any assumptions about the safety or validity of any input to your application, no matter where it comes from. Trust no one.

  18. Avatar for Dr Saxe
    Dr Saxe July 17th, 2008

    I have always been aware of many of the tactics used to gain information from you, but you have pointed out some things that I wasn't aware of and now I really will not trust user input and I thank you. Trust no one is a sad statement indeed.
    Dr Saxe