nasty patches in pkgsrc regarding CVE-2010-4651: relative paths with ../

["Dr. Thomas Orgis" <thomas.orgis%uni-hamburg.de@localhost>]

Hi,

due to a little scripting accident on my side, I ended up with

TOOLS_PLATFORM.patch=/usr/bin/patch

on a Debian/Ubuntu Linux machine with GNU patch 2.7.x installed.

This breaks the build of devel/cargo-c because it contains patches like
this:

$NetBSD: patch-.._vendor_faster-hex-0.9.0_src_lib.rs,v 1.1 2024/02/10 21:46:04 pin Exp $

Unify conditions for vectorization_support() use and definition.

--- ../vendor/faster-hex-0.9.0/src/lib.rs.orig	2024-02-10 21:16:02.053719873 +0000
+++ ../vendor/faster-hex-0.9.0/src/lib.rs
@@ -43,7 +43,7 @@ pub(crate) enum Vectorization {
 
 #[inline(always)]
 pub(crate) fn vectorization_support() -> Vectorization {
-    #[cfg(all(any(target_arch = "x86", target_arch = "x86_64")))]
+    #[cfg(all(any(target_arch = "x86", target_arch = "x86_64"), target_feature = "sse"))]
     {
         use core::sync::atomic::{AtomicU8, Ordering};
         static FLAGS: AtomicU8 = AtomicU8::new(u8::MAX);


This produces really weird response from patch:

=> Applying pkgsrc patches for cargo-c-0.9.32nb1
=> Verifying […]/cargo-c/patches/patch-.._vendor_faster-hex-0.9.0_src_lib.rs
=> Applying pkgsrc patch […]/pkgsrc/devel/cargo-c/patches/patch-.._vendor_faster-hex-0.9.0_src_lib.rs
can't find file to patch at input line 7

When I tell it the same file it quoted to me:

File to patch: ../vendor/faster-hex-0.9.0/src/lib.rs
../vendor/faster-hex-0.9.0/src/lib.rs: Invalid cross-device link

Whut?! This is some way to refer to a security check about relative
paths. According to Changelog of GNU patch 2.7:

    Ignore destination file names that are absolute or that contain a
    component of "..".  This addresses CVE-2010-4651.

The CVE is about patches being able to modify any files in the
filesystem, a feature pkgsrc clearly relies on to be able to patch
../vendor out of the cargo-c WRKSRC, among others, I presume.

$ find pkgsrc/ -name 'patch-*' | xargs grep '^+++ \.\./'| wc -l
186

Well, there are some instances. I did this with pkgsrc 2025Q1, as I am
re-creating an old install, but the current state should not differ
much.

With absolute paths, GNU patch actually emits a helpful error message:

Ignoring potentially dangerous file name /dev/shm/b/hello.txt.orig
Ignoring potentially dangerous file name /dev/shm/b/hello.txt

Just with ../ it's excessively cryptic. Oh, absolute paths?

$ find pkgsrc/ -name 'patch-*' | xargs grep '^+++ /'       
pkgsrc/editors/tamago/patches/patch-aa:+++ /dev/null	1 Jan 1970 00:00:00 -0000
pkgsrc/editors/tamago/patches/patch-aa:+++ /dev/null	1 Jan 1970 00:00:00 -0000
pkgsrc/editors/tamago/patches/patch-aa:+++ /dev/null	1 Jan 1970 00:00:00 -0000
pkgsrc/editors/tamago/patches/patch-aa:+++ /dev/null	1 Jan 1970 00:00:00 -0000
pkgsrc/wip/colorize/patches/patch-aa:+++ /root/colorize/colorize.c	2011-12-02 15:29:54.000000000 +0100
pkgsrc/wip/colorize/patches/patch-ab:+++ /root/colorize/ansi_escape_sequence.c	2011-12-02 10:30:28.000000000 +0100
pkgsrc/wip/kodi/patches/patch-xbmc_cores_DllLoader_coffldr.h:+++ /usr/pkgsrc/multimedia/kodi/work/xbmc-xbmc-147cec4/xbmc/cores/DllLoader/coffldr.h
pkgsrc/devel/elib/patches/patch-dll-debug.el:+++ /tmp/dll-debug.el	2023-09-09 06:49:20.470715606 +0900

What's that about, btw.? The /dev/null is fine … but …
/root/colorize/colorize.c?! Glad that's in wip only. that leaves
pkgsrc/devel/elib/patches/patch-dll-debug.el probably not doing what
was intended.

$NetBSD: patch-dll-debug.el,v 1.1 2023/09/08 22:08:18 mef Exp $

for emacs28

--- dll-debug.el	1995-12-11 09:50:54.000000000 +0900
+++ /tmp/dll-debug.el	2023-09-09 06:49:20.470715606 +0900
@@ -74,9 +74,9 @@
[…]

Or is that intentional behaviour? It's bad in multiple ways.

To get back to the general question, though:

This fact is hidden by pkgsrc normally using BSD patch for its work.
That patch doesn't mind arbitrary paths, apparently.

What is the stance on this in the NetBSD project and pkgsrc? Is it good
to rely on behaviour that has been deemed a serious vulnerability by
$some_people? Should the patches be rather applied from the containing
directory, avoiding the ../ in them?

While I was very annoyed at first at the cryptic nature of the
failures, I realize that it feels a bit safer that patch by itself
would only modify files in the current working directory and below,
without further sandboxing. Of course we're running random build
scripts that can do anything, but still, there is a point to limiting
the blast radius of patches.

Thoughts? As it seems to me, anyone could claim that the BSD patch is
vulnerable to CVE-2010-4651, if I'm not missing something here.
Apologies in that case;-)


Alrighty then,

Thomas

-- 
Dr. Thomas Orgis
HPC @ Universität Hamburg