Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the internal OpenPGP parser #2414

Closed
pmatilai opened this issue Mar 7, 2023 · 26 comments · Fixed by #2986
Closed

Remove the internal OpenPGP parser #2414

pmatilai opened this issue Mar 7, 2023 · 26 comments · Fixed by #2986
Assignees
Labels
cleanup crypto Signatures, keys, hashes and their verification
Milestone

Comments

@pmatilai
Copy link
Member

pmatilai commented Mar 7, 2023

Continuing where #1935 left off: the internal OpenPGP parser has now been deprecated and declared essentially frozen, but this is a difficult and cumbersome position to hold for a number of reasons, including hindering other development work in this area.

We simply can't have Rust as a hard build dependency (at least not in a long while), so we need some exit strategy here. Basically, we either need to accept that rust-free means no signature checking at all, or we need to come up with an alternative, C-family based OpenPGP parser that we can actually use.

@pmatilai
Copy link
Member Author

pmatilai commented Mar 9, 2023

Actually there is a way to have the cake and eat it too: once upon a time, rpm called an external program (gpg back then) to verify signatures. It could do it again. It might be cumbersome to fit into the current API, it would be slower and there would be other compromises to be made no doubt, but between no security and slow security...

@DemiMarie
Copy link
Contributor

If RPM goes this route, it should keep a small part of the internal parser. Specifically, it should keep the checks that the signature is a single OpenPGP signature packet of the correct type. This is a workaround for a known and unfixed denial-of-service vulnerability in GnuPG that I reported back in 2022, and should not increase the maintenance burden significantly. It also ensures consistency with the Sequoia implementation, which has a much stricter parser than GnuPG has.

@pmatilai
Copy link
Member Author

Nope. Rpm is better off not knowing the damnest thing about OpenPGP format. An external helper would be considered a stop-gap measure for those unable/unwilling to use rpm-sequoia for some reason, nothing more.

Another possibility (and these aren't exclusive) is to split the existing parser to a separate project that can maintained by those wishing to do so. I simply want the parser, all of it, out of my hair.

@mlschroe
Copy link
Contributor

I'm in favor of an separate project. I'm willing to take maintainership if nobody else steps up...

@nwalfield
Copy link
Contributor

@mlschroe : Do you mind sharing your motivation. Does OpenSUSE plan to stick with the internal OpenPGP implementation?

@mlschroe
Copy link
Contributor

For now we'll stick with the internal implementation. That doesn't mean we will never switch to sequoia when it's proven to be mature enough in a year or two. (Thanks Fedora for beta testing ;-) )

@nwalfield
Copy link
Contributor

Thanks for the explanation.

@pmatilai
Copy link
Member Author

I looked at this a bit and properly externalizing seems quite a lot of work because of two-way dependencies between it and rest of rpm.

I didn't actually try it yet, but I think splitting this into a submodule rather than a separate project would be far more reasonably achievable. Thoughts on that, especially @mlschroe if you're to maintain it?

@pmatilai
Copy link
Member Author

pmatilai commented Oct 12, 2023

I did some cleanup surrounding this today, managed to remove quite a bit of related unused cruft that has been just sitting there for twenty years.

We're now annoyingly close to be able to bury the rpmpgpval.h table stuff into the internal parser too. The only things needing that data now in the rest of rpm are

  • pgpValString(PGPVAL_PUBKEYALGO, ...)
  • pgpValString(PGPVAL_HASHALGO, ...)

We shouldn't need 170 lines worth of data to answer those two questions...

@pmatilai
Copy link
Member Author

The really annoying part about this is that if it wasn't for the stupid pgpIdentItem() function in librpmio, we could just hide a these two val->string conversions into a private helpers someplace in librpm. Back when it was added, pgpIdentItem() was a shortcut to avoid exposing a struct or a tonne of API to access the individual items (which we now have). So the shortcut is biting back now, doh...

@kanavin
Copy link
Contributor

kanavin commented Nov 24, 2023

Just wanted to add the Yocto perspective: we don't have anything against sequoia, except its build dependencies. It needs both rust and clang (via one of the crates), rust and cland are both extremely heavy items to build, and we can't inject them into the core build sequence because it would lengthen builds far too much, even if you have a 96 core epyc machine to build on.

So outsourcing the crypto to external gpg executable would be very welcome. We can live with rpm verification disabled too.

@Conan-Kudo FYI

@DemiMarie
Copy link
Contributor

So outsourcing the crypto to external gpg executable would be very welcome.

This isn’t going to happen because spawning an external program breaks in too many situations.

We can live with rpm verification disabled too.

This is a terrible idea from a security perspective.

@kanavin
Copy link
Contributor

kanavin commented Nov 25, 2023

We can live with rpm verification disabled too.

This is a terrible idea from a security perspective.

In embedded linux world, production systems are rarely if ever updated from package feeds by a package manager. Rather, the whole root filesystem gets overwritten from an image file. Package manager is used to compose that root filesystem from local packages in a controlled CI environment (where package-level security isn't needed), and to allow developers to install additional items into a running system on their desks used for development and testing (where there's no need to sign packages either).

So Yocto can accept that regression in package security, we'll make sure to place warnings where appropriate.

@DemiMarie
Copy link
Contributor

We can live with rpm verification disabled too.

This is a terrible idea from a security perspective.

In embedded linux world, production systems are rarely if ever updated from package feeds by a package manager. Rather, the whole root filesystem gets overwritten from an image file. Package manager is used to compose that root filesystem from local packages in a controlled CI environment (where package-level security isn't needed), and to allow developers to install additional items into a running system on their desks used for development and testing (where there's no need to sign packages either).

So Yocto can accept that regression in package security, we'll make sure to place warnings where appropriate.

Another option would be to use the host system’s RPM for verifying the packages.

@kanavin
Copy link
Contributor

kanavin commented Nov 28, 2023

So Yocto can accept that regression in package security, we'll make sure to place warnings where appropriate.

Another option would be to use the host system’s RPM for verifying the packages.

Using host distro tools in cross-compilation builds is problematic, as we don't have control over what versions we're going to get, and how they are built and configured. To ensure things work in a reproducible manner, yocto builds its own rpm executable that can run on the build machine.

FWIW, the only host tools allowed to bootstrap the yocto build are python, gcc, wget, tar, git and various (de)compression utilities - things you need to fetch the sources, and bootstrap a cross-compiler.

@DemiMarie
Copy link
Contributor

So Yocto can accept that regression in package security, we'll make sure to place warnings where appropriate.

Another option would be to use the host system’s RPM for verifying the packages.

Using host distro tools in cross-compilation builds is problematic, as we don't have control over what versions we're going to get, and how they are built and configured. To ensure things work in a reproducible manner, yocto builds its own rpm executable that can run on the build machine.

What about fetching a correctly configured binary RPM and verifying its hash before using it?

FWIW, the only host tools allowed to bootstrap the yocto build are python, gcc, wget, tar, git and various (de)compression utilities - things you need to fetch the sources, and bootstrap a cross-compiler.

Do you also need tools to cryptographically verify the downloaded sources? Or is that done in Python?

@kanavin
Copy link
Contributor

kanavin commented Nov 28, 2023

Using host distro tools in cross-compilation builds is problematic, as we don't have control over what versions we're going to get, and how they are built and configured. To ensure things work in a reproducible manner, yocto builds its own rpm executable that can run on the build machine.

What about fetching a correctly configured binary RPM and verifying its hash before using it?

Possible, but comes with a significant support burden - someone needs to write and support the code that does this, and provide and update the binaries, for all of the architectures that yocto builds can run on. There needs to be tests and documentation too. And a possibility to opt out of it, and build rpm binary locally anyway.

When there are alternatives, we pick one which puts the least pressure on our very limited maintainer resources, or ideally makes it less than it was. The standard approach is to build the tools locally, and we'd rather stick with it.

FWIW, the only host tools allowed to bootstrap the yocto build are python, gcc, wget, tar, git and various (de)compression utilities - things you need to fetch the sources, and bootstrap a cross-compiler.

Do you also need tools to cryptographically verify the downloaded sources? Or is that done in Python?

For tarballs, their checksums are verified against local record of what they should be with python. For git checkouts we trust the git executable that the specified commit id will result in the correct tree, or there will be an error.

@DemiMarie
Copy link
Contributor

@kanavin Are all of the RPMs used also built locally? In that case disabling signature checking is fine.

FYI, both rustc and clang are native cross compilers with support for multiple targets. The same rustc and clang that are used to compile programs for the build environment can also be used to compile code for the target system.

@DemiMarie
Copy link
Contributor

The reason for getting rid of the internal OpenPGP parser is that it turns out to have security vulnerabilities that are exploitable if someone does gpg2 --export --armor -o s.asc FINGERPRINT && rpmkeys --import s.asc. Patching these vulnerabilities isn’t practical, as it would require a whole bunch of logic nobody is interested in implementing.

@kanavin
Copy link
Contributor

kanavin commented Nov 28, 2023

@kanavin Are all of the RPMs used also built locally? In that case disabling signature checking is fine.

Yes of course. Yocto is fully self-contained, except for the bootstrap items mentioned above. It builds components from source, then makes its own packages from the binaries, then makes a system image from those packages. You can also pick what package format you prefer: ipk/deb/rpm, or all three.

@DemiMarie
Copy link
Contributor

@kanavin Are all of the RPMs used also built locally? In that case disabling signature checking is fine.

Yes of course. Yocto is fully self-contained, except for the bootstrap items mentioned above. It builds components from source, then makes its own packages from the binaries, then makes a system image from those packages. You can also pick what package format you prefer: ipk/deb/rpm, or all three.

Ah, then there is not a security concern at all.

@pmatilai
Copy link
Member Author

Finally managed to convince myself that it should be feasible (with reasonable amount of work) to have a "nopgp" build option by adding a dummy implementation of the internal PGP interface that just returns -ENOTHOME for everything, and allow choosing between libgcrypt and openssl for the hash functionality using existing code.

That way there's a minimal bootstrap option capable of building legitimate rpm packages, without making people choose between Rust or unmaintained security code with known holes.

@pmatilai
Copy link
Member Author

#2984 implements the dummy PGP option.

pmatilai added a commit to pmatilai/rpm that referenced this issue Mar 20, 2024
Now that we have an alternative to building without Rust, it's time
to say bye to this old thing. We will not support the parser but
preserve minimal hooks in cmake to allow building with it, at least
for a transition period:

	https://github.com/rpm-software-management/rpmpgp_legacy

Fixes: rpm-software-management#2414
@pmatilai
Copy link
Member Author

I'm in favor of an separate project. I'm willing to take maintainership if nobody else steps up...

@mlschroe , shall I transfer the ownership of https://github.com/rpm-software-management/rpmpgp_legacy to you? Otherwise I'll just archive the thing.

@mlschroe
Copy link
Contributor

Yes, I'll take the ownership for now. Thanks.

@pmatilai
Copy link
Member Author

Okay, made you the admin of that repo. Have fun, as they say 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup crypto Signatures, keys, hashes and their verification
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants