6

A Fortify security review informed us of some path manipulation vulnerabilities. Most have been obvious and easy fixes, but I don't understand how to fix the following one.

string[] wsdlFiles = System.IO.Directory.GetFiles(wsdlPath, "*.wsdl");

"wsdlPath" is input from a textbox. Is this something that just can't be fixed? I can validate the path exists, etc. but how is that helping the vulnerability?

Induster
  • 733
  • 1
  • 6
  • 15
  • 1
    How is that code run? If it is a Windows application that runs with the credentials of the user entering the `wsdlPath` I see nothing wrong. If it runs in a Windows Service or as part of a web site it is a problem. – Anders Abel Apr 10 '12 at 17:26
  • Did the Fortify review provide the injected string? – Tung Apr 10 '12 at 17:32
  • @AndersAbel - web app. an authenticated user enters the path, and if it's a valid path then it's accepted. – Induster Apr 10 '12 at 18:10
  • @Tung - they didn't provide the string, they assume any string could be potentially bad. – Induster Apr 10 '12 at 18:14

4 Answers4

8

If the data is always obtained from a text box whose contents are determined by the user, and the code runs using the permissions of that user, then the only threat is that of the user attacking themselves. That is not an interesting threat.

The vulnerability which the tool is attempting to alert you to is that if low-trust hostile code can determine the contents of that string then the hostile code can mount an attempt to discover facts about the user's machine, like "is such and such a program that I happen to know has a security vulnerability installed and unpatched?" or "is there a user named 'admin' on this machine?" and so on.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • The only user that can access this page is a system admin. There are just a few people with admin level access. I suppose this is enough "damage control" to consider this vulnerability "low". I just don't see any way to make this stronger than it is. Thanks for your input. – Induster Apr 10 '12 at 18:41
  • I marked this as the answer because the authenticated user inputting this information isn't going to obtain any useful information to them. An end-user, yes absolutely... but that isn't the case here. – Induster Apr 10 '12 at 18:44
  • @Induster - you make it stronger by ensuring the input is filtered to only allow valid path and filenames. Yes, admin access only does mitigate things, but what if an attacker gains admin access, this could allow them to gain access to the server itself, not just your app. – Erik Funkenbusch Apr 10 '12 at 18:49
  • 1
    @MystereMan: If the attacker gains admin access by some means then the attacker does not need to attack via the app. The horse is already inside the walls of Troy and the attackers are opening the gates for their army of Myrmidions to invade. Defense in depth is a good idea, but hostile admins will simply bypass your defenses; that's what being an admin *means*: being admin is precisely *possessing the ability to bypass the normal rules that keep the system secure*. – Eric Lippert Apr 10 '12 at 18:58
  • That said, there is some value in differentiating between site admins and server admins. However, it is easy to inadvertently create a security hole which turns a site admin into a de facto server admin. – Brian Apr 10 '12 at 19:12
  • @EricLippert - Not necessarily. Having admin access to the app does not give them admin access to the server. – Erik Funkenbusch Apr 10 '12 at 19:17
  • @MystereMan: Sure. Let's suppose that there is some vulnerability whereby a site admin may become a server admin. The ability of the site admin *to determine filenames on their own machine* does not help them become a server admin. But let's suppose it does, somehow. *Preventing that from happening in this program does not prevent the site admin from doing so by other means*; they are the *admin*. – Eric Lippert Apr 10 '12 at 19:43
  • @EricLippert - determining filenames could help to guess usernames, which the attacker could then try dictionary password attacks against via remote desktop or some other remote connection. Or what if there there is a vulnerability in the GetFiles API that allows arbitrary code execution? – Erik Funkenbusch Apr 10 '12 at 20:20
  • @MystereMan: Yes, absolutely. But *if the attacker is already a site admin then they do not need to use this application to enumerate the user names*. They can just enumerate the user names directly. And if the attacker is already an admin then they do not need to use an exploit to execute arbitrary code: *they are the admin; they already can execute arbitrary code directly*. – Eric Lippert Apr 10 '12 at 20:28
  • @EricLippert - i get the feeling we're talking past each other here. How can a web site admin enumerate server users directly? At most they could enumerate web site users (those stored in the database, not the web servers users) – Erik Funkenbusch Apr 10 '12 at 20:39
  • Hello Everyone(@EricLippert @MystereMan), Please help me in the below question as i'm finding difficulties to fix the coverity issue - [https://stackoverflow.com/questions/66455078/how-to-solve-the-path-manipulation-vulnerabilities-issue-coverity-scan-asp-n?noredirect=1#comment117509646_66455078] – Chandu Mar 10 '21 at 15:18
1

You should never feed anything directly into OS API's unfiltered. You should sanitize the input, make sure it doesn't contain paths (ie "../../../somefile" And make sure it truncates long names, and contains only valid filename characters (for instance, there have been various bugs relating to international characters).

Erik Funkenbusch
  • 92,674
  • 28
  • 195
  • 291
  • The input is being handled as it should. This was obviously red flagged because it originates from user input. But that needs to remain intact. To me, it looks like there is nothing we can do here short of eliminating user input, which isn't an option. Maybe somebody can see something here that I can't. – Induster Apr 10 '12 at 18:19
  • 2
    @Induster - Your response is baffling. What do you mean by "the input is being handled as it should"? Your comments indicate that it's not. You must sanitize the input to make sure you aren't feeding illegal data into an OS api, or paths to files that should not be allowed. If you think it's perfectly acceptable for a user to input, say the system password file, then hey.. forget i said anything. – Erik Funkenbusch Apr 10 '12 at 18:46
1

With that code, any user that is authenticated and authorized to use that function, is able to access the file system on the server. The access will be done using the credentials of the service account that runs the web application.

Depending on how the returned data is used, a malicious user might be able to get more information or make the server behave in a way that was not intended.

You should limit the set of allowed paths to only consist of one or a few carefully selected directories. Use the functions in the Path class to combine strings into paths - they take care of things like a user entering c:\allowedpath\..\windows\system32 for you.

Anders Abel
  • 67,989
  • 17
  • 150
  • 217
0

This kind of scenarios needs encoding and decoding to make sure that data is not manipulated anywhere. Because while decryption if data is changed you will get the wrong results.

You can create your encoding and decoding. I did it using RijndaelManaged and PasswordDeriveBytes classes provided by System.Security.Cryptography;

Ankit Garg
  • 129
  • 7