PR#3690: only pad header lengths for signature headers - koji

koji

#3690 only pad header lengths for signature headers

Merged a year ago by tkopecek. Opened a year ago by mikem.

mikem/koji non-padded-headers into master

unit test: more closely mimic rpm2cpio behavior when reading past header

Mike McLean • a year ago

41a89dd

unit test: close fd

Mike McLean • a year ago

fec447a

unit test

Mike McLean • a year ago

f9c1f0c

only pad header lengths for signature headers

Mike McLean • a year ago

a04e00c

koji/__init__.py

file modified

+6 -4

		`@@ -618,11 +618,12 @@`

		`# The lead is a fixed sized section (96 bytes) that is mostly obsolete`
		`sig_start = 96`
		`- sigsize = rpm_hdr_size(path, sig_start)`
		`+ sigsize = rpm_hdr_size(path, sig_start, pad=True)`
		`+ # "As of RPM 2.1 ... the Signature section is padded to a multiple of 8 bytes"`
		`return (sig_start, sigsize)`


		`- def rpm_hdr_size(f, ofs=None):`
		`+ def rpm_hdr_size(f, ofs=None, pad=False):`
		`"""Returns the length (in bytes) of the rpm header`

		`f = filename or file object`
		`@@ -652,8 +653,9 @@`
		`# this is what the section data says the size should be`
		`hdrsize = 8 + 16 * il + dl`

		`- # hdrsize rounded up to nearest 8 bytes`
		`- hdrsize = hdrsize + (8 - (hdrsize % 8)) % 8`
		`+ if pad:`
		`+ # signature headers are padded to a multiple of 8 bytes`
		`+ hdrsize = hdrsize + (8 - (hdrsize % 8)) % 8`

		`# add eight bytes for section header`
		`hdrsize = hdrsize + 8`

tests/test_lib/test_header_sizes.py

file added

+71

		`@@ -0,0 +1,71 @@`
		`+ # coding=utf-8`
		`+ from __future__ import absolute_import`
		`+ import os.path`
		`+ import unittest`
		`+`
		`+ import koji`
		`+ import rpm`
		`+`
		`+`
		`+ SIGTAG_SIZE = 1000`
		`+ try:`
		`+ SIGTAG_LONGSIZE = rpm.RPMTAG_LONGSIGSIZE`
		`+ except NameError:`
		`+ SIGTAG_LONGSIZE = None`
		`+`
		`+`
		`+ class TestHeaderSizes(unittest.TestCase):`
		`+`
		`+ RPMFILES = [`
		`+ "test-deps-1-1.fc24.x86_64.rpm",`
		`+ "test-files-1-1.fc27.noarch.rpm",`
		`+ "test-nosrc-1-1.fc24.nosrc.rpm",`
		`+ "test-deps-1-1.fc24.x86_64.rpm.signed",`
		`+ "test-nopatch-1-1.fc24.nosrc.rpm",`
		`+ "test-src-1-1.fc24.src.rpm",`
		`+ ]`
		`+`
		`+ def test_header_sizes(self):`
		`+ for basename in self.RPMFILES:`
		`+ fn = os.path.join(os.path.dirname(__file__), 'data/rpms', basename)`
		`+`
		`+ # the file length we want to match`
		`+ st = os.stat(fn)`
		`+ file_length = st.st_size`
		`+`
		`+ # An rpm consists of: lead, signature, header, archive`
		`+ s_lead, s_sig = koji.find_rpm_sighdr(fn)`
		`+ ofs = s_lead + s_sig`
		`+ s_hdr = koji.rpm_hdr_size(fn, ofs)`
		`+`
		`+ # The signature can tell use the size of header+payload`
		`+ # try LONGSIZE first, fall back to 32bit SIZE`
		`+ sighdr = koji.rip_rpm_sighdr(fn)`
		`+ rh = koji.RawHeader(sighdr)`
		`+ size = None`
		`+ try:`
		`+ tag = rpm.RPMTAG_LONGSIGSIZE`
		`+ size = rh.get(tag)`
		`+ except NameError:`
		`+ pass`
		`+ if size is None:`
		`+ size = rh.get(SIGTAG_SIZE)`
		`+`
		`+ # Expected file size`
		`+ calc_size = s_lead + s_sig + size`
		`+ self.assertEqual(calc_size, file_length)`
		`+`
		`+ # The following bit uses rpmlib to read the header, which advances the file`
		`+ # pointer past it. This is the same approach rpm2cpio uses.`
		`+ ts = rpm.TransactionSet()`
		`+ ts.setVSFlags(rpm._RPMVSF_NOSIGNATURES \| rpm._RPMVSF_NODIGESTS \| rpm.RPMVSF_NOHDRCHK)`
		`+ fd = os.open(fn, os.O_RDONLY)`
		`+ try:`
		`+ hdr = ts.hdrFromFdno(fd)`
		`+ p_offset = os.lseek(fd, 0, os.SEEK_CUR)`
		`+ finally:`
		`+ os.close(fd)`
		`+ expect_payload = s_lead + s_sig + s_hdr`
		`+ if not hdr:`
		`+ raise Exception("rpm did not return a header")`
		`+ self.assertEqual(p_offset, expect_payload)`

mikem commented a year ago

Based on a workaround by puiterwijk
https://github.com/fedora-iot/rpm-head-signing/pull/61/files

Fixes #3689

mikem commented a year ago

We should definitely double check that this is sane and correct.

For Koji's use, we're mostly concerned with signature headers. We have a function for ripping the main header too, but we never call it

Edited a year ago by mikem

rebased onto 7470bb9f553d8be36e54e0ee1e423c432db44af5

a year ago

rebased onto a04e00c

a year ago

mikem commented a year ago

There is a bit in the hub (_scan_sighdr) where we calculate the main header size in order to write out a signed copy with no payload. I doubt that the padding makes a difference there.

mikem commented a year ago

Trying to convince myself this is really correct.
A detour into the rpm code finds this bit in hdrblobRead

if (regionTag == RPMTAG_HEADERSIGNATURES) {
    size_t sigSize = uc + sizeof(rpm_header_magic);
    size_t pad = (8 - (sigSize % 8)) % 8;
    size_t trc;
    if (pad && (trc = Freadall(fd, block, pad)) != pad) {
        rasprintf(emsg, _("sigh pad(%zd): BAD, read %zd bytes"), pad, trc);
        goto exit;
    }
}

The uc value here is calculated the same as our initial hdrsize value (8 + 16 * il + dl). For signature headers only, rpm calculates the padding the same way we do and reads past it.

So it seems that only the signature is padded. I guess this is because they want the following header to be 8 byte aligned. I presume this is not needed for the main header because alignment does not matter for the payload the follows it. Meanwhile, the signature header itself is always 8 byte aligned because the lead has a fixed length of 96.

tkopecek commented a year ago

:thumbsup:

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

a year ago

1 new commit added

unit test

a year ago

mikem commented a year ago

Testing notes -- I've added a unit test based on a script I was using to validate this code. The test just uses the handful of test rpms we have in the repo. It reports a failure if I run it on the current main branch (as it should) and passes on this PR branch.

If anyone wants the original script for more extensive testing, just msg me.

2 new commits added