Hi Frank, On Fri, 2023-10-27 at 15:15 -0400, Frank Ch. Eigler wrote: > > > I would not expect the emailed patch to apply, esp. with all the other > > > work done in the intermediate months, which is why the code is also in > > > the git branch. The binary files do not seem effectively reviewable > > > anyway. > > > > It would be really convenient though. And modern git format-patch does > > includes base tree information which allows tools to stich commits at > > the right place. > > (I would be surprised if many-month-old patches could just be > automatically "stitched".) > > > It would also help with patchwork and pre-commit CI. > > https://git-scm.com/docs/git-format-patch#_base_tree_information > > Considering how easily the trybots can process the actual code - and > have done so before posting the patch for review - we can consider > some CI well done already. After approval but before merge, it would > undergo another round of trybotting. With such workflow, patchwork > does not need to concern itself with additional pre-commit CI/CD.
My point is really that posting with git format-patch or send-email makes it possible for someone to simply use git am, b4 or git pw to try out a patch. If the patch doesn't apply then that will be the first review issue. > > > > [...] > > > > > The default is ima:permissive mode, which allows signatures to > > > > > function like a checksum to detect accidental corruption, but > > > > > accepts > > > > > operation in a mix of signed and unsigned packages & servers. > > > > > > > > I still think "permissive" is confusing. Since it is a term also used > > > > by e.g. selinux, but doesn't work that way. And it doesn't seem > > > > connected with the threat-model that enforcing protects against. > > > > > > The connection is the following: > > > "enforcing" mode protects against accidental or deliberate (MITM) > > > corruption. > > > "permissive" mode protects against accidental corruption. > > > > > > > Since it is a different concept maybe it shouldn't be part of this > > > > patch. It is a form of integrity checking, but doesn't protect (or > > > > warns) about integrity failures. > > > > > > It does protect and warn against integrity failures of the form of > > > incorrect signatures. > > > My issue is that I don't really understand "permissive". Originally I > > assumed it was like selinux permissive mode, it does do the checks, > > but if they fail we just warn and continue. That seems a clear concept. > > The proposed documentation explains it thusly: > > ima:enforcing Every downloaded file requires a valid signature. > > ima:permissive Every downloaded file accompanied by a signature > must be valid, but downloads without signatures are accepted. > > ima:ignore Skips verification altogether. > > You're right that it is not an exact match for the selinux concept. > But if one's not hunting around for a precise analogy, and just reads > the single sentence description, it tries to be clear. > > > [...] if there is a signature, but we don't have the corresponding > > certificate to check it against, should it still fail, or is it > > more like a none-signed file and we can be "permissive" and accept > > it? Maybe I don't have enough imagination. > > I see your point. One could make an argument either way, coming from > fuzziness with the concept of an "invalid signature". One could > clarify with a rewording to "known-invalid". Then "permissive" > becomes permit everything except known-invalid files. Missing > certificates would not qualify as known-invalid, merely unknown. > Would you like me to draft up a sentence or two description of that > concept for the man page? > > The intended benefit of permissive mode as a default is to give > elfutils users as much reassurance possible, without requiring manual > configuration changes or manual downloads. See also the certificate > distribution topic below - it's really toward the same goal. I think my issue is really that it is unclear what "reassurances" we are making. What is the threat-model? Permissive says to me that although checks are done, they don't block receiving files. Maybe calling it ima:known-valid ? ima:checking (if you reject unknown- valid)? I don't have an issue with ima:enforcing, that seems to have clear semantics. The threat-model is clear, you only want to get files that are signed by a specific set of keys/certificates you trust. No middle- person acting as an intermediary between the distributor and user can circumvent that. > > [...] > > > Yes, it is odd. Unfortunately, it is not possible to enforce crypto > > > signatures from distros upon unsigned slices. A couple of possible > > > solutions: > > > [...] > > > - disable section queries from enforcing-mode servers (which could > > > then nuke gdbindex capability for e.g. future fedora/gdb users) > > [...] > > > > I think only option 2 makes sense given the enforcing threat-model. > > > > Optionally we could do the section part locally, download the whole > > file, check the ima signature, then provide the application with just > > the section data. > > Yeah. That is what I was thinking, just not expressing properly. OK. But again in enforcing mode this is simple and clear, the semantics for "permissive" not so much imho. > > > > Including default system directories seems fine. But I don't think > > > > elfutils should ship certificates itself. That is the job of the > > > > distro or user. > > > > > > The user or the distro the user is running may not be the same one > > > that the binaries the user is debugging comes from. By shipping > > > Fedora/RHEL/CentOS certificates, we allow a Ubuntu person to debug a > > > RHEL container, and trust debuginfod content for it. > > > > But it should be the distro/user who makes that choice. We cannot > > decide for others who they trust as provider of the files they > > download. > > They already make the decision whom they download debuginfo from. > That's literally what setting $DEBUGINFOD_URLS is. The scenario > you're describing would be trusting a server enough to supply content, > trusting our code to fetch & check that content, but not trusting us > to redistribute public certificates for the servers. The user shouldn't trust a middle-person. Unless we are signing those files we shouldn't distribute the certificates are being trustable imho. > > > > We aren't in a position to make sure the certificates are valid > > > > and/or can be trusted. > > > > > > Why not? We can document where we got them - I believe they are all > > > public somewhere or other already. > > > > We certainly should document that and provide pointers to where > > distros publish their certificates. But we shouldn't install them by > > default. The distro/user can make their own choice of using them, just > > like they decide whether or not the have default DEBUGINFOD_URLS. > > An elfutils-carrying distro can already decide what to do with out > certificates by including or excluding them from their package. They > govern what's installed by default, not we. By including the certs in > elfutils, we are making it easy for a packager to pass these on, if > they wish. So you propose we setup a curating process to decide which certificates to include? If we do then I would suggest we create a separate package for that (or just a separate tar ball or repository). But I really don't think we are the right party to do that. > Another way of looking at it is to remind ourselves of the goal of > this permissive/cert-distribution default mentality: to provide > maximum possible assurance possible out of the box. See above, what is this "maximum possible assurance" that "permissive" provides? And how does that interact with the enforcing policy? > > > > [...] > > > > It would be good to add some comments for extract_skid, I am not sure > > > > I understand how this works. > > > > > > (Ditto.) > > > > I do understand hex2dec, but I don't understand what extract_skid > > does. Maybe add an explanation what a certificates subject key id is > > and why we need it. > > (I meant I'm not sure how this works either. :-) It's based on code > from imaevm.) That concerns me a little. > > [...] > > > Not sure, but this is how libimaevm.c similar code does it. I assume > > > the first byte of the signature is something else. > > > (https://git.code.sf.net/p/linux-ima/ima-evm-utils) > > > > ewww. Does this pass ubsan (--enable-sanitize-undefined)? > > Haven't tried but it passes valgrind. valgrind doesn't check for undefined behavior or type alignment requirements. > > The issue is that this seems to access structure values at a > > possibly unaligned address. > > Interesting. > > > > > > What does init_public_keys do? Is it thread-safe? > > > > > > Good catch. It initialized a global inside libimaevm.c. It does not > > > appear thread-safe. Will wrap this in a pthread-once or somesuch. > > > libimaevm.c seems not thread-safe in general. You might have to put > > a big lock around the whole signature extraction/checking block > > which uses those library functions. > > OK, will take a look at that. What other global-state conflicting > things did you notice? A quick look at the code shows that various functions can read/write a static public_keys variable linked list, including (indirectly) ima_verify_signature. So that can cause data-races. One other issue I noticed is that it seems to be distributed under GPLv2-only. Which seems to mean that anything based on it should also be distributed under GPLv2-only, which would include libdebuginfod. Is there code we can rely on that is GPLv2+ and LGPLv3+ compatible? > > Another possible issue might be the initialization of openssl in the > > static constructure. How does that interact with how libcurl inits > > openssl? > > openssl's initialization is fine & thread-safe in practice, despite > the documentation's warnings. OK. But even if it is thread-safe, you also need to make sure it inits the same. This for example worries me a little: https://github.com/curl/curl/pull/12153 Cheers, Mark
