portsnap: only move expected snapshot contents from snap/ to files/

[emaste]

Summary

Previously it was possible to smuggle in addional files that would be used by later portsnap runs. Now we only move those files expected to be in the snapshot into files/ and require that there are no unexpected files.

portsnap attacks 2, 3, and 4 from "non-cryptanalytic attacks against FreeBSD update components" anonymous gist.

[emaste]

usr.sbin/portsnap/portsnap/portsnap.sh

700 ↗

(On Diff #20750)

will change sort | uniq to sort -u

[jhb]

usr.sbin/portsnap/portsnap/portsnap.sh

700 ↗

(On Diff #20750)

From the surrounding flow it seems like this should be treated as a check that fails perhaps? You could probably do something like:

cut -f 2 -d '|' tINDEX.new INDEX.new | sort -u > files1
find snap -mindepth 1 | sed -e 's,^snap/^^' | sort > files2
if diff -q files1 files2; then
     echo "unexpected files in snapshot"
     return 1
fi

And perhaps do that before the 'echo "done."' on line 694. This way the check is part of "Verifying snapshot integrity..."

It also avoids leaving bits in files/ that came from a corrupt snapshot by doing the full check up front rather than during the mv.

[emaste]

Use an approach along the lines of @jhb's suggestion

[emaste]

Also move check earlier as suggested by @jhb

[emaste]

For testing I hacked portsnap.sh to simulate a broken snapshot by uncommenting one of the two lines in the diff below:

--- a/usr.sbin/portsnap/portsnap/portsnap.sh
+++ b/usr.sbin/portsnap/portsnap/portsnap.sh
@@ -680,6 +680,8 @@ fetch_snapshot() {
        tar -xz --numeric-owner -f ${SNAPSHOTHASH}.tgz snap/ || return 1
        rm ${SNAPSHOTHASH}.tgz
        echo "done."
+       #touch snap/0000000000000000000000000000000000000000000000000000000000000000.gz
+       #rm snap/000*
 
        echo -n "Verifying snapshot integrity... "
 # Verify the metadata files

usr.sbin/portsnap/portsnap/portsnap.sh

698 ↗

(On Diff #20757)

will add . to end of message

[emaste]

usr.sbin/portsnap/portsnap/portsnap.sh

695–697 ↗

(On Diff #20757)

This is mostly @jhb's suggestion, except:

• added -type f to explicitly list files
• update sed expression for .gz
• use cmp instead of diff, as diff is one bit of remaining GPL software and cmp is sufficient for binary same/not same
• fix sense of return value

[jhb]

usr.sbin/portsnap/portsnap/portsnap.sh

695–697 ↗

(On Diff #20757)

So I had originally not included '-type f' on purpose so that we'd bail if there were extra directories, symbolic links, etc. If the stricter check (i.e. no -type f) works, then I'd be inclined to go with that.

[emaste]

remove -f from find
add trailing . to message for consistency

[allanjude]

seems straight forward enough

[delphij]

usr.sbin/portsnap/portsnap/portsnap.sh

698 ↗

(On Diff #20780)

Do we intentionally leave the two files as-is? (it's fine, just wanted to make sure if it's intended).

The patch looks otherwise fine to me.

[delphij]

Looks good to me.

[emaste]

usr.sbin/portsnap/portsnap/portsnap.sh

698 ↗

(On Diff #20780)

I did it somewhat intentionally so that if something goes wrong and the error comes out we can ask the user to send the two files to diagnose the issue.

[cperciva]

usr.sbin/portsnap/portsnap/portsnap.sh

695 ↗

(On Diff #20780)

Maybe make this

cut -f 2 -d '|' tINDEX.new INDEX.new | sort -u | lam -s 'snap/' - -s '.gz' > files.expected
find snap -mindepth 1 | sort > files.snap

?

[emaste]

usr.sbin/portsnap/portsnap/portsnap.sh

695 ↗

(On Diff #20780)

I've tested the current version so plan to use it initially, but will switch to your approach in a subsequent patch.