Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Kasatkin, Dmitry]

On Fri, Mar 15, 2013 at 5:41 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Thu, Mar 14, 2013 at 11:08:45PM +0200, Kasatkin, Dmitry wrote:
>> On Thu, Mar 14, 2013 at 10:37 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> > On Thu, Mar 14, 2013 at 04:30:28PM -0400, Vivek Goyal wrote:
>> >
>> > [..]
>> >> I thought explicitly using signature format is more intutive. Exporting
>> >> signature version is not. I personally associate versioning with minor
>> >> changes like addition of some fields etc.
>> >
>> > For instance, you might want to add some fields to  signature_v2_hdr down
>> > the line. I think even after that change, it still remains "asymmetric
>> > signature" just that structure size changes and there is additional
>> > info. If there is versioning info assciated with signature type
>> > ASYMMETRIC, we could simple bump it to 1.1 or whatever and keep the
>> > version detail internal to ima/integrity subsystem.
>> >
>>
>> Yes, it will make some things cleaner.
>
> So do you agree that every new signature format should have a new entry
> and we can introduce new signature type for asymmetric signature.
>

Hi Vivek,

I was/am a bit busy with responding.
I will do it asap.

Thanks,
Dmitry

>> We recently discussed with Mimi how to extend IMA with memory locking and
>> one of the ideas was to use a flag in the signature header to indicate
>> that we need
>> require memory locking. There we need new subversion of the asymmetric
>> signature type.
>
> I have few concerns here.
>
> - Whether a file should be mem locked or not is not file property. It
>   should not be stored in the file signature when file is being signed.
>   Whether file should be locked or not depends on the caller and that
>   in turn depends on the system environment.
>
>   For example, one can create a system where whole of the user space is
>   signed. (Extending secureboot fully to the user space). I think in that
>   system one might not need to lock executables/files in memory.
>
> - IMA should not be mmaping files in process address space. IMA does not
>   know what should be mapped where. For example, executable files.
>   Respective binary loader knows what sections of the file should be
>   mapped at what address.
>
>   IIUC, do_mmap_pgoff() will map file at first suitable available address.
>
>   Secondly will IMA return with file mapped and locked or will unmap file
>   before returning. If it does not unmap then it can conflict with binary
>   loader who wants to map a section of file on a particular address which
>   is already used. And if IMA unmaps the file before returning to caller,
>   then mapping and mlocking has no benefit.
>
> - Special hardcoded handling of BPRM_CHECK hook sounds like a kludge.
>
> +       if (function == BPRM_CHECK && (iint->flags & IMA_DIGSIG))
> +               iint->flags &= ~IMA_DONE_MASK;
>
>   IMA in general has the issue of direct writes to disk. If we want to solve
>   the caching issues, these should be solved in general and not be made hook
>   specific.
>
>   Even with ima_appraise_tcb policy in effect, there are vulnerabilities.
>   ima_appraise_tcb will appraise only root files and a member of group
>   "disk" can directly write to disk without being appraised and bypass all
>   the caching logic.
>
> - Memory locking will not solve the problem of knowing what did successful
>   appraisal mean. User needs to know whether its rule got executed or not
>   and based on that it can make a decision.
>
> I really think that it is caller who should decide whether a file should
> be locked or not. If there is a need, caller should first map full or part
> file at appropriate address range and lock mapped pages in memory and
> then call into IMA for signature verification.
>
>>
>> Please have a look to 3 top patches.
>>
>> http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=working
>>
>> It is a prototype to verify signature with arbitrary hash algorithm.
>
> Thanks. I had a quick look. Hash related improvements look fine to me.
>
>> Top patch also locks the memory if executable is verified and it has
>> asymmetric signature type.
>
> I have concerned with ima memory locking as mentioned above. We need to
> have some discussion here first w.r.t what problem we are solving by
> locking files on bprm_check hook and whether it is appropriate to do
> it in IMA or not.
>
>> This unconditional locking is just for development and we might use
>> mlock bit from signature header.
>
> As mentioned above, I have concerns here. Whether a file should be
> memlocked or not is not a property of file or file signature.
>
>> Notice, that patch reset appraisal status when we get BPRM check and
>> there is a signature.
>
> This is also shouds like a kludge. Why BPRM_CHECK only is special. Same
> problem will exist with any other measurement hook, isn't it.
>
>>
>> BTW, do you have a kernel repository somewhere.
>> It is often easier to understand and discuss when seeing "complete"
>> set of commits.
>
> No, I don't have yet. The moment I have something (even in raw form), I
> will post patches as RFC. If things still don't work, I will setup a
> repository somewhere.
>
> Thanks
> Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/