search

linux-application-whitelisting / fapolicyd

File Access Policy Daemon
GNU General Public License v3.0
192 stars 55 forks source link

Ebuild backend #274

Open Kangie opened 9 months ago

Kangie commented 9 months ago

I stumbled across fapolicyd and thought "that sounds pretty cool" and "I bet I could teach it to read the portage database".

After a month or so of "Fun" this PR is the result.

It's currently a WIP but I figured that good enough to request feedback and see if (once I write some tests and validate that I didn't break anything else...) we can sneak it in before the next release.

I am a C novice, so please let me know what can be improved!

Highlights:

TODOs:

Kangie commented 8 months ago

@stevegrubb

Here's the other work I'm hoping to land before the next release, but if we don't make it I can snapshot the code for Gentoo after it's merged.

277 has been raised to separate out the deb-backend changes; I'll rebase once that's merged so that there's less to review here.

Would you like me to split out this commit too, or should we squash all of the ebuild-related mess in the middle down into a few logical commits?

Edit: I have my TODO up the top, but aside from getting derailed by some 'not caused by my changes' ASAN stuff that I'm debugging in the background, I need to:

On my todo, but not part of this PR, is adding a Gentoo CI/CD stage - there's some work that I'll need to do to come up with something suitable for downstream use. I suppose a short-term alternative, as the ebuild backend adds no new dependencies, would be to tarball that up as test data and just extract it in the test container?

I'll ping you again once I think the code is ready for review, but if you have any early feedback I'm willing to address it. :)

Edit the second: It won't make this PR / release, but would you be receptive to a Meson build system as an alternative to (or replacement for) autotools? It's starting to get to me a bit, so I'm willing to put in the porting work.

stevegrubb commented 8 months ago

PR #277 appears ready to merge. Do we want to do that first and then rework this patch (if needed)? I have not yet looked this patch over.

Kangie commented 8 months ago

PR #277 appears ready to merge. Do we want to do that first and then rework this patch (if needed)? I have not yet looked this patch over.

Yes, please - that's a prereq for this PR. I've had some people better at C than I provide some feedback offline and I'll be addressing it over the next few days.

Kangie commented 8 months ago

I've adapted the deb test to provide some basic assurances that the ebuild backend can read a vdb. I spent a bit of time today working on generating a psuedo-vdb that would enable us to test this without relying on a real Portage installation / database, but I didn't get there in the end. I'll keep plugging away at it in the background, but it may not make it in here.

I'm reasonably happy with the current state, reading the VDB is fast enough, though I could stand to work out if there's anything else on a Gentoo system can be safely filtered to reduce the amount of md5ing that happens when populating the trust db.

Happy to address any feedback!

image

stevegrubb commented 8 months ago

OK, the other patch was merged which appears to create a couple conflicts that need fixing. I'll see if I can look this PR over soon.

stevegrubb commented 8 months ago

Was it really necessary to rename init.d to extra? That added a whole lot of churn.

There is a lot of code in /usr/share. It's in the HFS docs as the place for purely interpreted libraries.

Have you compiled and run this with address/undefined behavior sanitizers?

Kangie commented 7 months ago

Apologies for the delayed response, I got deployed to help out with the response to the local severe weather event - today is my first day back at the keyboard.

Was it really necessary to rename init.d to extra? That added a whole lot of churn.

It seemed like the best way to separate the systemd init scripts, new openrc init stuff, and the application data / configuration that was all stored together under the 'init' folder. If you have a better way of organising it (or just one that you prefer) I'm happy to amend that commit.

There is a lot of code in /usr/share. It's in the HFS docs as the place for purely interpreted libraries.

In Gentoo it's 'architecture independent application data which is not modified at runtime.', but I do see some python, JS, and... C header files in amongst all the other binary files installed there on my development box.

I'll have to come up with a better filter. I'll force push and drop that for the short term.

Have you compiled and run this with address/undefined behavior sanitizers?

Yes. I actually have a branch where I'm tracking down some weirdness that I thought I caused but apparently existed in the code before I started touching it. I have a minimal broken example so if I can't work that out I'll log a bug:

library/escape.c:100:9: runtime error: load of address 0x7fb4ae604175 with insufficient space for an object of type 'char'
0x7fb4ae604175: note: pointer points here
 72 6f 6f 74 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
             ^ 
    #0 0x55bd93a119a6 in unescape_shell library/escape.c:100
    #1 0x55bd939f9c8f in handle_mounts daemon/fapolicyd.c:373
    #2 0x55bd939f8661 in main daemon/fapolicyd.c:637
    #3 0x7fb4b325fee9 in __libc_start_call_main (/usr/lib64/libc.so.6+0x23ee9)
    #4 0x7fb4b325ffa4 in __libc_start_main_alias_1 (/usr/lib64/libc.so.6+0x23fa4)
    #5 0x55bd939f96c0 in _start (/data/development/temp/fapolicyd/src/fapolicyd+0x946c0)

library/escape.c:116:7: runtime error: store to address 0x7fb4ae604175 with insufficient space for an object of type 'char'
0x7fb4ae604175: note: pointer points here
 72 6f 6f 74 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00
             ^ 
    #0 0x55bd93a11cad in unescape_shell library/escape.c:116
    #1 0x55bd939f9c8f in handle_mounts daemon/fapolicyd.c:373
    #2 0x55bd939f8661 in main daemon/fapolicyd.c:637
    #3 0x7fb4b325fee9 in __libc_start_call_main (/usr/lib64/libc.so.6+0x23ee9)
    #4 0x7fb4b325ffa4 in __libc_start_main_alias_1 (/usr/lib64/libc.so.6+0x23fa4)
    #5 0x55bd939f96c0 in _start (/data/development/temp/fapolicyd/src/fapolicyd+0x946c0)

I am currently leaking 13 bytes allocated here that I haven't been able to track down, I'll try and look into that soon but I'm still recovering from working a week and a half of my vacation!

stevegrubb commented 7 months ago

Hello, sounds like you've been busy. I think it might be best to not change the name of the directory as part of this patch set. That can/should be done separately. The idea is to minimize the changes in case we need to bisect back to this. The original idea was that everything related to config and initialization would live in the init directory. Let's address this after this patch is merged. Also, I think we should schedule this patch for after the 1.3.3 release so that we can get other bug fixes out to the community. Maybe since this is a new platform being on-boarded, we call the next release 1.4.

Regarding /usr/share, you might look at how we do this for rpm. We use a config file, fapolicyd-filter.conf. There is a library/filter.h file that shows the API that uses it. We had to resort to this because we found that we may be getting rid of files that the user really wants to trust. So, we gave them a way to take control.

As for the address sanitizer issue, I think that should be filed separately and treated as a 1.3.3 release blocker. I can see an issue it my testing too. Thanks for spotting it. Issue #281 was opened for this. @radosroka , have you seen the above address sanitizer issue?

Kangie commented 7 months ago

I think it might be best to not change the name of the directory as part of this patch set. That can/should be done separately. The idea is to minimize the changes in case we need to bisect back to this. The original idea was that everything related to config and initialization would live in the init directory

Would you be happy for a patch/PR that moves the existing files into the new structure so that the 'gentoo PR' can just add the openrc files, or is there a strong preference for sticking with the existing approach?

Also, I think we should schedule this patch for after the 1.3.3 release so that we can get other bug fixes out to the community. Maybe since this is a new platform being on-boarded, we call the next release 1.4.

No worries, less time pressure to look at filtering. :smile:

Gentoo has a binary package host now, so it will be sane to add a containerised CI/CD option where it still wasn't late last year - I'll look into that too if I have time!

Regarding /usr/share, you might look at how we do this for rpm. We use a config file, fapolicyd-filter.conf. There is a library/filter.h file that shows the API that uses it. We had to resort to this because we found that we may be getting rid of files that the user really wants to trust. So, we gave them a way to take control.

Will do.

radosroka commented 7 months ago

library/escape.c:100:9: runtime error: load of address 0x7fb4ae604175 with insufficient space for an object of type 'char' 0x7fb4ae604175: note: pointer points here 72 6f 6f 74 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^

0 0x55bd93a119a6 in unescape_shell library/escape.c:100

#1 0x55bd939f9c8f in handle_mounts daemon/fapolicyd.c:373
#2 0x55bd939f8661 in main daemon/fapolicyd.c:637
#3 0x7fb4b325fee9 in __libc_start_call_main (/usr/lib64/libc.so.6+0x23ee9)
#4 0x7fb4b325ffa4 in __libc_start_main_alias_1 (/usr/lib64/libc.so.6+0x23fa4)
#5 0x55bd939f96c0 in _start (/data/development/temp/fapolicyd/src/fapolicyd+0x946c0)

library/escape.c:116:7: runtime error: store to address 0x7fb4ae604175 with insufficient space for an object of type 'char' 0x7fb4ae604175: note: pointer points here 72 6f 6f 74 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^

0 0x55bd93a11cad in unescape_shell library/escape.c:116

#1 0x55bd939f9c8f in handle_mounts daemon/fapolicyd.c:373
#2 0x55bd939f8661 in main daemon/fapolicyd.c:637
#3 0x7fb4b325fee9 in __libc_start_call_main (/usr/lib64/libc.so.6+0x23ee9)
#4 0x7fb4b325ffa4 in __libc_start_main_alias_1 (/usr/lib64/libc.so.6+0x23fa4)
#5 0x55bd939f96c0 in _start (/data/development/temp/fapolicyd/src/fapolicyd+0x946c0)


I am currently leaking 13 bytes [allocated here](https://github.com/linux-application-whitelisting/fapolicyd/pull/274/files#diff-b2e2388c4837a311031ffe6082dfdc129c60bad8def1a9f1fbe5f73ad3f90622R284) that I haven't been able to track down, I'll try and look into that soon but I'm still recovering from working a week and a half of my vacation!

Is this from the daemon itself or CLI?

Kangie commented 7 months ago

It's from the daemon itself.

radosroka commented 7 months ago

It's from the daemon itself.

And was it on this branch or main?

Kangie commented 4 months ago

Not forgotten, just had a few other projects eat up my time. I'll aim to get this finished off for late April / Early may; I have some leave coming up :)