Problem/Motivation

Current implementation assumes extraction of zip archive before verification.

Extraction of an unvalidated zip-file might be a problem of itself (think zipbomb, invalid filenames).

Proposed resolution

Instead of using a single file (which has benefits from not needing to download as many files), use two. The csig file validates the zip file.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 3093782-13.patch8.77 KBheddn
#13 3093782-13.patch8.77 KBheddn
#6 3093782-6.patch10.63 KBheddn
#6 interdiff_5-6.txt1.32 KBheddn
#5 3093782.patch10.51 KBheddn
#4 drupal-8.7.6-to-8.7.9.zip933.72 KBdrumm
#3 drupal-8.7.6-to-8.7.9.zip933.51 KBdrumm

Comments

heddn created an issue. See original summary.

drumm’s picture

drumm
he/him
Location NY, US
commented

Drupal.org’s in-place-updates being served no longer have the checksumlist.csig file. The same path as the .zip files, like https://www.drupal.org/in-place-updates/drupal/drupal-8.7.4-to-8.7.5.zip, with .csig appended, now serves the signed hash of the zip file. For example, https://www.drupal.org/in-place-updates/drupal/drupal-8.7.4-to-8.7.5.zip.csig.

drumm’s picture

drumm
he/him
Location NY, US
commented
StatusFileSize
new drupal-8.7.6-to-8.7.9.zip933.51 KB

To reduce HTTP requests, maybe using the zip archive comment to store the signing is possible. Attached is an example.

$ zipnote drupal-8.7.6-to-8.7.9.zip | grep -v '^@'
untrusted comment: verify with root.pub
RWTtDh2Yu5YZWeipXt0S7xIDidhiNXahsc1pLtRzC/TOh9Zg/GpAN9ZOr8zVchh1GbJL9PYl5dqfmF+ZgSvZ3BbLdDTa/ihmdgc=
2020-01-01
untrusted comment: TESTING build keypair - not for production use public key
RWRi8iT14BMp6VPfITGCJ9bpPB4IsIJHRmCdsSpolFZTwVpljz0UUuWa
untrusted comment: verify with sites/all/modules/drupalorg/drupalorg/temporary-keys/build.pub
RWRi8iT14BMp6ZnWax9iYynhxMKqFe4/kRwl29pjZp/1EbHREhifVs/qXU+mX5gMUgmuhlr1bHnW3EC4Y7hJij2U+kzxsqv5Ww4=
SHA256 (drupal-8.7.6-to-8.7.9.zip) = eb14f40fbe40ff22291e73a6a9d8593041252d8fa9b8e9e94af691190ca5ac6d

If PHP code can safely extract that note, strip the note from the file, and get the same eb14f40fb… shasum before extraction, this will work.

drumm’s picture

drumm
he/him
Location NY, US
commented
StatusFileSize
new drupal-8.7.6-to-8.7.9.zip933.72 KB

And a sample with a base64-encoded archive comment.

$ zipnote drupal-8.7.6-to-8.7.9.zip | grep -v '^@' | base64 -d
untrusted comment: verify with root.pub
RWTtDh2Yu5YZWeipXt0S7xIDidhiNXahsc1pLtRzC/TOh9Zg/GpAN9ZOr8zVchh1GbJL9PYl5dqfmF+ZgSvZ3BbLdDTa/ihmdgc=
2020-01-01
untrusted comment: TESTING build keypair - not for production use public key
RWRi8iT14BMp6VPfITGCJ9bpPB4IsIJHRmCdsSpolFZTwVpljz0UUuWa
untrusted comment: verify with sites/all/modules/drupalorg/drupalorg/temporary-keys/build.pub
RWRi8iT14BMp6ZnWax9iYynhxMKqFe4/kRwl29pjZp/1EbHREhifVs/qXU+mX5gMUgmuhlr1bHnW3EC4Y7hJij2U+kzxsqv5Ww4=
SHA256 (drupal-8.7.6-to-8.7.9.zip) = eb14f40fbe40ff22291e73a6a9d8593041252d8fa9b8e9e94af691190ca5ac6d
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Active » Needs review
StatusFileSize
new 3093782.patch10.51 KB

We ran into issues adding the CSIG as a comment to the zip file. PHP vs posix ZIP doesn't save the file in the same format. And we have to strip the CSIG from the zip comment before hashing it. Otherwise it doesn't pass validation. But stripping it via PHP seems to rewrite the entire zip file in a slightly different format. Since the files are binary, it is hard to say exactly what is different. So we're back to downloading 2 distinct artifacts. One for the zip file and for the csig validation of the zip archive.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
StatusFileSize
new interdiff_5-6.txt1.32 KB
new 3093782-6.patch10.63 KB

heddn credited David Strauss.

heddn credited mbaynton.

heddn credited pwolanin.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Adding credit from slack discussions.

  • heddn committed d84e5fb on 8.x-1.x 
    Issue #3093782 by heddn, drumm, David Strauss, mbaynton, pwolanin: Use...
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work

Landed this on 8.x and rolled an alpha4. Next up a 7.x backport and new alpha there too.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Needs review
StatusFileSize
new 3093782-13.patch8.77 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Version: 8.x-1.x-dev » 7.x-1.x-dev
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
StatusFileSize
new 3093782-13.patch8.77 KB

  • heddn committed 7d13c12 on 7.x-1.x 
    Issue #3093782 by heddn, drumm, David Strauss, mbaynton, pwolanin: Use...
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Fixed

heddn credited Heine.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

7.x alpha3 is now also tagged.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.