search

AppImage / AppImageSpec

This repository holds the specification for the AppImage format.
http://appimage.org/
MIT License
71 stars 22 forks source link

Specify ".digest_md5" and ".sig_key" sections #29

Open probonopd opened 5 years ago

probonopd commented 5 years ago

Specify .digest_md5 section

@TheAssassin I am not 100% sure anymore what it is and why we have it - can you help me?

TheAssassin commented 5 years ago

https://github.com/AppImage/AppImageKit/blob/master/src/appimagetool.c#L961

probonopd commented 5 years ago

Can you please describe it in words that can be added to AppImageSpec? Every other aspect of the format is described in the spec.

Also, I don't quite remember the reason for this. Is the following halfway correct?

If it is correct until here,

TheAssassin commented 5 years ago

The use case for AppImageLauncher is to be able to properly differentiate between AppImages. Many people don't version their AppImages. By embedding something that is veeeeery likely unique, we can tell them apart.

You guessed the reason why a hash over an entire file cannot be embedded within that file. The .digest_md5 hash is calculated by assuming the section it is embedded into is zeroed. That way, one can embed it. The hash of the entire file subsequently changes, but if you calculate the hash as described, it'll always be the same as the embedded one.

probonopd commented 5 years ago

By looking at

https://github.com/AppImage/libappimage/blob/4d6f5f3d5b6c8c01c39b8ce0364b74cd6e4043c7/src/libappimage_shared/digest.c#L9

it seems to me like for the generation of the digest, the ELF sections

are skipped.

Questions:

TheAssassin commented 5 years ago

Please don't change your post too often. I provide quotes of the current question I'm answering to provide the original context.

Why is it a md5 (deemed insecure) and not a SHA256 like we use already in https://github.com/AppImage/AppImageKit/blob/801e789390d0e6848aef4a5802cd52da7f4abafb/src/validate.c?

MD5 is not "insecure" per se, only when used in security relevant contexts. We just use it for some collision detection or other uses where you'd like to identify an AppImage in a better way than just by its filename. There's no reason not to use MD5, but a lot reasons to actually do so. It's much less complex than SHA-2, and can be calculated faster, saving runtime.

Couldn't the code be unified with https://github.com/AppImage/AppImageKit/blob/801e789390d0e6848aef4a5802cd52da7f4abafb/src/validate.c which basically seems to do a very similar thing?

I never touched this validate.c thin, as it doesn't make sense (to me at least). Don't know what its goal is, but security wise it doesn't make too much sense. The title is misleading, AFAICS, it only can (or could in the past) be used to check whether a signature in an AppImage has been made correctly. But that's useless on any computer other than your own. It doesn't provide any security. The only real world scenario you could use it for is a post-build error check.

Did introducing the additional sections break https://github.com/AppImage/AppImageKit/blob/801e789390d0e6848aef4a5802cd52da7f4abafb/src/validate.c because it was not updated to skip those sessions, too? Wouldn't that mean that all signature checking using validate.c fails?

See above.

What is .sig_key and where is it specified? The only mention of it I can see is in AppImage/AppImageKit#801. Nothing in AppImageSpec. How come that undocumented stuff like this creeps in? AppImage/AppImageKit@1beecf3#diff-f3022b60ad71f5f773f5fa0290b2c787 seems to have introduced it in runtime.c? Why does this have to be an ELF section and not just e.g., a file int the AppDir?

There hadn't been gone too much thought into signature validation before changes were made in ... 2017, I think? This feature hasn't been used before AppImageUpdate has first implemented some features which make heavy use of signatures. There, a lot of problems were discovered. I think a lot of the discussion can be found in issues over at the AppImageUpdate repository. But let me provide a brief summary.

What we had embedded before this section was just a signature. To verify a signature, you need not only the signature, but also the (public) key. The signature doesn't contain information what key has been used. Even if it did, there had been a classic distribution problem: where to get the key from? You cannot assume every key is available from the usual keyservers, nor do you want to rely on the availability of these servers (read: shall also work offline).

What to do to fix the issue? Well, it's as simple as embedding the public portion of the key which signed an AppImage into the AppImage. That way, the public key is always available for signature verification.

I hope you now understand why making this a file in the AppDir is a pointless idea. That'd render the signature useless again, as you'd have to have a second file in order to be able to make any checks about it. This is not the goal of an AppImage.

What about .upd_info? The reason why the update information is in an ELF section is so that a server operator can change the update information without having to unpack and repack the whole AppImage. If this section is not skipped in the calculation of the hash, the the whole concept of using ELF sections is moot.

Dunno what validate.c does. It hasn't been maintained too well, and the code is in a bad state. If it is skipped in the signature calculation, you need to skip it. But it doesn't make too much sense to skip that section when calculating a signature. However, as long as an AppImage is signed and you use AppImageUpdate which performs a signature validation, manipulations to that string won't have any major consequences. An attacker might be able to trick you into updating from a different server than initially configured; however, as long as the attacker can't provide properly signed AppImages on that other location, an update will fail due to the lack of a signature (or a signature signed with a different key than the original AppImage).

Where has this stuff been discussed and documented?

It's been in for more than 2 years. Since most of it has been created for AppImageUpdate, you should check that issue tracker.

Why are you asking all these questions now? I'm kinda curious. Nobody's noticed these things haven't been documented properly in over two years after all.

probonopd commented 5 years ago

Well, it's as simple as embedding the public portion of the key which signed an AppImage into the AppImage. That way, the public key is always available for signature verification.

I hope you now understand why making this a file in the AppDir is a pointless idea. That'd render the signature useless again, as you'd have to have a second file in order to be able to make any checks about it. This is not the goal of an AppImage.

I don't understand this: When the public key would be inside the AppImage filesystem rather than in an ELF section, why couldn't the tool that verifies the signature extract it from there?

Dunno what validate.c does. It hasn't been maintained too well, and the code is in a bad state.

It worked and was feature complete, until the changes discussed above apparently broke it.

What about .upd_info?

Can we agree that this section may, and should, be skipped for the reasons you wrote above? If we don't skip that section, then it is not needed anymore - then we could put the updateinformation into the filesystem image because then it would also be impossible to change.

It was a design goal for the updateinformation to be changeable "after the fact".

It's been in for more than 2 years.

Doesn't matter. I didn't have time to look at that stuff or didn't notice that the file format was changed/amended in undocumented ways that are not in the spec.

Why are you asking all these questions now? I'm kinda curious. Nobody's noticed these things haven't been documented properly in over two years after all.

Lack of time.

I am doing an experimental implementation of the AppImage tools in Go and therefore I now am asking myself whether I have to implement this stuff that is not (yet?) specified. I need functionality like .digest_md5 but I would naturally lean to do it as .digest_md5 and skipping the .upd_info section, and not implementing the .sig_key section at all because that information can just as well be stored inside the AppImage.

TheAssassin commented 5 years ago

I don't understand this: When the public key would be inside the AppImage filesystem rather than in an ELF section, why couldn't the tool that verifies the signature extract it from there?

Well, that requires any tool that is supposed to validate an AppImage to implement the AppImage filesystem in order to be able to open it without running it itself. Which is a waste of time and code, and also less secure. There is no reason not to make this a section, but again there's many to do so (faster and easier to extract, way less complex, no need for any external libraries).

Can we agree that this section may, and should, be skipped for the reasons you wrote above? If we don't skip that section, then it is not needed anymore - then we could put the updateinformation into the filesystem image because then it would also be impossible to change.

There's no reason to not make it a section. Again, easier to extract, less complex, faster to extract. However, there's no real reason not to sign that information. For .digest_md5 there's also no reason to exclude the signatures section. I think it even doesn't exclude that, it only excludes itself. The signatures however have to exclude .digest_md5, as they have to be calculated at first. Once they're known however, that information won't change any more and can therefore be included in .digest_md5.

How is that impossible to be changed? You can alter an AppImage filesystem. You can even make a new one, and then concat it to the original executable header.

It's an order of calculation thing, where every already-known information has to be included to make sure it can't be tampered with. If your signature calculation is done on the entire AppImage (skipping only ELF sections that must be skipped because they're calculated later on), there's no difference.

Please don't mix things up.

A brief introduction in how and why to calculate information that is embedded into the AppImage executable header

Rule of thumb: When calculating a piece of information that is embedded into the AppImage, this calculation can and should involve anything that is already embedded, except for itself and what will be embedded later on.

  1. embed key
  2. calculate signature
    • --> should include key, that'll also make sure the key won't be tampered with
    • --> anything that will be embedded after calculating the signature must be excluded (which, for us, means, "assume it's zeroed out")
    • --> signature is known and can be embedded
  3. embed signature
  4. calculate MD5 hash for collision detection purposes
    • --> now, the signature is also known and already embedded, and therefore can and should be included in this calculation
    • --> only the own section where this digest will be embedded in can and should be excluded
  5. embed MD5 digest
probonopd commented 5 years ago

How is that impossible to be changed?

I need a way to change the contents of __.upd_info__ after signing has happened. So clearly, that section needs to be skipped in generating any digests (as verify.c does).

Use case: I have a server with 1,000 AppImage files. Now my domain name changes and I don't want to re-create all of the AppImages.

OK, now that I think about it, one could say "just change .upd_info and then sign the whole thing again". I suppose this would be possible without extracting and re-building the AppImage? Am I correct?

Well, that requires any tool that is supposed to validate an AppImage to implement the AppImage filesystem in order to be able to open it without running it itself.

The same argument could easily be made especially for at least .DirIcon, desktop file, AppStream metainfo. It would be much easier and faster for system integration daemons and thumbnailers to just read them from a section rather than to fetch them from zisofs (type-1) and squashfs (type-2).

  1. calculate signature
  2. embed signature
  3. calculate MD5 hash for collision detection purposes
  4. embed MD5 digest

For 2. we are already calculating a digest. It is what gets signed. Now, can't we use that signature as the hash for collision detection purposes? Then we would not need steps 4. and 5.

Ah right, what about unsigned AppImages, those don't have a signature that we could use for collission detection purposes. Solution is easy: Not only embed the Signature in the .sha256_sig section, but also the SHA256 digest that the signature signed. We can calculate that SHA256 digest also when the AppImage is not signed.

You may say that changing the mechanism now this breaks already-signed AppImages. Not necessarily, because AppImageUpdate could be changed with minor effort to take the possibility that there may (not must) now be a SHA256 digest in the .sha256_sig section into account. Wdyt?

probonopd commented 5 years ago

As it stands now, .sig_key has a (slim) chance to be considered in the AppImageSpec, but .digest_md5 has not. it only complicates things without clear technical necessity.

Rationale:

That non-spec conforming implementations have been out in the wild for so long is not valid rationale to put something into a spec.

probonopd commented 5 years ago

Fact: TheAssassin's implementation of the signature checking only zeros .sha256_sig, .sig_key.

According to https://travis-ci.org/AppImage/AppImageKit/jobs/616099634#L3374 the SHA256 digest is claimed to be: 26099f8178b1fbb13007e6022e3adf8a6f8c3b05736085c80adc31d2ba07a8f5

According to https://travis-ci.org/AppImage/AppImageKit/jobs/616099634#L3484 the download URL is https://artifacts.assassinate-you.net/artifactory/AppImageKit//travis-2111/appimagetool-x86_64.AppImage

So let's verify it:

wget https://artifacts.assassinate-you.net/artifactory/AppImageKit//travis-2111/appimagetool-x86_64.AppImage

# Make a copy
cp appimagetool-x86_64.AppImage appimagetool-x86_64.AppImage.zeros

# Zero .sha256_sig
dd conv=notrunc if=/dev/zero of=/home/me/Downloads/appimagetool-x86_64.AppImage.zeros bs=1 seek=176808 count=1024

# Zero .sig_key
dd conv=notrunc if=/dev/zero of=/home/me/Downloads/appimagetool-x86_64.AppImage.zeros bs=1 seek=177832 count=8192

sha256sum -b /home/me/Downloads/appimagetool-x86_64.AppImage.zeros