Page MenuHomeFreeBSD

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

Authored by emaste on Sep 28 2016, 2:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 26 2024, 7:17 AM
Unknown Object (File)
Dec 22 2023, 9:46 PM
Unknown Object (File)
Nov 24 2023, 10:51 AM
Unknown Object (File)
Nov 19 2023, 9:11 PM
Unknown Object (File)
Nov 9 2023, 6:01 AM
Unknown Object (File)
Nov 9 2023, 2:54 AM
Unknown Object (File)
Nov 7 2023, 2:53 PM
Unknown Object (File)
Nov 6 2023, 5:25 AM
Subscribers
None

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste retitled this revision from to portsnap: only move expected snapshot contents from snap/ to files/.
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added reviewers: cperciva, glebius, jhb, delphij, allanjude.
usr.sbin/portsnap/portsnap/portsnap.sh
700 ↗(On Diff #20750)

will change sort | uniq to sort -u

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.

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

Also move check earlier as suggested by @jhb

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

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
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.

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

allanjude edited edge metadata.

seems straight forward enough

This revision is now accepted and ready to land.Sep 28 2016, 8:37 PM
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 edited edge metadata.

Looks good to me.

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.

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

?

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.

This revision was automatically updated to reflect the committed changes.