Fix IMA signature lengths assumed constant (#1833, RhBug:2018937) #1844

pmatilai · 2021-11-26T10:39:54Z

ECDSA signatures can vary in length, but the IMA code assumes constant
lengths and thus may either place invalid signatures on disk from
either truncating or overshooting, and segfault if the stars are just
so.

Luckily the signatures are stored as strings so we can calculate the
actual lengths at runtime and ignore the stored constant length info.
Extend hex2bin() to optionally calculate the lengths and maximum,
and use these for returning IMA data from the rpmfi(les) API.

Additionally update the signing code to store the largest IMA signature
length rather than what happened to be last to be on the safe side.
We can't rely on this value due to invalid packages being out there,
but then we need to calculate the lengths on rpmfiles populate so there's
not a lot to gain anyhow.

Fixes: #1833

nullr0ute · 2021-11-26T14:58:44Z

Should there be tests for this too?

pmatilai · 2021-11-29T08:50:17Z

In a better world, yes of course. It's just that we don't have any good means to test it, because our test-suite expects to run non-root so applying IMA signatures is a non-starter, and plugins are disabled in the test-suite due to other artifacts. Even testing signing is pending for somebody to sit down and figure how to do it in the environment. One could test the API returns of course, but this stuff isn't exposed to Python bindings, sigh.

mlschroe · 2021-11-29T10:43:19Z

RSA sigs can also vary in length. Leading zeros of the exponentiation result will be stripped, so there's a 1/256 chance that the sig is smaller than expected.

pmatilai · 2021-11-29T11:31:52Z

Oh, right. Remember running into those "missing zeros" on occasion...

pmatilai · 2021-11-29T13:01:03Z

Rebased on top of #1847 for access to the data from Python, test added and commit message adjusted to list RSA as variable length too.

Conan-Kudo · 2021-12-02T14:24:49Z

@pmatilai This PR is behaving oddly, it shows me the Python data access commit even though that's already present in git master. Perhaps you need to rebase again?

dmnks

LGTM overall but see inline comment.

dmnks · 2021-12-06T10:33:06Z

lib/rpmfi.c

@@ -579,9 +580,9 @@ const unsigned char * rpmfilesFSignature(rpmfiles fi, int ix, size_t *len)

    if (fi != NULL && ix >= 0 && ix < rpmfilesFC(fi)) {
 	if (fi->signatures != NULL)
-	    signature = fi->signatures + (fi->signaturelength * ix);
+	    signature = fi->signatures + (fi->signaturemaxlen * ix);


Is this actually correct, given that the lengths can now vary?

Is there a proof that this cannot overflow?

I guess this should have just been signaturelengths[ix] but somehow got missed. Or I'm missing something (non)obvious here...

No, this is an integer overflow with potentially bad consequences.

rpmfi.c is likely one big integer overflow if you go looking, the overall assumption in the codebase (in this file and basically everywhere) is that they don't overflow. Which is why I'm not going to bother with this one either.

That said a limit on signature lengths would be another approach.

The header is size-capped to 256MB, there's a limit for you.

...and here's another: #1858

Is this actually correct, given that the lengths can now vary?

So @dmnks actually did point out the flaw in this patch but it got lost in all the noise.

...except that this isn't the flaw, only pointing to the general direction. The real flaw is in the part of hex2bin() that's not touched in this patch so it's hard to spot.

dmnks · 2021-12-06T10:36:04Z

lib/rpmfi.c

@@ -116,7 +116,8 @@ struct rpmfiles_s {
    struct fingerPrint_s * fps;	/*!< File fingerprint(s). */

    int digestalgo;		/*!< File digest algorithm */
-    int signaturelength;	/*!< File signature length */
+    int *signaturelengths;	/*!< File signature length */


Cosmetic but the comment should be pluralized too 😄

DemiMarie

There are a few areas where I would like to see a proof of no integer overflow.

DemiMarie · 2021-12-06T13:41:34Z

lib/rpmfi.c

@@ -579,9 +580,9 @@ const unsigned char * rpmfilesFSignature(rpmfiles fi, int ix, size_t *len)

    if (fi != NULL && ix >= 0 && ix < rpmfilesFC(fi)) {
 	if (fi->signatures != NULL)
-	    signature = fi->signatures + (fi->signaturelength * ix);
+	    signature = fi->signatures + (fi->signaturemaxlen * ix);


Is there a proof that this cannot overflow?

DemiMarie · 2021-12-06T13:43:08Z

lib/rpmfi.c

+	/* Figure string sizes + max length for allocation purposes */
+	if (lengths) {
+	    int maxl = 0;
+	    int *lens = xmalloc(num * sizeof(*lens));


Suggested change

int *lens = xmalloc(num * sizeof(*lens));

int *lens = xcalloc(num, sizeof(*lens));

This is safer in the event of an overflow.

DemiMarie · 2021-12-06T13:43:38Z

lib/rpmfi.c

+	    int i = 0;
+
+	    while ((s = rpmtdNextString(&td))) {
+		lens[i] = strlen(s) / 2;


What if strlen(s) is odd? That should be an error.

I guess the assumption here is that a hex string will always be even (one byte is represented by two chars), but otherwise yeah, we should probably have a check here as we can't fully control the input.

Right, these tags should be sanity checked along everything else in the signature header, but that doesn't currently happen for data that is not used for content checking. The same goes for fsverity signatures. Not that it matters a whole lot, the signature will be invalid in that case anyhow.

, RhBug:2018937) At least ECDSA and RSA signatures can vary in length, but the IMA code assumes constant lengths and thus may either place invalid signatures on disk from either truncating or overshooting, and segfault if the stars are just so. Luckily the signatures are stored as strings so we can calculate the actual lengths at runtime and ignore the stored constant length info. Extend hex2bin() to optionally calculate the lengths and maximum, and use these for returning IMA data from the rpmfi(les) API. Additionally update the signing code to store the largest IMA signature length rather than what happened to be last to be on the safe side. We can't rely on this value due to invalid packages being out there, but then we need to calculate the lengths on rpmfiles populate so there's not a lot to gain anyhow. Fixes: rpm-software-management#1833

pmatilai · 2021-12-13T11:16:30Z

FWIW, there appear to be other flaws in the patch, as in segfaults in some circumstances. Will fix of course once I manage to reproduce, but just goes to show that reviews should be concentrating on the actual change. Not theory.

pmatilai · 2021-12-13T14:34:45Z

Yep, this PR was utter garbage. Anybody needing to cherry-pick the IMA fix, be sure to include commit 31e9daf too.

pmatilai added the bug label Nov 26, 2021

pmatilai mentioned this pull request Nov 29, 2021

Add Python bindings for rpmfilesFSignature() and rpmfilesVSignature() #1847

Merged

pmatilai force-pushed the imasiglen-pr branch 2 times, most recently from ee305ab to 8fa91f3 Compare November 29, 2021 12:59

pmatilai force-pushed the imasiglen-pr branch from 8fa91f3 to feb9b52 Compare December 2, 2021 14:35

dmnks reviewed Dec 6, 2021

View reviewed changes

DemiMarie reviewed Dec 6, 2021

View reviewed changes

pmatilai force-pushed the imasiglen-pr branch from feb9b52 to 1699a8b Compare December 13, 2021 08:09

pmatilai merged commit a79d7ae into rpm-software-management:master Dec 13, 2021

pmatilai deleted the imasiglen-pr branch February 2, 2022 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IMA signature lengths assumed constant (#1833, RhBug:2018937) #1844

Fix IMA signature lengths assumed constant (#1833, RhBug:2018937) #1844

pmatilai commented Nov 26, 2021

nullr0ute commented Nov 26, 2021

pmatilai commented Nov 29, 2021

mlschroe commented Nov 29, 2021

pmatilai commented Nov 29, 2021

pmatilai commented Nov 29, 2021

Conan-Kudo commented Dec 2, 2021

dmnks left a comment

dmnks Dec 6, 2021

DemiMarie Dec 6, 2021

dmnks Dec 6, 2021

DemiMarie Dec 6, 2021

pmatilai Dec 7, 2021 •

edited

DemiMarie Dec 7, 2021

pmatilai Dec 13, 2021

pmatilai Dec 13, 2021

pmatilai Dec 13, 2021

pmatilai Dec 13, 2021

dmnks Dec 6, 2021

DemiMarie left a comment

DemiMarie Dec 6, 2021

DemiMarie Dec 6, 2021

DemiMarie Dec 6, 2021

dmnks Dec 6, 2021 •

edited

pmatilai Dec 13, 2021

pmatilai commented Dec 13, 2021

pmatilai commented Dec 13, 2021

	int lens = xmalloc(num sizeof(*lens));
	int lens = xcalloc(num, sizeof(lens));

Navigation Menu

Fix IMA signature lengths assumed constant (#1833, RhBug:2018937) #1844

Fix IMA signature lengths assumed constant (#1833, RhBug:2018937) #1844

Conversation

pmatilai commented Nov 26, 2021

nullr0ute commented Nov 26, 2021

pmatilai commented Nov 29, 2021

mlschroe commented Nov 29, 2021

pmatilai commented Nov 29, 2021

pmatilai commented Nov 29, 2021

Conan-Kudo commented Dec 2, 2021

dmnks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai Dec 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmnks Dec 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai commented Dec 13, 2021

pmatilai commented Dec 13, 2021

pmatilai Dec 7, 2021 •

edited

dmnks Dec 6, 2021 •

edited