rpmfilesFSignature() returns a const pointer which callers must *not* free so this would mean every call to rpmfilesFSignature() leaks memory and we can't very well change the API in rpm 4.16. 
 

The different length case is just a peculiarity of ECDSA, right? How about we just declare that unsupported for file signatures in RHEL-9... 

BTW there's another leak in the implementation that fi->signatures is copied as independently allocated strings which are never freed. You don't need to do that, you could call headerGet() with HEADERGET_ALLOC to get a single allocated blob for the whole thing, and as this is a common operation in rpmfi, there's an ugly shortcut macro. Replacing the call to copy_array() with this should do it all, error checks included:

  _hgfi(h, RPMTAG_FILESIGNATURES, &td, defFlags, fi->signatures);

> The different length case is just a peculiarity of ECDSA, right? How about
> we just declare that unsupported for file signatures in RHEL-9... 

Product Management asked us to use ECDSA as the size is about 200 bytes vs 2048 - 4096 for RSA depending on the keysize, it also has a speed advantage. We can't invariably just go and mark it unsupported now as the ship has already sailed. 
 
> BTW there's another leak in the implementation that fi->signatures is copied
> as independently allocated strings which are never freed. You don't need to
> do that, you could call headerGet() with HEADERGET_ALLOC to get a single
> allocated blob for the whole thing, and as this is a common operation in
> rpmfi, there's an ugly shortcut macro. Replacing the call to copy_array()
> with this should do it all, error checks included:
> 
>   _hgfi(h, RPMTAG_FILESIGNATURES, &td, defFlags, fi->signatures);

It was marked as a draft patch for a reason, it was intended as a proof of concept not intended as the final solution as we aren't specialists in the rpm codebase.

So the ship sailed with a previously untested configuration. Nice.

(In reply to Panu Matilainen from comment #3)
> So the ship sailed with a previously untested configuration. Nice.

If we're going to do sarcastic replies I would ask where was the testing when the feature landed and where's the tests that would have would have picked this up from the outset.

I wasn't around at the time so cannot comment. However I have been making quite a lot of noise against IMA first in Fedora and then in RHEL because it's not production ready IMO but nobody listened. Yet here we are, and yes I am a little grumpy.

(In reply to Panu Matilainen from comment #5)
> I wasn't around at the time so cannot comment. However I have been making
> quite a lot of noise against IMA first in Fedora and then in RHEL because
> it's not production ready IMO but nobody listened. Yet here we are, and yes
> I am a little grumpy.

The fact of the matter is we have product requirements and security requirements and unfortunately these often drive things from not ready to ready. If some people had their way it would never be ready. We need to be able to secure products, and there is numerous ways of doing this, security needs multiple layers like an onion. Unfortunately sometimes if we sit around and wait until things are ready we miss that market. So the situation is what is it and salty comments doesn't help engineers like myself work to get us a working product. So I get that you're grumpy but please don't take it out on other engineers that are just trying to do their job.

Yes, stuff only gets ready by people pushing and working on them, but that gruntwork is supposed to happen in Fedora. We never received a single patch regarding IMA despite all the complaints people raised at the time, and apparently some different algorithm was used. I don't know if IMA even initially supported ECDSA - I certainly didn't know it did even now. Yet here we have the entire distro now signed with what is clearly a previously untested algorithm. That's not how these things are supposed to be done, which is what I'm grumpy about.

Actually the ECDSA key was used in Fedora when it was tested so it was the same algorithm. Bugs happen, software is complex. I some how doubt it's going to be the last issue we find.

We found one bug and provided a patch, the issue that was found in el8 and was fixed in 8.4.

Ultimately it wasn't enabled in Fedora due to various reasons and we had to have this in place for el9 GA because of the requirement of signing all rpms as part of the el9 shipping requirements. This is functionality we couldn't wait until el10 for. It's not the first time a feature landed in an enterprise release due to Fedora community push back, I doubt it will be the last.

Ultimately the timing was out of my control and you are of course allowed to be grumpy but that:
1) doesn't move us forward to get this bug fixed
2) is not useful for the Fedora/RHEL ecosystem
3) improve RHEL/Fedora's ability to provide users the ability to better secure their platforms

We do plan on resubmitting the feature to Fedora but timing/dealines/resources has restricted that and having to deal with politics/pitchforks hasn't helped that. We already have plans to resubmit it for F-36.

As for the actual issue, this is missing a reproducer entirely. I understand the description of the issue, but having a some real-world specimen is necessary to develop + validate a proper fix. Also there's zero indication as to what happens because of this, ie severity. A later, independent report at upstream seems to suggest this causes segfaults which is of course quite severe.

So a reproducer please, and then we can deal with this with an appropriate priority.

Created attachment 1843675 [details]
reproducer-rpm

The attached RPM will show this: this has two files (/usr/share/example1 and /usr/share/example2) that are both IMA signed with ECDSA, one has an 80 bytes long signature, the other 79 bytes.

When you look at the package contents with for example Koji's sighdr parsing:
import koji
sighdr = koji.rip_rpm_sighdr("test-1.0-1.fc34.noarch.rpm")
sighdr = koji.RawHeader(sighdr)
sighdr.dump()

This contains the following contents for FILESIGNATURES:
Tag: 274 [?], Type: 8, Offset: 232, Count: 2
String(162): b'030204a598255400483046022100e5117bdafa73baaeb1f1dc46ecaa46981a62d417745a33532572b63dc6d95d16022100c789107ac5b91e2d915e1df3c7b78414f6b3f50899d44c1de381d0e938dfc82b'
String(160): b'030204a598255400473045022100c10943795bff5d9c0db53dd4f8e4b845615fd08a2be295c30a80f5bdb4e6a41302203038840cc6abaab92acb56cb3e3ce520b17f22ff7444a8d5d0f703a44d5307a3'

Note that the second one is slightly shorter than the first.

And the following the FILESIGNATURELENGTH:
Tag: 275 [?], Type: 4, Offset: 378, Count: 1
[0, 0, 0, 80]


After installing the package with rpm-plugin-ima installed, this results in the following extended attributes:
  ▸ getfattr -e hex -n security.ima /usr/share/example2
getfattr: Removing leading '/' from absolute path names
# file: usr/share/example2
security.ima=0x030204a598255400473045022100c10943795bff5d9c0db53dd4f8e4b845615fd08a2be295c30a80f5bdb4e6a41302203038840cc6abaab92acb56cb3e3ce520b17f22ff7444a8d5d0f703a44d5307a3

  ▸ getfattr -e hex -n security.ima /usr/share/example1
getfattr: Removing leading '/' from absolute path names
# file: usr/share/example1
security.ima=0x030204a598255400483046022100e5117bdafa73baaeb1f1dc46ecaa46981a62d417745a33532572b63dc6d95d16022100c789107ac5b91e2d915e1df3c7b78414f6b3f50899d44c1de381d0e938dfc8


Note that /usr/share/example1 is missing the final byte of its signature: it ends in c8, instead of c82b, which would lead in an invalid signature.

(In reply to Patrick Uiterwijk from comment #12)

> The attached RPM will show this: this has two files (/usr/share/example1 and
> /usr/share/example2) that are both IMA signed with ECDSA, one has an 80
> bytes long signature, the other 79 bytes.
> 

Thanks, this should do. But if you have pointers to real-world packages with more files exhibiting this, that wouldn't hurt.

FWIW, this is easily observable with rpm cli queries, no need to program in Python:

$ rpm -qp --qf "[%{filesignatures}\n]" test-1.0-1.fc34.noarch.rpm 
warning: test-1.0-1.fc34.noarch.rpm: Header V4 RSA/SHA512 Signature, key ID 09daef21: NOKEY
030204a598255400483046022100e5117bdafa73baaeb1f1dc46ecaa46981a62d417745a33532572b63dc6d95d16022100c789107ac5b91e2d915e1df3c7b78414f6b3f50899d44c1de381d0e938dfc82b
030204a598255400473045022100c10943795bff5d9c0db53dd4f8e4b845615fd08a2be295c30a80f5bdb4e6a41302203038840cc6abaab92acb56cb3e3ce520b17f22ff7444a8d5d0f703a44d5307a3
$

Also when looking at header details, --xml query format is handy (if ugly) when you're not sure what you're looking for:

$ rpm -qpl --xml test-1.0-1.fc34.noarch.rpm 
[...]
  <rpmTag name="Filesignatures">
        <string>030204a598255400483046022100e5117bdafa73baaeb1f1dc46ecaa46981a62d417745a33532572b63dc6d95d16022100c789107ac5b91e2d915e1df3c7b78414f6b3f50899d44c1de381d0e938dfc82b</string>
        <string>030204a598255400473045022100c10943795bff5d9c0db53dd4f8e4b845615fd08a2be295c30a80f5bdb4e6a41302203038840cc6abaab92acb56cb3e3ce520b17f22ff7444a8d5d0f703a44d5307a3</string>
  </rpmTag>
  <rpmTag name="Filesignaturelength">
        <integer>80</integer>
  </rpmTag>
[...]

Proposed fix at https://github.com/rpm-software-management/rpm/pull/1844

Please test, I have only verified to the point that the installed signatures now match what's in the header and it doesn't blow up in the process.

Assigning to myself for the backporting work.

I was pointed to this BZ after reaching my wit's end trying to figure out why most of my package builds on the openSUSE Build Service have been failing for the past month: https://build.opensuse.org/project/show/home:Pharaoh_Atem:CS9EPELDev

If someone has a SRPM of rpm I can build with a test patch for CS9, I can build it and test to see if it fixes the issues I'm seeing in my build environments.

Created attachment 1844867 [details]
Scratch SRPM with the fix

Neal, I've attached an SRPM that you can try out.  I've tested it against the RPM provided by Patrick above and it worked.

Just to add - the SRPM is based off of the latest c9s branch at:
https://gitlab.com/redhat/centos-stream/rpms/rpm

I'm building the package here: https://build.opensuse.org/package/show/home:Pharaoh_Atem:CS9EPELDev:baseos-dev/rpm

I've configured it to be a dependency of https://build.opensuse.org/project/show/home:Pharaoh_Atem:CS9EPELDev

As rpm gets built in each of them, it'll trigger a mass build of all the packages in there.

If x86_64 gets to all green, then we did it. :)

So, I've now verified that this fixes my problem: https://build.opensuse.org/project/monitor/home:Pharaoh_Atem:CS9EPELDev?arch_x86_64=1&blocked=1&broken=1&building=1&defaults=0&deleting=1&dispatching=1&failed=1&finished=1&locked=1&repo_CentOS_9=1&scheduled=1&signing=1&succeeded=1&unresolvable=1

All packages successfully rebuilt in sequence! :)

Upstream PR:

https://github.com/rpm-software-management/rpm/pull/1914

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (new packages: rpm), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2022:4021