Repository navigation
Conversation
…tpakref `runtime_repo_keyfile` is documented as being nullable, and indeed if we try and install a flatpakref and pass `--no-deps` to avoid having to build a flatpakref for th runtime as well, flatpak crashes. This will be exercised by an upcoming test. Signed-off-by: Philip Withnall <[email protected]>
And support it in the `flatpak remote-{add,modify}` commands.
Signed-off-by: Philip Withnall <[email protected]>
Helps: flatpak#6517
This commit just adds the options and plumbs them up, but doesn’t do anything material with them yet. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
This will be used to allow a signed summary to update/rotate GPG keys. This commit just plumbs in the summary field, and doesn’t do anything material with it. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
This will be used in some upcoming GPG key work. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
This is exactly the same as what OSTree does in `ot_gpgme_ctx_tmp_home_dir()`: creates a new empty `pubring.gpg` in the temporary GPG home directory, if one hasn’t been copied in from the OSTree repository. This prevents GPG itself from creating one in the newer keybox format, which we don’t want to work with. Signed-off-by: Philip Withnall <[email protected]>
This is the core of flatpak#6517: a function which validates and potentially imports an updated GPG keyring for a remote. See the comments in the code (particularly on `flatpak_gpg_keys_validate_binding()`) for the full security model, but briefly: - New subkeys on an existing trusted primary key are always imported (including their revocation certificates, if they exist) - New primary keys are imported if they are cross-signed by a trusted key, including if that key is newly trusted as part of this import (i.e. chains of trust are allowed) - Expired keys and subkeys are imported (if trusted) as apps may have been signed with them before they expired - Revoked keys are not imported as revocation is an explicit signal to not trust a key This potentially imports an updated keyring on a remote with gpg-keys-url set every time the summary is updated for that remote, with appropriate HTTP caching. Tests and sysadmin documentation for the new functionality will come in following commits. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
This allows a gpg-keys-url option to be set in the repo summary, broadcasting an out-of-band update mechanism for the repository’s keyring to clients. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
This allows a gpg-keys-url option to be set in the bundle summary, broadcasting an out-of-band update mechanism for the keyring of the repository which generated the bundle. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
This allows viewing the gpg-keys-url configuration for a remote, using `flatpak remotes --column=gpg-keys-url`. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| The downloaded keyring is assumed to be non-malicious to the extent | ||
| that this is required by GPG’s code which handles loading an unknown | ||
| external keyring. Risks from injected malicious (malformed) keyrings | ||
| can be mitigated by providing a HTTPS |
There was a problem hiding this comment.
I think it should be prominently emphasized that this trust model depends on HTTPS for client system security, since an attacker who gains control of the HTTPS server can publish whatever keys they choose. Clearly stating this HTTPS-dependence both here and someplace more prominent (perhaps in or near the top level of the docs) would be wise, given how many systems trust software from flatpak repos, and how common it has become for web servers to be compromised.
Without such a warning, it would be easy for users to assume that they are using a trust-on-first-use security model, when in fact they are using a trust-whenever-the-HTTPS-server-changes-something model. The latter provides much weaker protection.
Or have I misunderstood or missed something? Is there a mechanism that I have overlooked for establishing trust in these newly delivered keys, that can stand up to web server compromise? (Compare Debian's APT, for example, which doesn't depend on HTTPS for security.)
There was a problem hiding this comment.
I think you’ve misunderstood a key part of the security model: keys are only imported if they’re signed by a key already in the trust root.
That means the security model does not depend on HTTPS. If an attacker gained control of the server, they could publish whatever keyring they liked, but unless they gained control of one of the secret keys already in clients’ trust roots, they could not publish a keyring with a key those clients would trust and import, as they could not sign it.
This paragraph is about a much lower risk: if the keyring were served over HTTP, and a middleman attacker substituted it for a malicious keyring, the risk is that they could cause the parsing code in GPGME to crash and give a denial of service. The attacker could not forge a signature which would cause their keys to be imported by clients.
I assume that GPG/GPGME is quite robust against malformed keyrings, but I have not tested this, hence my suggestion that keyrings should be served over HTTPS to eliminate the risk of middlemen.
There was a problem hiding this comment.
keys are only imported if they’re signed by a key already in the trust root.
I see the conditions for importing a key that say, "It contains at least one UID which is not revoked or invalid," and "That UID is signed by a key already in the trust root," but that wording refers to a signed UID, rather than a signed key.
Did I overlook another section that requires a newly published key to be signed by an already trusted key in order to allow importing/trusting it?
Or maybe when you wrote "UID" you meant "key"? These are distinct concepts in the GPG tools and docs, where an email address is an example of a UID. Obviously, anyone can generate a key with an email address that has also been signed by a Flathub-issued (and widely trusted) key. Hence my concern.
What am I missing?
There was a problem hiding this comment.
A UID does refer to a pair of (key, identifier) and GnuPG's own docs do that.
I can't see how there's any other interpretation of UID here other than to mean key: what is a signed UID if not a signed key, and what does it mean for a signed UID signed by an already trusted key vs a signed key signed by an already trusted key?
Anyway, I guess it can be changed to say "at least one key which is ..." and "that key is already signed by a key ..." but I don't think there's an actual difference here that leads to any meaningful alternative interpretation. I may well be wrong though, PGP terminology is good at tripping one up.
There was a problem hiding this comment.
I see the conditions for importing a key that say, "It contains at least one UID which is not revoked or invalid," and "That UID is signed by a key already in the trust root," but that wording refers to a signed UID, rather than a signed key.
A UID is bound to a primary key (that’s an inherent part of how GPG works). When you sign someone’s GPG key (or a key in the trust root here signs another key to create a chain), it’s actually signing a (key, UID) binding, as Sam says.
The documentation in this PR regarding this is correct.
There was a problem hiding this comment.
A UID does refer to a pair of
(key, identifier)
Maybe sometimes, but certainly not always. For example, those of us who use the gpg --edit-key command will be familiar with uid referring to a string containing name and email address, not to a key. Another example is OpenPGP RFC 4880 section 5.11: User ID Packet, which states, "A User ID packet consists of UTF-8 text that is intended to represent the name and email address of the key holder."
and GnuPG's own docs do that.
(Strictly speaking, that page doesn't use the term UID at all. I mention this not to quibble, but as a side note: Replacing "UID" with "User ID" in this document would better align it with well-established documentation, and therefore might be worth considering. I find that speaking strictly is often worthwhile when documenting security behavior. Now back to the issue at hand...)
I can't see how there's any other interpretation of UID here other than to mean key: what is a signed UID if not a signed key, and what does it mean for a signed UID signed by an already trusted key vs a signed key signed by an already trusted key?
The problem is that, as currently written, Mallory can satisfy the key import conditions by creating a new key containing a UID (email address) that he found on an already-trusted Flathub key. Mallory's new key then "contains at least one UID which is not revoked or invalid", and "that UID is signed by a key already in the trust root, and that signature is more recent than any revoked signatures, and is not expired or invalid."
If Mallory's new key was then placed on the server, clients following this Security Model doc would be obligated to import it into the trust root.
There was a problem hiding this comment.
When you sign someone’s GPG key (or a key in the trust root here signs another key to create a chain), it’s actually signing a (key, UID) binding,
That is indeed one way in which GPG signs UIDs, but it's not the only way. Notably, anyone can add whatever UID they like to their own key. This, too, involves signing, and the OpenGPG RFC calls the underlying mechanism a self-signature.
The documentation in this PR regarding this is correct.
See my previous comment for a description of how the documentation in this PR describes conditions that are exploitable.
If we mean a (key, UID) pair, then we should say a (key, UID) pair. Treating that as implicit would be a big mistake IMHO. (I suspect "key" would likely be sufficient in this particular case, though.)
There was a problem hiding this comment.
A UID does refer to a pair of
(key, identifier)Another example is OpenPGP RFC 4880 section 5.11: User ID Packet, which states, "A User ID packet consists of UTF-8 text that is intended to represent the name and email address of the key holder."
That’s within the context of the same document saying that User ID packets are found within a public key packet.
and GnuPG's own docs do that.
(Strictly speaking, that page doesn't use the term
UIDat all. I mention this not to quibble, but as a side note: Replacing "UID" with "User ID" in this document would better align it with well-established documentation, and therefore might be worth considering. I find that speaking strictly is often worthwhile when documenting security behavior. Now back to the issue at hand...)
Sure, I can s/UID/User ID/ in the docs next time I update them, that would improve clarity.
I can't see how there's any other interpretation of UID here other than to mean key: what is a signed UID if not a signed key, and what does it mean for a signed UID signed by an already trusted key vs a signed key signed by an already trusted key?
The problem is that, as currently written, Mallory can satisfy the key import conditions by creating a new key containing a UID (email address) that he found on an already-trusted Flathub key. Mallory's new key then "contains at least one UID which is not revoked or invalid", and "that UID is signed by a key already in the trust root, and that signature is more recent than any revoked signatures, and is not expired or invalid."
If you can demonstrate that as an exploit then I’ll take this seriously, but I am fairly sure that’s not possible by construction.
If Mallory's new key was then placed on the server, clients following this Security Model doc would be obligated to import it into the trust root.
Perhaps there’s a misunderstanding here: this documentation exists to document flatpak’s implementation of key rotation, for the benefit of people trying to maintain and understand it in future. The client is always flatpak, using the implementation in this PR.
It’s not meant to be a spec to allow a clean-room reimplementation of the functionality, nor a formal specification of every detail of how it works. Interpreting it requires some context and some knowledge of how GPGME and GPG work. If I were to specify absolutely everything in the doc, it would become less useful as documentation for maintenance, through containing too much detail.
I care about inaccuracies in the document which would affect the understanding of someone who is trying to maintain the accompanying code (and who is looking at that code at the same time). I care about exploits in the code. I don’t care about exploits based on reading the document without reading the code.
If we mean a (key, UID) pair, then we should say a (key, UID) pair. Treating that as implicit would be a big mistake IMHO. (I suspect "key" would likely be sufficient in this particular case, though.)
So on that note, I think it’s clearer to call a user ID a ‘user ID’ than it is to call it a ‘(primary key, user ID) pair’. To anyone familiar with how GPG works, the latter is unnecessarily verbose, and raises the possibility that a user ID could exist outside a primary key, which I don’t believe is the case.
The only reason I even mentioned user IDs in the document is because the GPGME API leads the code to be structured into iterating over keys, then UIDs, then subkeys. For the purposes of explaining the security model it would be more straightforward to just talk about primary keys, since flatpak only ever has one user ID within a signing primary key, and doesn’t pay attention to it.
There was a problem hiding this comment.
The problem is that, as currently written, Mallory can satisfy the key import conditions by creating a new key containing a UID (email address) that he found on an already-trusted Flathub key. Mallory's new key then "contains at least one UID which is not revoked or invalid", and "that UID is signed by a key already in the trust root, and that signature is more recent than any revoked signatures, and is not expired or invalid."
If you can demonstrate that as an exploit then I’ll take this seriously, but I am fairly sure that’s not possible by construction.
I don't know what "by construction" means here.
In any case, I'm not going to launch an attack just to demonstrate my point, but as far as I can tell, the following key (mallory.gpg) satisfies the conditions in question:
It contains a UID that is not revoked or invalid, and that UID is signed by a key already in the trust root. (That signature can be found in the official Flathub public keyring.)
Perhaps there’s a misunderstanding here:
[...]
I care about inaccuracies in the document which would affect the understanding of someone who is trying to maintain the accompanying code (and who is looking at that code at the same time).
Okay, but let's also keep this in mind: People deciding whether Flatpak will meet the security needs of their organization are likely to start by reading the documentation on its security model, which includes this document. When they spot flaws in the logic described therein, they are not likely to delve into the code to see if today's implementation happens to be better than the documented model; they will instead dismiss Flatpak as not suitable.
Do you care about that?
There was a problem hiding this comment.
The problem is that, as currently written, Mallory can satisfy the key import conditions by creating a new key containing a UID (email address) that he found on an already-trusted Flathub key. Mallory's new key then "contains at least one UID which is not revoked or invalid", and "that UID is signed by a key already in the trust root, and that signature is more recent than any revoked signatures, and is not expired or invalid."
If you can demonstrate that as an exploit then I’ll take this seriously, but I am fairly sure that’s not possible by construction.
I don't know what "by construction" means here.
It means that the way the GPG signature packets are specified means that they always bind to a (key, user ID) pair. It’s not possible to construct a signature packet which just binds to a user ID.
In any case, I'm not going to launch an attack just to demonstrate my point, but as far as I can tell, the following key (mallory.gpg) satisfies the conditions in question:
It contains a UID that is not revoked or invalid, and that UID is signed by a key already in the trust root. (That signature can be found in the official Flathub public keyring.)
$ mkdir foob
$ gpg --homedir=./foob --import flathub.gpg
gpg: key 4184DD4D907A7CAE: public key "Flathub Repo Signing Key <[email protected]>" imported
gpg: Total number processed: 1
gpg: imported: 1
$ gpg --homedir=./foob --import mallory.gpg
gpg: key B6A048C1DE43AE7A: public key "Flathub Repo Signing Key <[email protected]>" imported
gpg: Total number processed: 1
gpg: imported: 1
$ gpg --homedir=./foob --list-signatures
-----------------------------------------
pub rsa4096 2017-06-16 [SC] [expires: 2027-06-14]
6E5C05D979C76DAF93C081354184DD4D907A7CAE
uid [ unknown] Flathub Repo Signing Key <[email protected]>
sig 3 4184DD4D907A7CAE 2017-06-16 [self-signature]
sub rsa4096 2017-06-16 [S] [expires: 2027-06-14]
sig 4184DD4D907A7CAE 2017-06-16 [self-signature]
pub rsa4096 2026-09-06 [SC]
6806B0299ACCECA795394813B6A048C1DE43AE7A
uid [ unknown] Flathub Repo Signing Key <[email protected]>
sig 3 B6A048C1DE43AE7A 2026-09-06 [self-signature]
sub rsa4096 2026-09-06 [S]
sig B6A048C1DE43AE7A 2026-09-06 [self-signature]I don’t see any signatures there on the mallory key from the official key.
ea338c8 to
54056d7
Compare
GPG keys are used for signing OSTree commits and summaries by flatpak,
and hence the subkey we use should have its signing bit set (and not
have its encryption bit set).
This has not caused problems before, because GPG will automatically use
the right (the only) subkey even though the usage bits are wrong,
because there’s no other option.
But my work at the moment is adding additional subkeys in some of the
tests, and getting GPG to use the correct subkey to sign things in each
test is not possible if the usage bits are not set correctly.
Hence, change the usage mode of key 1, using `change-usage` in the
interactive session, to enable signing and disable encryption.
I have also unset the signing bit on the primary key (`key 0`), for the
same reasons.
Finally, the cross-certification between subkeys was refreshed using
`cross-certify` in the interactive GPG session.
The base64-encoded copies of the keys in `libtest.sh` needed to be
updated, but instead of regenerating them verbatim I made it dynamic to
avoid them getting out of sync in future.
Overall, this changes the output of
`gpg --homedir $srcdir/tests/test-keyring --list-keys` from:
```
$srcdir/tests/test-keyring/pubring.gpg
----------------------------------------------------------
pub rsa2048 2016-02-25 [SC]
3718EEBEB5740A7AB3D651B7138B31E07B0961FD
uid [ unknown] Xdg-app testing
sub rsa2048 2016-02-25 [E]
```
to:
```
$srcdir/tests/test-keyring/pubring.gpg
----------------------------------------------------------
pub rsa2048 2016-02-25 [C]
3718EEBEB5740A7AB3D651B7138B31E07B0961FD
uid [ unknown] Xdg-app testing
sub rsa2048 2016-02-25 [S]
```
Signed-off-by: Philip Withnall <[email protected]>
This will be used in key rotation tests.
It was generated using:
```
gpg --homedir ./path/to/test-keyring \
--output 3718EEBEB5740A7AB3D651B7138B31E07B0961FD.rev
--gen-revoke 3718EEBEB5740A7AB3D651B7138B31E07B0961FD
```
Signed-off-by: Philip Withnall <[email protected]>
Helps: flatpak#6517
These will be used in upcoming new tests. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
54056d7 to
663f077
Compare
thesamesam
left a comment
thesamesam
left a comment
There was a problem hiding this comment.
I had a quick look and only two small comments, but I'm not familiar with the Flatpak codebase. We do however use GnuPG a lot and the tests cover what I was looking fr.
This makes it less likely for tests to break in future if GPG changes its human readable output format. The `gpg` man page also recommends `--status-fd`, but since we don’t need more detailed status information let’s not pass that for now. Suggested by Sam James: flatpak#6798 (comment) Signed-off-by: Philip Withnall <[email protected]>
These depend on being able to specify subkeys for signing operations in OSTree, which is ostreedev/ostree#3633. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
663f077 to
35fcdac
Compare
This documents the key rotation feature (`xa.gpg-keys-url`), its security model, and an example of how it’s intended to be used by repository administrators. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
|
Two things that are missing from the docs IMO:
I was trying to figure out if the sideloading via the summary has any issues. I was a bit worried about an attacker which managed to take over the infra and with it the key. The summary then also becomes attacker controlled, and a key rotation might be prevented because of that. Arguably if one can't restore access to the summary somehow, then there are bigger problems, so I guess this is fine? But then, why do we have the repo configuration? Isn't the summary enough? |
I think these two are covered by the first two paragraphs of the ‘Security Model’ section. It’s brief, but I’m not sure there’s anything more to say about them. Was something in particular unclear to you?
If the attacker takes over the server pointed to by the If the web server contains the private half of the repo’s key, then it’s been set up incorrectly and is asking to be compromised. The private half of the repo’s key should be kept in a HSM, offline. Exactly how a particular repo does this is flexible, but the configuration I’d recommend is in the first paragraph of the ‘System Administration’ section.
If an attacker gains control of the server and replaces the summary file with their own, no client will trust it. This does mean they have achieved denial of service to the clients, but they could equally well do that by deleting all content on the server; they don’t need to mess about with replacing summary files.
The summary should be sufficient by itself, but also allowing the |
35fcdac to
aa2a4bc
Compare
This is by no means a full set of ShellCheck fixes, but might be enough to get it to stop shouting at me on flatpak#6798. https://www.shellcheck.net/wiki/SC2046 Signed-off-by: Philip Withnall <[email protected]>
The following commits will add another command line option to it, and that’ll be a right faff to deal with without proper command line parsing, so port the existing parsing to `GOptionContext`. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
Aggressive caching is normally absolutely excellent (and more software should do it). However, in an upcoming commit we want to be able to double-check with the server that our cached copy of a GPG keyring is definitely up to date. Add a flag to `flatpak_cache_http_uri()` to allow the max-age of the cache data to be ignored. This will force the code to query with the server (sending the cached ETag if known), to either get the latest file content (if changed) or a 304 response (if the cached file is still valid). Without this, `flatpak_cache_http_uri()` will just return the cached file from disk if it’s not particularly old yet. In cases where we have an external signal to hint that our cache is out of date (like a GPG signature failing to verify due to being by an unknown key), we want to force a check. Signed-off-by: Philip Withnall <[email protected]> Helps: flatpak#6517
|
Pushed with some minor docs updates (as discussed above), and again with the remaining commits to complete the feature (which allow for keys to be updated proactively on failure to pull a flatpak). Still ready for review, let me know if there’s anything else I can do to help with that :) |
pwithnall
changed the title
If we’re trying to pull the summary or a commit, and the signature verification fails (e.g. due to not knowing the key which made the signature), take that as a signal that our local keyring might be out of date and we should update it from the `xa.gpg-keys-url` (if known). This is the last piece in the GPG key rotation puzzle: it allows for clients which have been offline for a long time (since before a new subkey was published, until after the old subkey expired) to still grab an updated keyring if they go straight into trying to install something when they come back online (as opposed to refreshing the summary). Signed-off-by: Philip Withnall <[email protected]> Fixes: flatpak#6517
4f13e54 to
f545eeb
Compare
This branch has not been deployed
5 participants
This is the bulk of the work to fix #6517. Still remaining after this is:
xa.gpg-keys-urlif signature verification fails when pulling a summarySee the commit messages for details. This implements the high level plan outlined in #6517, but with a lot more detail put into the key verification algorithm. It’s documented in the gtk-doc comment for
flatpak_gpg_keys_validate_binding()and at a higher level indoc/flatpak-key-rotation.xml. This PR was put together completely without the use of LLMs.A few things/pending questions to highlight for review:
xa.gpg-keys-urlto be HTTPS? Currently the code allows any URI scheme, but I can’t think of a good reason for allowing that. It could always be relaxed later, so I’m tempted to enforce HTTPS inflatpak build-update-repo, but allow any scheme in the client-side download code.flatpak-gpg-keys.c), as they’re all unlikely to be seen and are very technical. I can mark them for translation if you’d prefer though.!ostreedev/ostree#3633. It would be good if someone could review that, as if it’s rejected then there may need to be significant changes in this PR. I’ve added askip_without_ostree_versionto the relevant new tests, but since the OSTree PR hasn’t been merged/in a release yet, I can’t put the right version number on that. So probably all the new tests will be skipped for you unless you comment that line out and run with a patched OSTree build.