A few days ago I tweeted about this "open and read a file with popen+cat" gem I found in the firmware of one of NETGEAR's devices:

How to read a file in C according to NETGEAR pic.twitter.com/TRbxWC5vsY

— Gynvael Coldwind (@gynvael) July 1, 2021

Since there were some questions about "why is this a bad pattern?", I decided to write a short blog post explaining this.

But before we get there, please also see this short thread, or just remember to not blame an individual engineer for writing that code – rather blame the procedures NETGEAR has with regards to secure code development and quality assurance.

Context?

Let's start by adding some more context to this tweet – where is this code from, and is it the source code or something else?

This code was found in the firmware of GS110TPv3 managed switch – you might remember it from the Feral Terror vulnerability report – in one of the web UI's CGI scripts. And it's not the source code, it's the output of Ghidra's decompiler (thus if (__stream != (FILE *)0x0) part isn't what the programmer wrote, it's what the decompiler reasoned out from the disassembly). To put it another way, this code was found in what should have been production quality code shipped on devices customers buy, for example, to secure their network with VLANs and other security provided and marketed features.

To add some more technical details, this is a MIPS-based Linux platform running at 500MHz using uClibc version 0.9.31 (from 2010) and GCC 4.8.5 (from 2015) as part of Realtek MSDK.

The fopen way

So why is this pattern bad?

To explain this we need to start with the normal way to open a file for reading in C:

FILE *f = fopen("/path/and/name", "r"); if (f == NULL) { // Error handling and function exit. }

The fopen() function is implemented in uClibc (uClibc/libc/stdio/_fopen.c as _stdio_fopen), and it does three major things:

1. It allocates some memory for the FILE structure and initializes it.
2. It attempts to open the file passed as parameter using open() function.
3. It allocates some more memory for the read buffer.

The open() function is just a thin wrapper for the open system call (i.e. kernel-provided programming interface), so this whole process takes about 1-2 system calls to complete (2 in case memory allocation doesn't have enough pre-allocated free space and needs to reach out to the kernel to ask for more). Pretty neat and efficient. And yes, if we would use open() directly this could be even faster, but we lose buffering and its perks – like the ability to read line-by-line, so, meh.

Note: I'm counting system calls since context switches are pretty expensive. That said, one must remember that some system calls are pretty fast, while others might be more CPU intensive, or blocking. So this is a simplification, but it will do for this case.

The popen way

Let's compare this to popen("cat /path/and/name", "r").

What the popen() function does is basically spawning a shell (usually bash, though in this specific case that's BusyBox's Ash/Dash), replaces one of the standard I/Os with a pipe, and passes the parameter as a script to execute (yes, a script, not a single command – it can be multiline, have functions, variables, etc). To be more precise, these are the major steps taken by uClibc's popen():

1. It allocates memory to store some internal context.
2. It creates a set of pipes.
3. It calls fdopen() to create a FILE structure given a pipe descriptor (that's one memory allocation + initialization).
4. It forks the process (using vfork()).
5. In the child process:
   1. It replaces one of the standard I/Os with a pipe.
   2. It closes the file descriptor for the other end.
   3. It executes /bin/sh (BusyBox in this case) with the script to execute in one of the parameters.
6. In the parent process:
   1. It does some more internal initializations.
   2. It closes the file descriptor for one end of the pipe.
   3. And it finally returns the FILE* pointer for the pipe.

That's already a bit slower than the fopen() scenario – we have around 6 system calls and 2 memory allocations. Most importantly though, suddenly we get another process. And this is where things get relatively heavy.

So what's involved in running /bin/sh (based on system call trace using QEMU User)? Well, for start, a lot of file reads (keep in mind this is an embedded device, not a gaming PC):

1. BusyBox binary - 610KB
2. ld-uClibc.so.0 dynamic loader - 30KB
3. libubacktrace.so.0 library - 4KB
4. libdl.so.0 library - 12KB
5. libc.so.0 library (a link to libuClibc) - 486KB
6. there's also a read from /dev/urandom, but meh

And a lot of logic code in between, including (in total) 67 different system calls.

After all that we get to actually run the cat command from the script which – guess what – is also a standalone application. This means we get to read more files!

1. BusyBox binary (yes, cat is just a link to BusyBox) - 610KB
2. ld-uClibc.so.0 dynamic loader - 30KB
3. libubacktrace.so.0 library - 4KB
4. libdl.so.0 library - 12KB
5. libc.so.0 library (a link to libuClibc) - 486KB
6. there's also a read from /dev/urandom, but meh

Yeah, that's another sixty-odd system calls.

And then, after all that, cat finally uses the same open() function on the desired file – yes, the one that was used on the second step of fopen() explanation above.

I think the difference is clear, even without diving into the rabbit whole of all the system calls used in the process, or digging into what the script parser in BusyBox's shell had to do.

To summarize, instead of a nice quick file opening, we got a over 2MB of data read from multiple files, two processes spawned, multiple pages of memory allocated and deallocated, a pair of pipes created, and all that happened even before we actually got to open the desired file. All this was slow, easily avoidable, totally unnecessary, and undesirable, and is one of many similar reasons why the web UI of this device is needlessly sluggish.

Error handling

There is also one important difference between the fopen() and popen("cat ... – the latter obfuscates the error in case the file is not found. I.e. fopen() would just return NULL, but popen() will return a FILE* wrapped around a pipe regardless, and the only way for the programmer to know that the file wasn't found is to check the value of pclose() after all the data has been received from the pipe. That's pretty unintuitive when dealing with – well – a file reader, and might lead to surprising errors. E.g. would you expect to get an empty file with no warning only because you exceeded the limit of processes running in your container? Well, it would be neither the first nor the second thing I would check, that's for sure.

But wait! There's more!

If we switch the context from "a CGI script" to "a suid program" (i.e. a file with the set UID bit set and owned by a different user, which is run with privileges that user, even if invoked by a different one), we have two more problems there:

1. The cat command is specified without a path, meaning that it will be sought out in the directories listed in the standard PATH environment variable. But what if the invoking user changes PATH to a directory they control and puts an evil cat there? Well, it gets executed with the privileges of the owner of the suid program, and the attacker gets their privileges elevated / jumps to the account of the owner of the suid program (frequently that's root). There have been countless vulnerabilities of this class found over the last few decades.
2. The other problem is that popen() runs an actual shell interpreter, and shell interpreters tend to be configurable with various other environment variables as well. Historically, there were some variables which could be (ab)used to give the attacker leverage over the shell, commonly leading to some form of command execution. A most known example was the usage of IFS, but there was also e.g. shellshock and friends (and that's far from a complete list).
3. And actually (yes, I know this is the third entry in the list of two), the above is also true for the dynamic loader itself – it's yet another part of a spawned process that can be controlled (to some extent) with environment variables. See the LD_AUDIT bug as an example.

And yes, one could fairly argue, that all the known avenues with environment variables (except maybe for PATH) have been found and patched over the years. But please consider that the same argument could have been made before LD_AUDIT or shellshock were discovered – there might be more vulnerabilities where these came from. The key thing is, that there's just a simpler solution – use fopen(). And if you really need to spawn some other process, just do it directly (vfork()+execve()), without spawning a shell interpreter (and be very, very careful when you do that).

How did that code get there?

So why did someone use popen("cat ... in the first place?

Well, your guess is as good as mine.

Maybe it was some script or command executed there in a previous version, and the engineer just swapped in the cat /path/to/file there without thinking much about it.

Maybe they were trying to use fopen() but reading line-by-line together with too-small buffers was giving them unpredictable results when operating on a procfs pseudo-file (well, yeah, it's better to just read the whole pseudo-file in one go, and then parse it).

And maybe it was something else.

But all in all there is just no reason to do it. And that code should not have passed any form of review, in-depth Q&A, or a security review. And definitely should not have been found in an off-the-shelf device.