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

Specify how digital signatures are to be crafted on Type-2 #30

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ssaavedra
Copy link

The previous specification does not provide enough information on how to properly construct valid signatures in the AppImage ecosystem.

For interoperability, and using the current implementation as a reference, this changeset specifies how to construct valid signatures for Type-2 AppImages that are compatible with the current toolset.

C.f.: AppImage/AppImageKit#1010

@probonopd
Copy link
Member

probonopd commented Mar 26, 2020

Thank you very much @ssaavedra. Looks like this adequately describes the current implementation, but are we sure that we want to mandate gpg-style signatures? Should we also allow OpenSSL style signatures? @TheAssassin, all, what do you think?

@shoogle
Copy link
Contributor

shoogle commented Jun 24, 2020

Make it simple and allow one type only. "Can your application verify AppImage signatures?" should be a Yes / No question, not "We support some signatures but not others".

@probonopd
Copy link
Member

probonopd commented Jul 30, 2020

While we are at it, we should probably also document how the signature has to be made and checked (e.g., how the digest works and what has to be skipped). Issue with this is that current implementation differs from what I had in mind (even using an undocumented section), so need to discuss with @TheAssassin what to do.

draft.md Outdated
MUST NOT have text other than whitespace following them on the same
line.

* This means that a valid digital signature **MUST** begin with a newline character, and end with a newline character, and **SHOULD** be followed by as many `0x00` bytes as required in order to fill the ELF section.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to also describe the .sig_key section, which is currently the only way how to deliver pubkeys along with the signatures to allow for validating at all.

Copy link
Member

@probonopd probonopd Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires discussion. .sig_key is not part of the spec and has never been formally proposed or discussed.

What good is it to deliver the public key along with the payload? Doesn't this circumvent the whole concept? Do you know any example where a public key is delivered in the same file as the payload to be verified?

The way I think signing should work:

  • With the default AppImageLauncher/appimaged/AppImageUpdate/whatever installation, the public key for the official AppImage tools is delivered
  • AppImageUpdate verifies that the signature of the new version matches the signature of the old version (if both are signed). In other words, AppImageUpdate makes sure that by updating you cannot suddenly get an AppImage that has a different signature.
    • If the old AppImage had no signature, then the status is "grey"
    • If the old AppImage had no signature but the new AppImage has a signature, then the state is "grey"
    • If the old AppImage had a signature and the new AppImage has a signature and they do not match, then the state is "red"
    • If the old AppImage had a signature and the new AppImage has a signature and they match, then the state is "yellow"
  • If you as the user want even more security, you need to manually get, verify, and import the public key for that AppImage into you keyring.
    • If the old AppImage had a signature and the new AppImage has a signature and they match and the public key is found in the user's keyring, then the state is "green"

Can you follow my logic? "Grey" means "I don't know". "Red" means "something bad is going on, someone tried to tamper with something". "Yellow" means "basic checks passed, but no full security". "Green" means "full security". Getting "Green" may require the user to do something, or otherwise we cannot really guarantee this security level.

Or can we?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About "any example including the public key along the payload", you can see that with email, for example. In particular, etiquette for key-signing includes each party attaching their public key and signing the multipart data. Protonmail even has UI to include the public key when sending emails, which I would believe is on by default.

In any case, since public key distribution is its own can of worms (and public distribution has issues like the SKS DoS attacks), if we allow including the public key along the payload we solve the distribution problem entirely by leveraging whichever distribution channel the AppImage used to get into the user. Besides, the fact that it is named public key does not mean that you want to distribute the key content, maybe the AppImage content is a trade secret and will never see the light of day but still want to check it's packaged by a trusted source.

Whether you include the public key or not, the security level of the checks can be the same you propose. The statuses can be the same, i.e., we can still check the old version of the AppImage (and the old public key and see if they match) but by including it on the AppImage we remove the requirement to download signatures off-channel, and we allow operation in air-gapped systems. Adding the public key is public information anyway, and therefore cannot be detrimental to security, but it can be convenient.

But what's more, by including the public key, we could even think of adding signatures among keys, or backup keys, so that future versions could be signed by another key but only if there is a signature from the parent key to the next. Anyway, I would move that discussion further down the road, and just stick with single keys for now, since without PKI and OCSP, checking key validity is not straightforward.

About the status:

I would insist on disagreeing with this state rule: when old Appimage has no signature and new AppImage has one, the state should be at least "orange", not "grey".

You could hijack a non-signed AppImage by injecting once a signed one. In this sense, we should at least warn the user that the update will be signed and that henceforth signed-appimage rules will be applied on updates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheAssassin I had not included it because although I believe what I just said, the discussion as to whether to include the field was happening on another issue, and I would have wanted to see this merged soon (although in retrospective it hasn't happened so soon 😄 )

Copy link
Member

@probonopd probonopd Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should I trust a key that comes inside an AppImage more than the rest of the AppImage?

In that case I would at least need to manually verify the key fingerprint before I could trust the key. Would that bring any advantage over importing the key into the keyring?

Copy link
Member

@TheAssassin TheAssassin Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does not shipping the key solve any issue?

Why should I trust a key that comes inside an AppImage more than the rest of the AppImage?

Well, it's as simple as "you don't". AppImageUpdate doesn't trust these keys automatically. It builds "transitive trust", by enforcing that the key in the old AppImage must be the same with which the new AppImage has been signed. But you first need the key at all.
This concept is not new; in fact, many software projects (e.g., Conversations) have implemented similar mechanisms.
It's been thoroughly discussed back in the days, and I'm pretty sure you were part of it. It just never made its way into the spec.

Arguments like "it's insecure" or anything are just pure nonsense, and shouldn't prevent its inclusion. It's not insecure at all. You just have a wrong imagination of what guarantees the signature can provide. The plain truth is, these signatures don't add any kind of security outside AppImageUpdate, which is the only tool that can make any use of them.

It has never been said that this key is to be trusted automatically. But not shipping doesn't increase security or anything. You are free to verify a key's fingerprint externally.

By the way, you realize that this validate tool has never provided any functionality with regards to security, don't you? Its mere purpose is to check that signing was successfully done in the first place. That's more an integrity check than anything security related.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It builds "transitive trust", by enforcing that the key in the old AppImage must be the same with which the new AppImage has been signed. But you first need the key at all.

This completely makes sense to me. But why do we need to have the key to check that two AppImages were signed by the same key?

Copy link
Member

@TheAssassin TheAssassin Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the key just for that (but also). You need the public key to verify the signature at all. Chicken egg problem: how to verify something if you don't have the key? Without shipping the key, verification can't take place at all. Once you can verify, you can then start to build transitive trust.

Copy link
Member

@probonopd probonopd Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

I guess we should keep a short version of the explainations near the spec (under a "Why?" link).

@ssaavedra
Copy link
Author

Is there any reason to prevent this from getting into the draft? I've reviewed the text and modified it a bit with the hope to keep it as unambiguous as possible.

@probonopd
Copy link
Member

Hi @ssaavedra. Thank you very much for your contribution.

@TheAssassin and I need to fact-check it. Also, I think the current implementation is more complicated than it needs to be but I don't remember the exact specifics (something about some identifier that had been quietly introduced into the implementation at some time). So it's questionable whether we want to document the current behavior, or whether we want to simplify it first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants