Fix sinfoCmp to order signatures correctly #3194
Conversation
This requires adjusting a number of test that reflect the ordering. The changes in tests/rpmsigdig.at look straight forward and correct - just changing the order in which the signatures and checksums are presented. The changes in tests/rpmi.at seem to drop the relevant information. This might be accidental as the code just returns the first issue found. But "no signature" seems kinda weird result when before it complaint about a specific signature. The next patch tries to fix that. Resolves: rpm-software-management#3185
|
Ok, now the error reporting is better. It probably still should use the most secure hash / signature that is broken but at least it will no longer report "no signature" |
lib/transaction.c
Outdated
| vd->type[sinfo->type] = sinfo->rc; | ||
| for (int type=0; type < 3; type++) { | ||
| if (type != sinfo->type && vd->type[type] >= sinfo->rc) | ||
| return 1; |
There was a problem hiding this comment.
We will still fall through (to the switch below and thus potentially replace the error message) if the current error in sinfo->rc is the same or lower than what we have stored for this type so far.
lib/transaction.c
Outdated
| vd->type[sinfo->type] = res; | ||
| if (sinfo->rc > vd->type[sinfo->type]) { | ||
| vd->type[sinfo->type] = sinfo->rc; | ||
| for (int type=0; type < 3; type++) { |
There was a problem hiding this comment.
Maybe just use sizeof(vd->type) here?
Don't rely on the fist issue found being the most meaningful. Always return 1 to loop through all signatures / hashes. Use the first error of the highest severity. Using the severity in vd->type[] is a bit of a hack but OK as it is only checked for == RPMRC_OK (aka 0) in verifyPackageFiles. Related: rpm-software-management#3185
|
OK, this was a bit more involved than the 2 line fix this originally looked like. I kept the "Legacy compat" behavior in |
| if (res > vd->type[sinfo->type]) | ||
| vd->type[sinfo->type] = res; | ||
| if (severity > vd->type[sinfo->type]) { | ||
| vd->type[sinfo->type] = severity; |
There was a problem hiding this comment.
Don't. Don't abuse a completely unrelated field when there's no reason to do so, just because you think you might get away with it. It only achieves making fairly complicated logic even more confusing and that's not what you want in security sensitive code. This is an internal struct, if you need a severity field then add it. And since it can then be used to aid sorting these items, maybe it'll all fall into place more naturally that way. For one, using a nicer data structure (now that we have plenty available) might make it all much saner.
This verification callback fubar was written under RHEL deadline pressure six years ago and isn't exactly one of my pridest moments...
There was a problem hiding this comment.
Ehm, okay so vd->type is something entirely different (a field badly named by me), I first thought this was abusing the data contained in sinfo->type. This is more harmless but also there's nothing saved by avoiding an extra field for the purpose, this code is bad enough as it is 😆
|
I wonder if there isn't a saner way to handle the legacy compat thing of allowing unsigned packages. |
|
Other remarks found from the dusty archives while poking around this area in my head: IIRC the reason the transaction verify reports the first issue found is that in that context, there's no point continuing. And so, you can just abort on the first error found. Or so the theory goes, clearly the existing code gets it wrong in the case of NOTFOUND encountered first which is more likely to happen with the sinfo sorting fixed. Makes me think the NOTFOUND handling is basically in the wrong place, the verify callback is not interested whether the signature is RSA or DSA or whatever, it just wants to know if there was a signature that we could verify. Which I think should really be hidden inside rpmvs. There's a Finnish idiom "juosten kustu", basically "peed while running" that seems appropriate for the existing signature callback mess - it's just the kind of thing you get when you're think you're in hurry and don't stop to think properly 😄 I do quite vividly remember thinking this is klunky as hell but it'll have to do for now... |
|
So ... maybe we're better off looking at this in the context of other signature changes coming for v6, and concentrate on those 4.20 matters just now instead. |
|
Yet another thought: maybe what the existing code unintentionally does is not so bad: it basically fades out the phony "X is better than Y" comparison between RSA and (EC)DSA, which is particularly silly when only type will ever be present in a single package. Ie you just fix the hashalgo comparison and drop the sigalgo comparison entirely (although it'd warrant a comment), the test-suite still passes with zero modifications... |
|
Closing this here and dropping the ticket back into the ToDo pile. This needs a fresh start and not use up more time befort the 4.20 release. |
This requires adjusting a number of test that reflect the ordering. The changes in tests/rpmsigdig.at look straight forward and correct - just changing the order in which the signatures and checksums are presented.
The changes in tests/rpmi.at seem to drop the relevant information. This might be accidental as the code just returns the first issue found. But "no signature" seems kinda weird result when before it complaint about a specific signature. May be someone with more clue about this should ahve a second look before merging.
Resolves: #3185,#1057