What things should I check for when giving a client control of the filename parameter to C's open() function?

Question

I'm doing a school project where we are building a simple web server in C. To implement this I read the first line of the request (all I need for my purposes) and parse the middle string as the file name using strtok. So a typical request would be something like:

GET / HTTP/1.0

and my response code would be something like this:

  // first token is request type
  token = strtok(buffer, " \n\r");
  if(strcmp(token, "GET") != 0) {
    // sendResponse just sends a response to the client
    // with the appropriate headers
    sendResponse(connection, -1, HTTP_11_400, strlen(HTTP_11_400), NULL);
    break;
  }
  char *request_type = token;

  // second token is url
  token = strtok(NULL, " \n\r");
  if(strstr(token, "../") != NULL) {
    sendResponse(connection, -1, HTTP_11_403, strlen(HTTP_11_403), NULL);
    break;
  }
  char *request_url = token;

  // third token is HTTP protocol
  token = strtok(NULL, " \n\r");
  if(strcmp(token, "HTTP/1.0") != 0 && strcmp(token, "HTTP/1.1") != 0) {
    sendResponse(connection, -1, HTTP_11_400, strlen(HTTP_11_400), NULL);
    break;
  }
  char *protocol = token;

  printf("filename: %s\n", request_url);
  if(strcmp(request_url, "/") == 0) {
    request_url = "index.html";
  }
  if(*request_url == '/') {
    request_url = request_url++;
  }
  char* fileType = strrchr(request_url, '.') + sizeof(char);
  fd = open(request_url, O_RDONLY);

As you can see I already check to make sure the client is not able to go up a directory. Looking at my code, I'm basically putting a user-inputted string and trusting the user with it, are there other security features that I should take into account? (I also have to write a paper on this implementation and I think it would be nice to write about security).

Answer 1

This is a deceptively simple question as it boils down to how can paths be be abused. I for one don't claim to know how many ways they can be abused because there are many. Some of them are platform specific and there was no platform mentioned. You aren't doing any matching of paths for access control, passing off control to different handlers based on the path contents or decoding percent encoding so it's a bit simpler.

The main issue I see with your code is the incomplete check for a absolute path. The desire appears to be that only files in the current directory will be returned and hence the stripping off of a leading slash. This is incomplete though as starting the path with more then one slash will allow an attacker to request files outside of the current directory (eg: //etc/passwd). You can fix this by either stripping any number of leading slashes or by preppending the server root path to the request path. Either would do. Both might be wise.

After the open and error checks you should us fstat to do some sanity checking that what you just opened is just a plain file and bail otherwise. Although not likely to be in the server root, you likely don't want to be reading from a device file. Also some platforms (again, not specified in the question) will happily return a stream of dirent structures with simple open of a directory giving the attacker directory listings.

You need to know the potential oddities of the platform you're on as well (your code appears to be posix but it could be a library layer like cygwin). If you're unlucky enough to be running such on windows you need to consider issues such as whether you want to allow access to alternate file streams (:stream suffix). In particular you would need to make sure you're not allowing the attacker to access dos devices names (https://en.wikipedia.org/wiki/Device_file#DOS).

One last pedantic tidbit. strtok is not thread safe. You don't specify in the question if this was a multi-threaded server or not. If so you have a race condition. Even if it's not multi-threaded I'd change your code to use strtok_r. I once saw a apache mod written for 1.3 which did preforked workers ported to 2.x which usually does threads that used strtok. It was perfectly fine solution for 1.3 but the porters didn't notice the issue when they ported it.

Answer 2

The minimum you need to do is to ensure that .. is never used as a path component. It is enough to check that:

• The requested path begins with /. (If it doesn't, you may want to either reject it or forcibly prepend / before doing the next check.)
• The requested path does not contain the substring /../.
• The requested path does not end with the suffix /...

Your check is not completely correct: it lets /.. through.

Prepend the web root to the user-supplied path (recall that it must begin with /). The web root should be an absolute path (in a multithreaded program, you often can't count on the current directory).

This is enough to confine clients to the web root. Note that if there are any symbolic links in the web root, they will be followed. Make sure not to have special files (named pipes, device nodes, etc.) under the web root.

If you do access control, note that there may be multiple paths refering to the same file, for many reasons, including:

• // is equivalent to /
• /./ is equivalent to /
• symbolic links
• some filesystems equate characters, e.g. A = a on case-insensitive filesystems
• the meaning of non-ASCII characters may be different in the request and on the filesystem as it depends on the character encoding.

If you convert the request from URL encoding (with %-hexadecimal escapes) to a raw string, note that / and . characters may pop up at that stage. The path checks need to be performed on the final string that you'll use as a path fragment.

My answer assumes Linux. Windows is more dangerous due to its special file names (see Graham's answer).

Answer 3

Approach it as a mapping from "what the user should be allowed to do" to "files on the server."

Checking that they cannot go "up" a directory is one example of a limitation on the mapping: it assumes that the server configuration has designated a "safe" folder, such that all files in that folder, recursively, are safe.

Everything else is very dependent on your situation. Apache, for instance, uses a file called .htaccess to control permissions. Accordingly, Apache has to ensure the end-user does not ask for a copy of .htaccess directly (which would tell them the permissions on all other folders). As another example on unix based Apache releases, apache also double checks that the file requested is not a filesystem soft-link to a file outside of the "safe" folder.

Answer 4

C is a fraught language for doing security-related string manipulation. But presumably you have no choice.

If you need to support unicode, then I'd recommend using an appropriate library for it. Otherwise (as this is a school project) I would suggest simply rejecting any requests that aren't limited to 7-bit ascii. So that's (uchar) c > 127 for any character in the request.

You may also want to filter out any characters that could represent any silly business. If you know all your files consist of A-Za-z0-9._-, then reject any requests that don't follow this pattern. Best to white-list rather than black-list. This step could take the place of the one above. Make yourself a list of acceptable characters, and for each character in the request, verify that it exists in your whitelist.

Now that you've got a clean character set, check for directory traversal. Easiest to just look for .. anywhere in the request (the dangerous cases are /../, ../ at the start, and potentially /.. at the end and .. alone). You may also want to look for //, which strictly speaking isn't illegal, but could be trouble if interpreted wrong (it's especially risky at the start of the local path). Verify that the request starts with / (not optional in HTTP per RFC),

And then from there you should be safe appending it to your document root and checking access(path,R_OK) to verify it exists and is readable. stat() it to verify that it's a file (not a dir or symlink... unless those are allowed; but certainly not a socket or something), and THEN you can open it for reading.