Bug#864082: Improved patch
Sorry about the noise, The long mail was sent only to control@... and therefore invisible. With kind regards, Roland Clobus Forwarded Message Subject: Improved patch Date: Fri, 28 Jan 2022 18:08:28 +0100 From: Roland Clobus To: cont...@bugs.debian.org reopen 864082 = thanks Hello maintainers, Thanks for releasing a new version of fontconfig with the patch for the reproducible cache files (0001-Ensure-cache-checksums-are-determinstic.patch). Unfortunately, I see side-effects of the applied patch (as shown in the live-build images [1]) * Potential out-ouf-bounds-read issue: The function uuid_generate_sha1 is called with an incorrect second argument. The second argument must be guaranteed to be of length 16 (or longer), which is the size of a uuid. E.g. /root/.fonts is only 12 bytes, which means that some random bytes at the end of the string will be used for the sha1 sum. The updated patch uses the null namespace as the basis for the sha1 sum. ... or should I have use one of the predefine namespaces instead? * The patch adds new compiler warnings. I've added some casts to remove compiler warnings * There is a second scenario: initramfs with fonts: plymouth-set-default-theme tribar --rebuild-initrd or update-initramfs -k all -u The value for 'target' is contains a random part: /var/tmp/mkinitramfs_ijJP8d//usr/share/fonts This path is created by the plymouth hook in initramfs which uses 'fc-cache -s -y TEMPDIR' The fonts in the ramdisk can be listed with: zless /initrd.img | cpio --list --quiet | grep fontconfig | grep cache-7 For regular invocations of fc-cache, the '-y' argument is not used and then 'target' and 'dir' are identical. The attached patch uses 'dir' instead of 'target' and then the cache of the embedded fonts in the ramdisk is reproducible as well. Attached you'll find the patch that fixes all three issues mentioned above. With kind regards, Roland Clobus [1] https://jenkins.debian.net/view/live/ From 70565e0f73d116a2a9523146228efcca1e76b016 Mon Sep 17 00:00:00 2001 From: Chris Lamb Date: Mon, 29 Oct 2018 15:48:51 -0400 Subject: [PATCH] Make the cache filenames determinstic Whilst working on the Reproducible Builds[0] effort, we noticed that fontconfig generates cache files with unreproducible/non-deterministic filenames. This is a supplement to the changes added in f098adac54ab where we ensured that the checksums themselves were determistic but the files that were stored in the cache directory are currently being given "random" names via uuid(3)'s uuid_generate_random function, thus any images that generate such files have different contents on every build. This patch changes the behaviour of the cache directory filename calculation to be based on the "source" directory name, rather than being entirely random. An alternative solution could be to continue to use the previous uuid_generate_random function but use this alternative codepath if the SOURCE_DATE_EPOCH[1] environment variable was determined to be present via getenv(3). This work was sponsored by Tails[2] and tracked in Debian in #864082[3]. [0] https://reproducible-builds.org/ [1] https://reproducible-builds.org/specs/source-date-epoch/ [2] https://tails.boum.org/ [3] https://bugs.debian.org/864082 --- src/fccache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: fontconfig-2.13.1/src/fccache.c === --- fontconfig-2.13.1.orig/src/fccache.c +++ fontconfig-2.13.1/src/fccache.c @@ -101,7 +101,13 @@ FcDirCacheCreateUUID (FcChar8 *dir, ret = FcFalse; goto bail3; } - uuid_generate_random (uuid); + if (getenv("SOURCE_DATE_EPOCH")) + { + const uuid_t nil = { 0 }; + uuid_generate_sha1 (uuid, nil, (const char *)dir, strlen((const char *)dir)); + } + else + uuid_generate_random (uuid); if (force) hash_add = FcHashTableReplace; else OpenPGP_signature Description: OpenPGP digital signature
Bug#864082: Improved patch
Hi Roland, Quoting Roland Clobus (2022-01-28 18:22:17) > Sorry about the noise, as far as I'm concerned, you made no noise but you made a very valuable contribution instead. Thank you! :) > Thanks for releasing a new version of fontconfig with the patch for the > reproducible cache files > (0001-Ensure-cache-checksums-are-determinstic.patch). > > Unfortunately, I see side-effects of the applied patch (as shown in the > live-build images [1]) > > * Potential out-ouf-bounds-read issue: The function uuid_generate_sha1 > is called with an incorrect second argument. > > The second argument must be guaranteed to be of length 16 (or > longer), which is the size of a uuid. > E.g. /root/.fonts is only 12 bytes, which means that some random > bytes at the end of the string will be used for the sha1 sum. > > The updated patch uses the null namespace as the basis for the sha1 sum. > >... or should I have use one of the predefine namespaces instead? > > * The patch adds new compiler warnings. I've added some casts to remove > compiler warnings > > * There is a second scenario: initramfs with fonts: > plymouth-set-default-theme tribar --rebuild-initrd > or > update-initramfs -k all -u > > The value for 'target' is contains a random part: > /var/tmp/mkinitramfs_ijJP8d//usr/share/fonts > > This path is created by the plymouth hook in initramfs which uses > 'fc-cache -s -y TEMPDIR' > > The fonts in the ramdisk can be listed with: > zless /initrd.img | cpio --list --quiet | grep fontconfig | grep cache-7 > > For regular invocations of fc-cache, the '-y' argument is not used > and then 'target' and 'dir' are identical. The attached patch uses 'dir' > instead of 'target' and then the cache of the embedded fonts in the > ramdisk is reproducible as well. > > Attached you'll find the patch that fixes all three issues mentioned above. You completely rewrote the patch. I think it makes sense to replace Chris' authorship with yours. Chris, do you agree? Would you be willing to submit an updated patch containing the name and email of your choice and a commit message that explains your change? What you wrote above is a good explanation I think. Thanks! cheers, josch signature.asc Description: signature
Bug#864082: Improved patch
Hello Johannes, On 28/01/2022 20:22, Johannes Schauer Marin Rodrigues wrote: Would you be willing to submit an updated patch containing the name and email of your choice and a commit message that explains your change? What you wrote above is a good explanation I think. Attached is the updated patch. A commit message would be: This patch changes the behaviour of the cache directory filename calculation to be based on the "source" directory name, rather than being entirely random if the SOURCE_DATE_EPOCH[1] environment variable was determined to be present via getenv(3). The two main scenarios are covered by this patch: 1) Invocation of 'fc-cache' as a postinst step 2) Invocation of 'update-initramfs' as a postinst step where the initial ramdisk contains a font (e.g. when the plymouth hook calls 'fc-cache -s -y TEMPDIR') or much shorter: Generate deterministic cache directory filenames if SOURCE_DATE_EPOCH is set. With kind regards, Roland Clobus From: Roland Clobus Date: Sat 29 Jan 07:58:22 UTC 2022 Subject: [PATCH] Make the cache filenames determinstic Whilst working on the Reproducible Builds[0] effort, we noticed that fontconfig generates cache files with unreproducible/non-deterministic filenames. This is a supplement to the changes added in f098adac54ab where we ensured that the checksums themselves were determistic but the files that were stored in the cache directory are currently being given "random" names via uuid(3)'s uuid_generate_random function, thus any images that generate such files have different contents on every build. This patch changes the behaviour of the cache directory filename calculation to be based on the "source" directory name, rather than being entirely random if the SOURCE_DATE_EPOCH[1] environment variable was determined to be present via getenv(3). The two main scenarios are covered by this patch: 1) Invocation of 'fc-cache' as a postinst step 2) Invocation of 'update-initramfs' as a postinst step where the initial ramdisk contains a font (e.g. when the plymouth hook calls 'fc-cache -s -y TEMPDIR') This work is based on the patch written by Chris Lamb who is sponsored by Tails[3]. [0] https://reproducible-builds.org/ [1] https://reproducible-builds.org/specs/source-date-epoch/ [2] https://bugs.debian.org/864082 [3] https://tails.boum.org/ Bug-Debian: http://bugs.debian.org/864082 --- src/fccache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: fontconfig-2.13.1/src/fccache.c === --- fontconfig-2.13.1.orig/src/fccache.c +++ fontconfig-2.13.1/src/fccache.c @@ -101,7 +101,13 @@ FcDirCacheCreateUUID (FcChar8 *dir, ret = FcFalse; goto bail3; } - uuid_generate_random (uuid); + if (getenv("SOURCE_DATE_EPOCH")) + { + const uuid_t nil = { 0 }; + uuid_generate_sha1 (uuid, nil, (const char *)dir, strlen((const char *)dir)); + } + else + uuid_generate_random (uuid); if (force) hash_add = FcHashTableReplace; else OpenPGP_signature Description: OpenPGP digital signature