Lab-3-Rca

UWE-P564-01: Host header buffer overflow

The Basics

Authors: Samuel Tay, Chloe Claridad
Disclosure Date: 2022-11-22
Product: tinyserv
Reporter(s): CSE P564 staff
Exploit sample: see sploit1.sh

The Exploit

Exploit strategy: The header struct in tinyserv has two consecutive fields char host[32] and int is_admin. When assigning the incoming packet's Host header value, there is no limit on the length of the data copied into host. The exploit simply sends a request with a Host header value that exceeds 32 chars, thus overwriting the is_admin flag, allowing the user to access admin-restricted content.

Exploit primitives: This exploit has no limit on the buffer overflow. Thus while the sploit1.sh example simply overwrites the very next is_admin field of the header struct, the vulnerability is actually much greater. In particular, the hdr value is sitting on the serve function's stack frame, after two other stack variables int id and int timeout. Thus by using a host value that exceeds the size of the header struct and two ints, the attacker could overwrite the return address of the serve function and execute arbitrary code in a ret-lib-c style attack.

The Vulnerability

Vulnerability details: The vulnerability is a buffer overflow. Specifically the header struct has a buffer field char host[32] defined on line main.c:85. Then on lines main.c:483-488 this field is written to without regard to its 32-len size limit.

Bug class: CWE-787

Severity/priority score: The severity is high; arbitrary code execution is possible with this vulnerability.

Thoughts on how this vulnerability might have been found: Fuzzing probably could have found this, however for the size of this codebase an audit would be a good solution without huge overhead. In particular, for any of the buffers defined at the top of the file, simply search for writes to those variables and verify that length checks are in place.

The Patch

Proposed patch plan: The plan is to define a const HOST_MAX_LEN 32, use this constant in the host field definition char host[HOST_MAX_LEN], and then fix the get_host function such that it returns the lesser of the found length and HOST_MAX_LEN.

Killing the bug class: Oh jeez. I don't think there is a general accepted solution for ridding ourselves of buffer overflows in C. If it were up to me, with such a small codebase, I'd just port it to a different language like Rust and avoid unsafe. Outside of a rewrite, two things come to mind:

1. Lesser effort: create consts for each of the defined buffer lengths and put explicit bounds checks on all of the existing buffers. Having this pattern in the codebase goes a long way to others following this pattern in the future.
2. More effort: integrate some automated fuzz testing.

UWE-P564-02: Naive equality check for admin filepath

The Basics

Authors: Samuel Tay, Chloe Claridad
Disclosure Date: 2022-11-22
Product: tinyserv
Reporter(s): CSE P564 staff
Exploit sample: see sploit2.sh

The Exploit

Exploit strategy:: When checking to see if a request is for the restricted admin.txt file, a simple string equality check is performed; that is, the server checks if the requested file path is "admin.txt". This is exploited by using a file path that is equivalent to the restricted file, but a different string. For example when the server is started with a root directory ./files the user requests file "../admin.txt". This passes the check which determines the admin request, and the server will dutifully send back the restricted file ./files/../admin.txt.

Exploit primitives: The vulnerability allows the attacker to read arbitrary files on the server; to be precise, if the server is started by a user $USER, then the attacker can access any file for which $USER has read access.

The Vulnerability

Vulnerability details: There are actually two vulnerabilities here.

1. The user gains access to the specific restricted admin.txt file. This occurs in the function int packet_is_admin_page(char *packet), specifically on lines main.c:946-949, which simply performs a string equality check against "admin.txt".
2. The user can access content outside of the directory specified by the CLI argument. This is a check that should occur in the get_header function, after checking packet_is_admin_page. The proposed patch will insert an extra else if block between lines main.c:508 and main.c:510.

Arguably, (2) is the more dangerous exploit here. (1) does leak potentially sensitive data for the website, but (2) allows users to access local files on the server, which could potentially be used to gain root access and wreak much more havoc (e.g. imagine someone is running tinyserver as root, attacker request /etc/passwd or /etc/shadow, breaks hashes to get root password, etc.).

Also note that these are overlapping in the normal case, but need not be; in the case when the user makes the poor choice of using the tinyserv directory or one of its parent directories as the files argument, the user is not exploiting vulnerability (2) but still exploiting vulnerability (1).

Bug class (CWE):

1. The first vulnerability is class CWE-41.
2. The second vulnerability is class CWE-23.

Severity/priority score: Vulnerability (1) is medium severity; attackers get to see some data on the server usage. It's not good but it's not the end of the world. However vulnerability (2) is high; as explained above, this could in certain cases lead to someone having root access on the server.

Thoughts on how this vulnerability might have been found: The only thing I can think of is a code audit.

The Patch

Proposed patch plan: For (1) the plan is to pass both the requested path and admin path through realpath first, and then compare equality. Similarly, for (2) we will pass the requested path and the dir argument through realpath and check that realpath(dir) is a prefix of realpath(requested_file).

Killing the bug class: I'm not very familiar with C, so this might not be possible, but my thought is to solve this with types. For example in Haskell or Rust I would use a FilePath type that has lawfully correct Eq behavior.