ostreedev / ostree

Operating system and container binary deployment and upgrades
https://ostreedev.github.io/ostree/
Other
1.31k stars 300 forks source link

corrupt gpg signatures are written when repo is on cifs (samba) #3288

Closed foresto closed 2 months ago

foresto commented 3 months ago

When the repo is on a cifs filesystem, ostree writes gpg signatures full of null bytes, rather than writing the correct signature data. This causes signature validation to fail, completely breaking flatpak repository updates.

Reproducer:

#!/bin/sh

set -e

if [ "$#" -lt 2 ] || [ "$1" = "-h" ] ; then
    echo "usage: $(basename "$0") <repo-dir> <gpg-key-id>"
    exit 2
fi

repo=$1
keyid=$2
src="./foo"

echo "creating ostree repo at $repo"
ostree init --repo="$repo"

echo "creating test tree at $src"
mkdir -p "$src"
echo hi > "$src"/hello

ostree commit --repo="$repo" --branch=foo --gpg-sign="$keyid" "$src"

if ostree show --repo="$repo" foo; then
    echo ---
    echo success!
else
    echo ---
    ostree show --repo="$repo" --print-detached-metadata-key=ostree.gpgsigs foo
    echo failure!
    echo look for null bytes in the above commit signature
fi

I discovered this while exporting and updating a flatpak repo: flatpak/flatpak#5911

Reproduced on Debian Stable with a current kernel and ostree 2022.7-2, and on Debian Testing with ostree 2024.7-1.

$ uname -a Linux ink 6.10.6-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.10.6-1 (2024-08-19) x86_64 GNU/Linux

foresto commented 3 months ago

My flatpak bug report includes a sequence of events discovered with strace, revealing different behavior on cifs vs. etx4 with respect to temp files and memory mapping. I'm starting to think that behavior comes from libostree. Maintainers here might want to read the report.

foresto commented 2 months ago

I ended up tracing the corruption to a set of cifs regressions that made it into all the stable and mainline kernel releases.

https://linux-regtracking.leemhuis.info/regzbot/regression/lore/pv2lcjhveti4sfua95o0u6r4i73r39srra@sonic.net/

Regardless of whether the memory mapping done here is a good idea, the corruption seems to be gone, so I'm closing this.

cgwalters commented 2 months ago

Glad you were able to track that. I agree what we're doing here is really silly, there are some abstractions that interact in unnecessary ways. But indeed it looks like other things broke too.