Finding and Fixing Common Vulnerability Exposure (CVE) 2020-25212

While ensuring Titanium 8.1.0 could officially support NFS backends for its protected filesystems, I found and fixed a Linux kernel bug in NFS’s security label code, later published as CVE-2020-25212. The bug was caused by a misplaced buffer size check and a lack of a check for there being a buffer at all, causing any set of arguments to __vfs_getxattr(), except for the most mundane, to either overflow the buffer or kernel panic. Had NFS 4.2’s extended attribute support gotten a little more testing, perhaps by adding an SELinux policy to take advantage of security labels on NFS, this bug might have been caught and fixed sooner.

If You Don’t Test it, it’s Probably Broken

It's almost always a bad idea to write more than a few lines of code, assume they work as intended, and move on to a separate task. Even after the change compiles successfully, and even after code reviews, there's always the chance that some underlying assumption turns out to be wrong, yielding unexpected results in edge cases. Once such a bug is introduced, it might linger for long periods of time if that section of code isn't in frequent use.

Initial Investigation

This CVE first manifested itself as a null pointer kernel panic when mounting an Authfs or Fortifs (filesystems that Star Lab ships as part of Titanium) vault with an NFS backend while security label support is enabled. A coworker, to facilitate official NFS backend support in Titanium 8.1.0, modified a large portion of the Titanium test pipeline to run a second time with an NFS mount in place of the usual backend directory only to discover that, with SELinux enforcing, the tests would hit the aforementioned kernel panic early in each pipeline. Part of the output, reproduced with Ecryptfs on an unmodified kernel, is available at lkml.org/lkml/2020/8/5/648.

A few months later, I took on the task of investigating it. I found that, in addition to enforcing mode, I could reproduce the panic with SELinux in permissive mode too. I could also cause it with Ecryptfs after adding an SELinux CIL module containing (fsuse xattr "ecryptfs" (system_u object_r fs_t ((s0)(s0)))). From there, I connected GDB to my VM, set a breakpoint at the lowest-level NFS function in the panic trace (decode_getfattr_attrs()), and stepped through line by line until triggering the panic. Because the call traces from kernel errors aren’t 100% accurate, the actual panic line (memcpy(label->label, p, len);) was in the lower-level function decode_attr_security_label().

Further Debugging

Knowing the precise location of the panic, of course, wasn’t enough to understand or fix it, especially since this was my first foray into the NFS source code. I printed out label->label with GDB’s p command, which memcpy() would try to copy data into, and found that it was a null pointer, precisely what I was looking for to pinpoint the variable that caused the panic. The next logical step was to examine each stack frame to figure out where the null pointer came from. I’ve pasted the simplified Ecryptfs-based backtrace (plus inlined functions) here:

`
 0: decode_attr_security_label
 1: decode_getfattr_attrs
 2: decode_getfattr_generic
 3: decode_getfattr_label
 4: nfs4_xdr_dec_getattr
 5-8: RPC Functions
 9: nfs4_call_sync_sequence
10: nfs4_call_sync
11: _nfs4_get_security_label
12: nfs4_get_security_label
13: nfs4_xattr_get_nfs4_label
14: __vfs_getxattr
15: ecryptfs_getxattr_lower
16: ecryptfs_getxattr
17: ecryptfs_xattr_getget
18: __vfs_getxattr
19: sb_finish_set_opts

(Since my debugging, another patch created sb_check_xattr_support() between frames 18 and 19)

Frame 0 was attempting to populate the struct nfs4label that label pointed to and frames 1-4 merely passed label around. Frames 5-8 were Remote Procedure Call (RPC) functions, so I largely ignored them. In frame 11, _nfs4_get_security_label(), there was some interesting activity. It defined struct nfs4label label, populated label.label and label.len with its arguments void *buf and size_t buflen, and specified nfs4_xdr_dec_getattr() as the callback function for RPC. It also, rather incriminatingly, possessed what looked like a buffer size check… after the function call that panicked:

ret = nfs4_call_sync(server->client, server, &msg, &arg.seq_args, &res.seq_res, 0);

if (ret)
   return ret;

[...]

if (buflen < label.len)
    return -ERANGE;

That wasn’t necessarily the entire source of the bug, since I wasn’t sure if null pointers passed through frames 12-14, including __vfs_getxattr(), were expected. This made me somewhat worried because the remaining frames were not within NFS, leaving an increased chance that Ecryptfs, Authfs, and Fortifs might somehow be creating the null pointer.

Luckily, frames 15-17 merely pass the __vfs_getxattr() call through to the backend filesystem without changes. Reading through frame 19, sb_finish_set_opts(), gave me solid confirmation that a null pointer argument for __vfs_getxattr() is a supported use case because SELinux explicitly passes a null pointer and buffer size. That meant that I didn’t have to worry about the functions beyond the __vfs_getxattr() call at frame 14.

One thing to note while debugging the Linux kernel is that compilation will fail if you fully disable compiler optimizations. Variables will be optimized out, functions will be inlined, and to top it off, single stepping through a function will sometimes repeatedly jump forwards and backwards from one line to another. These issues can frequently, but not always, be mitigated via particular debugging techniques. For instance, most of the time, if p variable_name prints "<value optimized out>," that means variable_name held very predictable data, which can be inferred via the expression used to define it. If variable_name is an argument, that expression may be on a different stack frame. Function inlining might cause apparent inconsistencies between stack frames, where frame X doesn’t seem to call frame X-1. This makes it useful to have a terminal and text editor open to grep for definitions or references to those functions, thereby connecting the dots. Finally, single-stepping jumps just require patience to overcome. If the step GDB command doesn’t step into the next function, chances are it’ll jump back later. GDB’s TUI mode (tui enable) is also handy to not lose track of which line you’re on.

Comparing Behavior to Documentation

To figure out what __vfs_getxattr() and filesystem-specific getxattr functions are intended to do, particularly when the buffer size is zero, I looked up the getxattr man page. The man page specifies that, when the buffer size is zero, they should return the length of the specified extended attribute, as they would normally, without copying any data. I also noted that they should return -ERANGE if the buffer size is too small, which perfectly matched the misplaced buffer check in _nfs4_get_security_label().

To summarize, the normal behavior of getxattr() and __vfs_getxattr() is to write the value of an extended attribute to the specified buffer and return the length of the attribute. When the buffer is too small to contain the attribute, they should return -ERANGE except for the special scenario where the buffer size is zero, in which the buffer pointer is ignored, and the function returns the length of the extended attribute. In the first scenario, with a large enough buffer, NFS did its job. However, in either of the two less likely scenarios, due to the misplaced buffer size check, NFS would blindly write the entire attribute to the buffer, overflowing it or, in the case of SELinux’s check with a null pointer, kernel panic. The solution, therefore, was to move the length check backwards into decode_attr_security_label() and add a check so that, if the buffer has a size of zero, the function continues as normal without calling memcpy().

Applying, Testing, and Upstreaming the Patch

I compiled this change, manually verified in the Titanium kernel that I could mount Authfs and Fortifs vaults on an NFS backend while SELinux was enforcing, and reran the pipeline. The pipeline still failed but got much further than before and had a different, unrelated failure that was much less severe than a kernel panic. I put together a patch (commit ID b4487b93), tested the latest staging kernel with Ecryptfs, both with and without the patch, and sent it to the upstream maintainers. It and a follow-up patch I made (d33030e2, which fixed a separate function not resetting a reused label->len value) are both now in kernel versions 5.8.14+ and all kernels distributed with Titanium 8.1.0+.

Conclusion

This bug most likely went unnoticed because the relevant code hardly ever ran. SELinux doesn’t take advantage of NFS’s new security label support by default, so the only probable scenario that this bug would be noticed by accident is if someone either specifically created an SELinux policy to use security labels with NFS or mounted a layered filesystem with such a policy. Thanks to Titanium’s CI tests, I could fix this error before it sneaked into a release that officially supported the failing use case.

We can all agree data access has become ingrained in daily life. However, we don’t give much thought to where that data lives or the process that occurs before we see it. This is an intentional abstraction meant to limit the basic knowledge necessary to perform most data-related tasks. The Linux variant of this is called the Virtual Filesystem, or VFS for short. The VFS acts as the interface between the user and the file’s backing filesystem, masking any implementation details behind generic calls such as `open()`, `read()`, `write()`, etc.

How Overlooked Assumptions Result in Emerging Cybersecurity Threats