Bug 28204 - extend webapi / verification with forthcoming signed-contents archives
Summary: extend webapi / verification with forthcoming signed-contents archives
Status: ASSIGNED
Alias: None
Product: elfutils
Classification: Unclassified
Component: debuginfod (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Ryan Goldberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-06 21:11 UTC by Frank Ch. Eigler
Modified: 2023-08-29 13:09 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Submit A Patch for 28204 (16.66 KB, patch)
2022-07-13 19:44 UTC, Ryan Goldberg
Details | Diff
Submit A Patch for 28204, corrections made (17.06 KB, patch)
2022-08-04 19:45 UTC, Ryan Goldberg
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Ch. Eigler 2021-08-06 21:11:34 UTC
Efforts such as https://fedoraproject.org/wiki/Changes/Signed_RPM_Contents might look stalled, but some distro builds are experimenting with this stuff already.   We may soon avail ourselves of RPMs that carry per-file IMA signatures.  If so, we should extract those signature bits and pass them back to debuginfod clients.  They may be able to offline verify the integrity of the download, so as to not trust the debuginfod server.
Comment 1 Frank Ch. Eigler 2021-08-12 18:13:27 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=1896046#c10 (private, sorry) contains instructions on generating IMA-signed RPMs via

% rpm --addsign --signfiles --fskpath=FOO.pem foo.rpm
Comment 2 Mark Wielaard 2021-08-26 20:53:49 UTC
OpenSuse has some documentation IMA here:
https://en.opensuse.org/SDB:Ima_evm#Introduction

Gentoo:
https://wiki.gentoo.org/wiki/Integrity_Measurement_Architecture

How would this be used together with debuginfod? Where/how would the user get the signatures and how would the client library check those signatures?
Comment 3 Frank Ch. Eigler 2021-09-08 11:26:28 UTC
> How would this be used together with debuginfod?
> Where/how would the user get the signatures

From debuginfod, possibly via additional response headers,
when extracting files from IMA-signed archives.

> and how would the client library check those signatures?

If it has the file and the signature (and the appropriate
public key!) available, the debuginfod client can perform
this integrity check at download time.
Comment 4 Frank Ch. Eigler 2022-06-10 17:19:37 UTC
some more references

https://fedoraproject.org/wiki/Changes/Signed_RPM_Contents,              

https://man.archlinux.org/man/rpmsign.8.en                                                

rpmfiFSignature(), lib/rpmfi.c
Comment 5 Ryan Goldberg 2022-07-13 19:44:08 UTC
Created attachment 14208 [details]
Submit A Patch for 28204

At the moment there are no public x509 verification certificates available to validate the signatures generated for RHEL 9. There is discussion at https://issues.redhat.com/browse/SIGNSERVER-245 for the future location and in fedora 37 the cert will be available on the fedora security site, https://getfedora.org/security/
Comment 6 Mark Wielaard 2022-07-27 14:52:02 UTC
Still reviewing patch, but we seem to also need a configure check to make sure we have a new enough rpm to recognize the rhel9 rpm, otherwise some tests will fail with:

+ rpm2cpio ../../R/debuginfod-rpms/rhel9/hello2-1.0-1.x86_64.rpm
+ cpio -ivd
argument is not an RPM package
cpio: premature end of archive
Comment 7 Ryan Goldberg 2022-08-04 19:45:08 UTC
Created attachment 14256 [details]
Submit A Patch for 28204, corrections made

The following patch fixes the previously mentioned issues. It passes all the tests across the try buildbots (users/rgoldber/try-bz28204)
Comment 8 Mark Wielaard 2022-10-20 21:10:10 UTC
Note that we'll need rpm-devel ima-evm-utils-devel openssl-devel rpm-sign to test this (should go into elfutils.spec BuildRequires).
Comment 9 Mark Wielaard 2022-10-20 23:11:59 UTC
To make this slightly easier to review I applied the patch and rebased it to current master. Pushed here:
https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=ima
Comment 10 Ryan Goldberg 2023-07-06 19:49:13 UTC
Hi, it has been quite the journey, but the latest draft of this patch is ready for review

It is sitting on the try-branch users/rgoldber/try-bz28204c (figure'd it was big enough that just looking at a patch might be hard to follow)

Since the last review the major changes are as follows
* I added a --koji-sigcache flag to the server which will enable koji specific mappings of rpm paths to get IMA signatures. 
* DEBUGINFOD_IMA_CERT_PATH can now include paths to dirs containing PEM and DER encoded certificates. And will be traversed looking for the first cert which has a skid matching the signature which we need to validate.
* The verification certificates for RHEL and CentOS have been finalized and we have a green light to distribute copies of them alongside our source (since they have not been formally published to a known location yet). They are in debuginfod/ima-certs and will be installed to $(sysconfdir)/debuginfod/ima-certs. DEBUGINFOD_IMA_CERT_PATH will by default include this path. This dir also has copies of the current fedora verification certs (which are already public but not yet backported to f38 [fedora-repos commit 93b2c8a])
Comment 11 Mark Wielaard 2023-07-23 21:05:51 UTC
So as far as I understand this is now the three commits on top of users/rgoldber/try-bz28204c: https://sourceware.org/cgit/elfutils/log/?h=users/rgoldber/try-bz28204c

I find it slightly easier when patches are simply posted to the mailinglist (using git send-email).
Comment 12 Mark Wielaard 2023-07-23 21:07:27 UTC
In config/profile.csh.in and config/profile.sh.in the prefix variable is explicitly set and no longer unset. Is that deliberate?

In debuginfod_validate_imasig the file_data = malloc(data_len); depends on the (externally) given file size. It is then read in one pread call. And the whole buffer is then given to EVP_DigestUpdate. Note that this might create a giant malloc buffer, which might trigger OOM. pread might succeed with fewer bytes than given. It needs to be called in a loop. But it would be better if we could read it and feed it to EVP_DigestUpdate in (small) chunks.

Is EACCESS the right error code to return when the signature couldn't be checked/is invalid? That is the same as when we get CURLE_REMOTE_ACCESS_DENIED. It might be good if it was an unique error code so users can know that the file was not trusted.
Comment 13 Mark Wielaard 2023-07-23 21:08:18 UTC
The configure checks might need to check whether the rpm development headers define the needed constants.

On an rhel8 system this gives:

checking for headerGet in -lrpm... yes
checking for imaevm_hash_algo_from_sig in -limaevm... yes
checking for EVP_MD_CTX_new in -lcrypto... yes

but...

Making all in debuginfod
  CXX      debuginfod.o
elfutils/debuginfod/debuginfod.cxx: In function ‘MHD_Response* handle_buildid_r_match(bool, int64_t, const string&, const string&, const string&, int*)’:
elfutils/debuginfod/debuginfod.cxx:2026:47: error: ‘RPMSIGTAG_FILESIGNATURES’ was not declared in this scope
       if (!sig_hdr || 1 != headerGet(sig_hdr, RPMSIGTAG_FILESIGNATURES, &sig_tag_data, HEADERGET_ALLOC))
                                               ^~~~~~~~~~~~~~~~~~~~~~~~
elfutils/debuginfod/debuginfod.cxx:2026:47: note: suggested alternative: ‘RPMTAG_FILESIGNATURES’
       if (!sig_hdr || 1 != headerGet(sig_hdr, RPMSIGTAG_FILESIGNATURES, &sig_tag_data, HEADERGET_ALLOC))
                                               ^~~~~~~~~~~~~~~~~~~~~~~~
                                               RPMTAG_FILESIGNATURES
make[2]: *** [Makefile:657: debuginfod.o] Error 1
Comment 14 Mark Wielaard 2023-07-23 21:57:42 UTC
I think it is the user/distro packager who should decide which ima-certs to ship. I don't think elfutils should come with ima-certs itself.

Why is there a "permissive" policy? What is the use case for this?

Should the policy be per debuginfod url? So you can point to an official distro debuginfod which must be in enforcing mode, but can add a local one with an "ignore" policy.
Comment 15 Mark Wielaard 2023-07-23 22:23:44 UTC
The basic idea having a collection of certs, and an signature for each file that is transported with the fetch operation that will be checked against those trusted certs is understandable.

But I must admit that I am a little lost in the rpm and koji mechanisms to extract those signatures. How easy will it be to extend to other platforms that might store such signatures in different ways?
Comment 16 Ryan Goldberg 2023-07-31 14:27:18 UTC
(In reply to Mark Wielaard from comment #12)
> In config/profile.csh.in and config/profile.sh.in the prefix variable is
> explicitly set and no longer unset. Is that deliberate?
Taking a look at both files they seem to still contain `unset prefix` as their last lines. Would that not do the trick?

> In debuginfod_validate_imasig the file_data = malloc(data_len); depends on
> the (externally) given file size. It is then read in one pread call. And the
> whole buffer is then given to EVP_DigestUpdate. Note that this might create
> a giant malloc buffer, which might trigger OOM. pread might succeed with
> fewer bytes than given. It needs to be called in a loop. But it would be
> better if we could read it and feed it to EVP_DigestUpdate in (small) chunks.
Sure, just looked at the docs for EVP_DigestUpdate and seems like this change should be pretty straightforward.

> Is EACCESS the right error code to return when the signature couldn't be
> checked/is invalid? That is the same as when we get
> CURLE_REMOTE_ACCESS_DENIED. It might be good if it was an unique error code
> so users can know that the file was not trusted.
How do you feel about EPERM? Not exactly its usage but we've used up a good number of the more applicable ones with the CURL errors (EACCESS, EINVAL)

(In reply to Mark Wielaard from comment #13)
> The configure checks might need to check whether the rpm development headers
> define the needed constants.
Sure

(In reply to Mark Wielaard from comment #14)
> I think it is the user/distro packager who should decide which ima-certs to
> ship. I don't think elfutils should come with ima-certs itself.
From what I've seen finding the correct certificates for IMA verification has been pretty tricky, so distributing a copy of these (at least early certificates) might be a good idea. Eventually the intention is for these certs to be shipped in a known location which we can easily add to the search path, but until then having a copy seems like the best bet imho.

> Why is there a "permissive" policy? What is the use case for this?
It might be that a user wants to be aware of what's going on but does not need to be quite so strict on rejection. Permissive won't allow incorrect signatures but will allow say an unsigned file whereas an enforcing client will reject anything that does not come with a valid signature

> Should the policy be per debuginfod url? So you can point to an official
> distro debuginfod which must be in enforcing mode, but can add a local one
> with an "ignore" policy.
I was thinking of keeping things simple, but don't have anything against moving to a per URL sort of approach. What kind of format would you want for such a thing? For urls foo:bar:baz would we want something like ignore::enforcing where blanks are the default? Don't love this structure since it seems a little unwieldy. I'll think on a better format for it.

(In reply to Mark Wielaard from comment #15)
> But I must admit that I am a little lost in the rpm and koji mechanisms to
> extract those signatures. How easy will it be to extend to other platforms
> that might store such signatures in different ways?
The koji flag seems like a necessary evil in my opinion. At least the way it works is that when the flag is enabled we try the koji storage method and then fall back to the rpm itself, so to add another signature storage format we could do the same method, so adding more shouldn't be too difficult. Might require a little logic tweak but wouldn't think its too complex a change
Comment 17 Mark Wielaard 2023-08-02 16:37:51 UTC
(In reply to Ryan Goldberg from comment #16)
> (In reply to Mark Wielaard from comment #12)
> > In config/profile.csh.in and config/profile.sh.in the prefix variable is
> > explicitly set and no longer unset. Is that deliberate?
> Taking a look at both files they seem to still contain `unset prefix` as
> their last lines. Would that not do the trick?

Yes it would. I was just confused why it was needed.
But I now see that @sysconfdir@ is used and that uses $prefix.
So that explains why you need it defined.

> > In debuginfod_validate_imasig the file_data = malloc(data_len); depends on
> > the (externally) given file size. It is then read in one pread call. And the
> > whole buffer is then given to EVP_DigestUpdate. Note that this might create
> > a giant malloc buffer, which might trigger OOM. pread might succeed with
> > fewer bytes than given. It needs to be called in a loop. But it would be
> > better if we could read it and feed it to EVP_DigestUpdate in (small) chunks.
> Sure, just looked at the docs for EVP_DigestUpdate and seems like this
> change should be pretty straightforward.
> 
> > Is EACCESS the right error code to return when the signature couldn't be
> > checked/is invalid? That is the same as when we get
> > CURLE_REMOTE_ACCESS_DENIED. It might be good if it was an unique error code
> > so users can know that the file was not trusted.
> How do you feel about EPERM? Not exactly its usage but we've used up a good
> number of the more applicable ones with the CURL errors (EACCESS, EINVAL)

Yes, EPERM sounds like a good pick. It is unlikely to be produced by any other operation (unless there are file seals in play).

> (In reply to Mark Wielaard from comment #14)
> > I think it is the user/distro packager who should decide which ima-certs to
> > ship. I don't think elfutils should come with ima-certs itself.
> From what I've seen finding the correct certificates for IMA verification
> has been pretty tricky, so distributing a copy of these (at least early
> certificates) might be a good idea. Eventually the intention is for these
> certs to be shipped in a known location which we can easily add to the
> search path, but until then having a copy seems like the best bet imho.
> 
> > Why is there a "permissive" policy? What is the use case for this?
> It might be that a user wants to be aware of what's going on but does not
> need to be quite so strict on rejection. Permissive won't allow incorrect
> signatures but will allow say an unsigned file whereas an enforcing client
> will reject anything that does not come with a valid signature

Doesn't that give a false sense of "security"?
It still rejects some stuff, but doesn't really protect against "falsifying" files, all a server has to do is not provide an IMA 

If it is just to see what would happen if enabling ima file checking, then it probably shouldn't reject anything. In that case it should warn for both missing and invalid signatures, but still accept them.

But in general I like less "modes".
 
> > Should the policy be per debuginfod url? So you can point to an official
> > distro debuginfod which must be in enforcing mode, but can add a local one
> > with an "ignore" policy.
> I was thinking of keeping things simple, but don't have anything against
> moving to a per URL sort of approach. What kind of format would you want for
> such a thing? For urls foo:bar:baz would we want something like
> ignore::enforcing where blanks are the default? Don't love this structure
> since it seems a little unwieldy. I'll think on a better format for it.

Maybe prefixing or postfixing URLS with + or adding the name of the cert?
DEBUGINFOD_URLS="https://debuginfod.fedoraproject.org/+FEDORACERTNAME" ?

Yeah, gets ugly quick :{
But I think a common use case will be having the main distro debuginfod server in enforcing mode and your local/org debuginfod server in trusting mode.
Comment 18 Frank Ch. Eigler 2023-08-07 17:04:55 UTC
> Doesn't that give a false sense of "security"?
> It still rejects some stuff, but doesn't really protect against "falsifying"
> files, all a server has to do is not provide an IMA 

Yes, but trusted servers won't just do that.

> If it is just to see what would happen if enabling ima file checking, then
> it probably shouldn't reject anything. In that case it should warn for both
> missing and invalid signatures, but still accept them.

The difference between missing and invalid is that the latter is KNOWN bad.
An invalid signature is evidence that the file has a problem.
Comment 19 Ryan Goldberg 2023-08-09 18:17:36 UTC
(In reply to Mark Wielaard from comment #17)
> Maybe prefixing or postfixing URLS with + or adding the name of the cert?
I'm leaning towards a combination of this idea with my original. We can use DEBUGINFOD_IMA_POLICY to set the default ima verification response policy (possibly renaming to DEBUGINFOD_IMA_DEFAULT_POLICY) and then we can use DEBUGINFOD_URLS="url1+enforcing url2 url3+ignore". This seems like a simple way the user can define policy at a granularity of their choice. I wrote up a quick test patch for this and it seems pretty straightforward.

(In reply to Frank Ch. Eigler from comment #18)
> > Doesn't that give a false sense of "security"?
> > It still rejects some stuff, but doesn't really protect against "falsifying"
> > files, all a server has to do is not provide an IMA 
> 
> Yes, but trusted servers won't just do that.
It's up to the user to choose when to allow a permissive policy. Maybe not using it as the default will alleviate these concerns? A user would have to explicitly choose to 'let their guard down' when for instance using a trusted server

> The difference between missing and invalid is that the latter is KNOWN bad.
> An invalid signature is evidence that the file has a problem.
I agree that the distinction is enough that having a 3rd mode seems like a good bet. When using https://debuginfod.fedoraproject.org/ it doesn't feel like an issue to me to use a file that doesn't happen to have a signature, but otoh I wouldn't want one that I know is incorrect
Comment 20 Mark Wielaard 2023-08-17 15:39:25 UTC
(In reply to Frank Ch. Eigler from comment #18)
> > Doesn't that give a false sense of "security"?
> > It still rejects some stuff, but doesn't really protect against "falsifying"
> > files, all a server has to do is not provide an IMA 
> 
> Yes, but trusted servers won't just do that.

But isn't the idea of checking the IMA signatures that you don't have to trust the server providing the debuginfo files as the distro intended them?

> > If it is just to see what would happen if enabling ima file checking, then
> > it probably shouldn't reject anything. In that case it should warn for both
> > missing and invalid signatures, but still accept them.
> 
> The difference between missing and invalid is that the latter is KNOWN bad.
> An invalid signature is evidence that the file has a problem.

And a missing signature is UNKNOWN bad?

So both are bad in some way. Which imho means that if we support some kind of permissive mode, then it should explicitly warn for both kind of baddness.
Comment 21 Ryan Goldberg 2023-08-17 16:25:01 UTC
(In reply to Mark Wielaard from comment #20)
> But isn't the idea of checking the IMA signatures that you don't have to
> trust the server providing the debuginfo files as the distro intended them?
But this will allow for the case of a trusted server which only has some of it's RPMs per-file signed. Take for instance a server which has the RPMs for f36,37,38. The f36 files don't have signatures so using enforcing here is too strict since we are ok just letting a client know that these ones are unverifiable, but we still want to be able to reject any of the invalid ones for f38

> So both are bad in some way. Which imho means that if we support some kind
> of permissive mode, then it should explicitly warn for both kind of baddness.
In the permissive mode you'll get:
* "the signature is valid" for valid sigs
* "ALERT: this download is being rejected since the IMA signature could not be verified" for invalid sigs
* "the signature could not be verified" otherwise

So we do warn for both kinds of bad, we just don't reject the 'unknown' bad
Comment 22 Mark Wielaard 2023-08-25 15:00:13 UTC
(In reply to Ryan Goldberg from comment #21)
> (In reply to Mark Wielaard from comment #20)
> > But isn't the idea of checking the IMA signatures that you don't have to
> > trust the server providing the debuginfo files as the distro intended them?
> But this will allow for the case of a trusted server which only has some of
> it's RPMs per-file signed. Take for instance a server which has the RPMs for
> f36,37,38. The f36 files don't have signatures so using enforcing here is
> too strict since we are ok just letting a client know that these ones are
> unverifiable, but we still want to be able to reject any of the invalid ones
> for f38

This still feels odd. Since you cannot distinguish between non-sig f36 package (okish?) and non-sig f38 packages (bad?). I think you have to either trust or reject all non-sig packages from such a server.

> > So both are bad in some way. Which imho means that if we support some kind
> > of permissive mode, then it should explicitly warn for both kind of baddness.
> In the permissive mode you'll get:
> * "the signature is valid" for valid sigs
> * "ALERT: this download is being rejected since the IMA signature could not
> be verified" for invalid sigs
> * "the signature could not be verified" otherwise

I'll look at the code to see if I understand what this means in practice. Are those the messages presented to the user (in verbose mode? always?). And does this mean all three cases warn (or only the second and third)? And is it just a message or does it also mean actual rejection in some cases?

> So we do warn for both kinds of bad, we just don't reject the 'unknown' bad

But how is 'unknown' bad different from invalid signature bad?
I think you should assume that if there is no signature, then the signature is by definition invalid (assume it has a signature of 00000000000000000000000000000000).
Comment 23 Ryan Goldberg 2023-08-25 15:21:02 UTC
(In reply to Mark Wielaard from comment #22)
> This still feels odd. Since you cannot distinguish between non-sig f36
> package (okish?) and non-sig f38 packages (bad?). I think you have to either
> trust or reject all non-sig packages from such a server.
I see where you're coming from but what policy would you use for https://debuginfod.fedoraproject.org? We'd have to ignore then, which seems like one of the primary users of such verification might just skip it all together.

> I'll look at the code to see if I understand what this means in practice.
> Are those the messages presented to the user (in verbose mode? always?). And
> does this mean all three cases warn (or only the second and third)? And is
> it just a message or does it also mean actual rejection in some cases?
2008   if(NULL != url_ima_policies && ignore != url_ima_policies[committed_to])
2009   {
2010     int result = debuginfod_validate_imasig(c, target_cache_tmppath, fd);
2011     if(0 == result)
2012     {
2013       if (vfd >= 0) dprintf (vfd, "the signature is valid\n");
2014     }
2015     else if(EINVAL == result || enforcing == url_ima_policies[committed_to])
2016     {
2017       // All invalid signatures are rejected.
2018       // Additionally in enforcing mode any non-valid signature is rejected, so by reaching
2019       // this case we do so since we know it is not valid. Note - this not just invalid signatures
2020       // but also signatures that cannot be validated
2021       if (vfd >= 0) dprintf (vfd, "ALERT: this download is being rejected since the IMA signature could not be verified\n");
2022       rc = -EPERM;
2023       goto out2;
2024     }
2025     else
2026     {
2027       // By default we are permissive, so since the signature isn't invalid we
2028       // give it the benefit of the doubt
2029       if (vfd >= 0) dprintf (vfd, "the signature could not be verified\n");
2030     }
2031   }
2032 

Here is the relevant snippet
Comment 24 Mark Wielaard 2023-08-25 15:24:46 UTC
BTW. How does this interact with the "section" queries?

If the server doesn't support "section" then the debuginfod-client fallback to fetching "debuginfo" or "executable" should do the signature checking.

But there doesn't seem to be a way to make (client side) signature checking work when fetching "section". Does this mean that in enforcing mode we always need to use the fallback mode?
Comment 25 Mark Wielaard 2023-08-25 16:43:56 UTC
So I am looking at users/rgoldber/try-bz28204d but it isn't clear you want to merge that in separate commits or squashed together. I am comparing to users/rgoldber/try-bz28204c which I believe is the previous version reviewed.

It really makes things a lot easier if the actual patches that are intended to be merged are posted (with a description of what was changed since the last review).

So as far as I can tell the new series has been rebased from commit 35e059b654224b1a01d05877b13582c74c692388 to 27a84961f7a061b83f10f7e02bf433c229d6537a. Good. That is just 3 commits.

- configure.ac checks updated.
- debuginfod/debuginfod-client.c introduces ima_policy_t
  Includes an "undefined" policy?
  debuginfod_validate_imasig updated to read/digest in chunks of DATA_SIZE.
  Is the k += DATA_SIZE correct? Can't pread return an n < DATA_SIZE?
  If the cert_paths = strdup (...) fails cert_paths gets assigned a static string?
  Won't that crash the strtok calls or the free (cert_path) call?
  In debuginfod_query_server the server_urls are parsed to see te ima policy,
  as described debuginfod-client-config.7

Sorry, have to stop for a bit. Will try to look at the rest later.
Comment 26 Mark Wielaard 2023-08-26 22:25:19 UTC
(In reply to Ryan Goldberg from comment #23)
> (In reply to Mark Wielaard from comment #22)
> > This still feels odd. Since you cannot distinguish between non-sig f36
> > package (okish?) and non-sig f38 packages (bad?). I think you have to either
> > trust or reject all non-sig packages from such a server.
> I see where you're coming from but what policy would you use for
> https://debuginfod.fedoraproject.org?

I think that depends on which release I am using.
Normally I would be debugging/profiling local programs/libraries.
So for f38 I would like to a policy that checks the ima signatures.

If I am on f36 or if e.g. I expect to be inspecting a non-local core file based on f36 then I would not use a policy that checks the ima signatures, because they might not be there.

> > I'll look at the code to see if I understand what this means in practice.
> > Are those the messages presented to the user (in verbose mode? always?). And
> > does this mean all three cases warn (or only the second and third)? And is
> > it just a message or does it also mean actual rejection in some cases?
>
> 2008   if(NULL != url_ima_policies && ignore != url_ima_policies[committed_to])
> 2009   {
> 2010     int result = debuginfod_validate_imasig(c, target_cache_tmppath, fd);
> 2011     if(0 == result)
> 2012     {
> 2013       if (vfd >= 0) dprintf (vfd, "the signature is valid\n");
> 2014     }
> 2015     else if(EINVAL == result || enforcing == url_ima_policies[committed_to])
> 2016     {
> 2017       // All invalid signatures are rejected.
> 2018       // Additionally in enforcing mode any non-valid signature is rejected, so by reaching
> 2019       // this case we do so since we know it is not valid. Note - this not just invalid signatures
> 2020       // but also signatures that cannot be validated
> 2021       if (vfd >= 0) dprintf (vfd, "ALERT: this download is being rejected since the IMA signature could not be verified\n");
> 2022       rc = -EPERM;
> 2023       goto out2;
> 2024     }
> 2025     else
> 2026     {
> 2027       // By default we are permissive, so since the signature isn't invalid we
> 2028       // give it the benefit of the doubt
> 2029       if (vfd >= 0) dprintf (vfd, "the signature could not be verified\n");
> 2030     }
> 2031   }
>
> Here is the relevant snippet

Thanks.

So if the policy is "ignore" nothing is checked.
Otherwise the ima signature is checked.
If the policy is enforcing we return EPERM.
Otherwise we return the target/OK, independent of the result of the ima sig check.
All notices are printed if we are in verbose mode.

BTW. The last notice isn't really accurate.
The signature might have been verified. In which case it was invalid.

In practice "permissive" (or default) mode is no different from "ignore".
Which imho makes permissive mode not very useful.

So I think we shouldn't have a permissive mode, just use ignore.

If we have an permissive mode then I think it should work like the selinux permissive mode.
That is, it should always check the signature, but instead of failing with EPERM, it should
always produce a warning (whether or not we are in verbose mode or not).
Comment 27 Ryan Goldberg 2023-08-28 14:40:04 UTC
(In reply to Mark Wielaard from comment #24)
> BTW. How does this interact with the "section" queries?
Since these aren't ima verifiable anyways wdyt of just skipping verification all together (i.e treat that query in the same way as the ignore policy)

Tweaking the above to something like:
2008    if(NULL != url_ima_policies && ignore != url_ima_policies[committed_to] && NULL == section) { ... }

(In reply to Mark Wielaard from comment #25)
>   Includes an "undefined" policy?
Just used internally when parsing DEBUGINFOD_URLS
>   Is the k += DATA_SIZE correct? Can't pread return an n < DATA_SIZE?
Fixed
>   If the cert_paths = strdup (...) fails cert_paths gets assigned a static string?
Fixed

(In reply to Mark Wielaard from comment #26)
> If we have an permissive mode then I think it should work like the selinux
> permissive mode.
> That is, it should always check the signature, but instead of failing with
> EPERM, it should
> always produce a warning (whether or not we are in verbose mode or not).
I would be ok with this kind of permissive mode
Comment 28 Mark Wielaard 2023-08-29 13:09:04 UTC
(In reply to Ryan Goldberg from comment #27)
> (In reply to Mark Wielaard from comment #24)
> > BTW. How does this interact with the "section" queries?
> Since these aren't ima verifiable anyways wdyt of just skipping verification
> all together (i.e treat that query in the same way as the ignore policy)

Assuming ima verification is so one can be sure bits are what a distro really distributes I don't think a default to ignore policy is useful. I think in enforcing mode there is no good way to fetch just the section data and the client has to fall back onto requesting the full executable or debuginfo file, verify the ima signature, and then locally extract the section data (of course this is somewhat inefficient).

> Tweaking the above to something like:
> 2008    if(NULL != url_ima_policies && ignore !=
> url_ima_policies[committed_to] && NULL == section) { ... }
> 
> (In reply to Mark Wielaard from comment #25)
> >   Includes an "undefined" policy?
> Just used internally when parsing DEBUGINFOD_URLS
> >   Is the k += DATA_SIZE correct? Can't pread return an n < DATA_SIZE?
> Fixed
> >   If the cert_paths = strdup (...) fails cert_paths gets assigned a static string?
> Fixed

Thanks. It would be good to post a patch with these fixes (maybe rebased to current git trunk). So it is more clear what exact change you intent to commit.

> (In reply to Mark Wielaard from comment #26)
> > If we have an permissive mode then I think it should work like the selinux
> > permissive mode.
> > That is, it should always check the signature, but instead of failing with
> > EPERM, it should
> > always produce a warning (whether or not we are in verbose mode or not).
> I would be ok with this kind of permissive mode

This would have my preference too. Or just simply have either an ignore or enforcing mode.

But from our discussion on irc I think Frank believes it is better to see permissive mode as a kind of possible checksum mode. I am not sure about that use case.

Given that we have reliable transports a checksum mode might be something for the server to do (assuming the server doesn't trust its own storage). The server could do an ima check before sending any file and simply don't sent it if the ima check(sum) fails. This then would be independent of the client ima check (enforced or not).