Fix Bug #949, "BAD signature" when validating #1090
Conversation
|
Thank you very much @yobson, highly valuable. |
TheAssassin
left a comment
TheAssassin
left a comment
There was a problem hiding this comment.
Thanks for this PR. Indeed it fixes the problem with current runtimes.
However, I'm not so sure we should merge it at this point. It lacks support for older runtimes, and makes assumptions about the ELF file layout.
I could live with merging this, though, as this tool should be rewritten in a less annoying programming language at some point anyway.
| fprintf(stderr, "Failed to read .sha256_sig section"); | ||
| exit(1); | ||
| } | ||
| if (!appimage_get_elf_section_offset_and_length(filename, ".sig_key", &skip_offset_key, &skip_length_key)) { |
There was a problem hiding this comment.
Older AppImages simply don't have this section at all. When AppImageUpdate gained some actual signature verification support, there was a need for the pubkey to be distributed along with the AppImages. Before, that wasn't needed.
I think if we fix this, there should be a fallback for these older runtimes. I could however also live with this tool not supporting old AppImages any more. (The old code can always be restored from the repo, after all.)
@probonopd what do you think?
|
It would be really easy to get it working for older runtimes as well, I'd just need to check for the existence of the new elf section. I'll do that when I'm back in work on Monday. So at least don't merge until then. If you like, I can try a rewrite in another language. It isn't a complicated tool to write. But I'd need three things from you:
I'd do that as another merge request. |
|
Standard C++11 might work as a language, we already use that elsewhere. But we can also keep C. The masterplan is to rewrite these things in Rust at some point, but that only makes sense if you start over the entire project. |
|
Please let's keep this repository, especially I am starting to wonder whether we should write it in Go... |
|
I'll update this repo in c. No problem. Rust/go/c++ for the more general case rewrite of a few of these tools. Jury still out. Where shall I write this code if not in this repo? |
|
@yobson I think regarding this PR nothing needs to be rewirtten as the |
|
@TheAssassin pointed out that this pull request would break support for older appimages. I'm going to make a change so that it supports new and and old appimages (very quick fix) But the tool makes some assumptions about about the structure of the app image which I think @TheAssassin doesn't think is a good idea to make, so I'm happy to rewrite it in another language so that it doesn't break down the line either. |
|
Don't merge what I just pushed. I did it quickly on a mac. I'll test it when I'm in the office with my linux machine. |
|
@TheAssassin, It should now be backwards compatible. |
|
Is there anything else I need to do? I'm new to open source workflows |
|
@TheAssassin would you mind reviewing this PR again? We're still hoping to be able to sign AppImages but we want to make sure the signature is correct |
|
any chance this could get reviewed again? |
TheAssassin
left a comment
TheAssassin
left a comment
There was a problem hiding this comment.
Code looks good, haven't tested it, though. @Apfelwurm you may want to provide feedback on this.
|
Nice, that was fast :) Thank you! I have quickly tested it with https://github.com/Apfelwurm/appimagevalidate and when i checked out the current master here the error was still reproducable. When i use yobsons current master everything works like i would expect it. |
|
Lovely to hear! |
I never saw that notification that an update had been pushed. I just saw that yesterday. @yobson thanks for your contribution. |
5 participants
This was to fix this bug.
Changes made:
.sig_keypart of ELFCurrently, it works on the assumption that these parts are adjacent in the ELF.