The tale of a forgotten bug in bspatch.

This is the tale of an under-patched bug.

While auditing source code for a customer, Luis noticed that the bspatch sources seemed to not be the latest version. Further comparison with the FreeBSD sources shows that not all patches are applied. Most notably FreeBSD-SA-16:29 is missing.

The Bug

The bug stems from bspatch not correctly handling large integer values that are read from patch files.

This is easily fixed by checking for more corner cases. The patch resolves more issues, so this is not the only part you should apply.

 		/* Sanity-check */
-		if ((ctrl[0] < 0) || (ctrl[1] < 0))
-			errx(1,"Corrupt patch\n");
+		if (ctrl[0] < 0 || ctrl[0] > INT_MAX ||
+		    ctrl[1] < 0 || ctrl[1] > INT_MAX)
+			errx(1, "Corrupt patch");

A good description and demo exploit for the issue can be found in NON-CRYPTANALYTIC ATTACKS AGAINST FREEBSD UPDATE COMPONENTS from an unknown author.

The FreeBSD Patch

When looking at the FreeBSD patch, you will quickly notice that it is not just a security patch, but that it contains formatting changes as well.

-	y=buf[7]&0x7F;
-	y=y*256;y+=buf[6];
-	y=y*256;y+=buf[5];
-	y=y*256;y+=buf[4];
-	y=y*256;y+=buf[3];
-	y=y*256;y+=buf[2];
-	y=y*256;y+=buf[1];
-	y=y*256;y+=buf[0];
+	y = buf[7] & 0x7F;
+	y = y * 256; y += buf[6];
+	y = y * 256; y += buf[5];
+	y = y * 256; y += buf[4];
+	y = y * 256; y += buf[3];
+	y = y * 256; y += buf[2];
+	y = y * 256; y += buf[1];
+	y = y * 256; y += buf[0];

Furthermore, it adds support for capsicum, which is a great way of sandboxing. Due to it being specific to FreeBSD, third parties might have simply overlooked the actual issue patched here and skipped from applying the patch in fear of breakage.

+#ifdef HAVE_CAPSICUM
+	if (cap_enter() < 0) {
+		/* Failed to sandbox, fatal if CAPABILITY_MODE enabled */
+		if (errno != ENOSYS)
+			err(1, "failed to enter security sandbox");
+	} else {
+		/* Capsicum Available */
+		cap_rights_init(&rights_ro, CAP_READ, CAP_FSTAT, CAP_SEEK);
+		cap_rights_init(&rights_wr, CAP_WRITE);
+		cap_rights_init(&rights_dir, CAP_UNLINKAT);
+
+		if (cap_rights_limit(fileno(f), &rights_ro) < 0 ||
+		    cap_rights_limit(fileno(cpf), &rights_ro) < 0 ||
+		    cap_rights_limit(fileno(dpf), &rights_ro) < 0 ||
+		    cap_rights_limit(fileno(epf), &rights_ro) < 0 ||
+		    cap_rights_limit(oldfd, &rights_ro) < 0 ||
+		    cap_rights_limit(newfd, &rights_wr) < 0 ||
+		    cap_rights_limit(dirfd, &rights_dir) < 0)
+			err(1, "cap_rights_limit() failed, could not restrict"
+			    " capabilities");
+	}
+#endif

Auditing this patch shows that breakage is unlikely, but finding the part that just patches the security issue takes a minute. We don’t want to blame the FreeBSD folks, they are doing a good job, but we feel like we should highlight this, since we see this for both, open source and proprietary projects. Limiting a bugfix to just the minimum that resolves the security issue, eases people into applying the patch. It’s usually best to split security relevant patches from feature patches, so the user can easily audit them and has a limited risk of breakage.

The CVE Issue

For some reason, this issue had no CVE assigned in the FreeBSD advisory. The security issue mentioned in FreeBSD-SA-16:25, and fixed 3 month before, was assigned a CVE ID, but this one was not. We can only speculate whether this prevented people from noticing this issue or not. But not having a CVE to properly reference certainly made it harder to talk about this issue during the disclosure process.

The Upstream Code / Forks

The patch above never made it into what some people would consider upstream, so the code of Colin was never patched.

A lot of projects use and depend on bspatch for their update process, to be able to ship incremental updates. For another reason unknown to us, most of these projects ship their own fork or copy of this code and have therefore not received any patches either. We could identify at least the following projects:

• Jitsi
• Fork of Mendsley
• OBS-Studio

A github search finds several projects using copies of bspatch.

How to identify the issue in your fork

There are various ways to identify whether you are affected by this issue. Best is if you have a proper list of code you forked in the past and can look at this. But we noticed this is not what happens in most companies. If none of the developers happens to track the security reports and knows about the use of bspatch, this will get lost in the noise. We often like to use tools like spatch from the wonderful coccinelle project to identify such issues even when code has been changed to match local coding styles and variable names have been modified.

The following are two spatch files to identify both patches. Please note that the full patch is longer, this is kept short to identify the issue easier.

@@
expression ctrl, newpos, newsize, dbz2err;
expression dpfbz2, lenread, new;
@@ 

 		/* Sanity-check */
+		if ((ctrl[0] < 0) || (ctrl[1] < 0))
+			errx(1,"Corrupt patch\n");
+
+		/* Sanity-check */
 		if(newpos+ctrl[0]>newsize)
 			errx(1,"Corrupt patch\n");

		lenread = BZ2_bzRead(&dbz2err, dpfbz2, new + newpos, ctrl[0]);

@@
expression ctrl, newpos, newsize;
expression lenread, dbz2err, dpfbz2, new;
@@


 		/* Sanity-check */
-		if ((ctrl[0] < 0) || (ctrl[1] < 0))
-			errx(1,"Corrupt patch\n");
+		if (ctrl[0] < 0 || ctrl[0] > INT_MAX ||
+		    ctrl[1] < 0 || ctrl[1] > INT_MAX)
+			errx(1, "Corrupt patch");
 
 		/* Sanity-check */
		if(newpos+ctrl[0]>newsize)
			errx(1,"Corrupt patch\n");
 
 		/* Read diff string */
 		lenread = BZ2_bzRead(&dbz2err, dpfbz2, new + newpos, ctrl[0]);

Another possible way to spot this issue is to enable certain compiler warnings for your project. The -Wconversion flag to gcc will result in the following warning:

bspatch.c:159:56: warning: conversion to ‘int’ from ‘off_t {aka long int}’ may alter its value [-Wconversion]
   lenread = BZ2_bzRead(&dbz2err, dpfbz2, new + newpos, ctrl[0]);

Conclusion

The journey with this bug shows that there are several areas the disclosure and patching process can be improved upon. Although this is an issue in open source software, we can see these patterns with proprietary code in our daily work as well. Security patches are not minimized, not properly communicated and teams keep private forks of libraries used within the company.

About X41 D-Sec GmbH

X41 D-Sec GmbH is an expert provider of application security services. With extensive experience and expertise in the information security industry and a strong core security team of world-class experts, X41 can provide premium security services. Their fields of expertise in the area of application security are security-centered code reviews, binary reverse engineering, and vulnerability discovery. Custom research and IT security consulting and support services are the core competencies of X41.