Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Account for kernel.pid_max > 65535 #13

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

mrc0mmand
Copy link
Member

Today's kernel.pid_max= is usually set to 2^22 (4194304) instead of
the original 2^16 (65535), causing buffer overflow when trying to write
/proc/<PID>/cmdline into the stack-allocated buffer. Let's fix this by
using the maximum "string length" of int (calculated using a macro
borrowed from systemd, which might be useful in the future).

Fixes: #11

@evverx
Copy link
Member

evverx commented Feb 18, 2022

Thanks! Looking at https://github.com/matusmarhefka/dfuzzer/pull/8 I think df_open_proc_status_file could probably benefit from this as well. It has nothing to do with #11 though :-)

@willlling
Copy link

Your approach is clearly more thorough! I just solve the problem at hand in #8 .

@matusmarhefka
Copy link
Collaborator

@mrc0mmand @evverx Due to my job duties I am not always able to respond in timely manner to the PRs in this project (even though I try to review/merge PRs when I can). I am glad that people are still interested and use this project and I am okay with giving you merge rights to this repository if you plan to continue working on more improvements/fixes. Please let me know if you are interested.

@evverx
Copy link
Member

evverx commented Apr 7, 2022

I am glad that people are still interested and use this project

@matusmarhefka I think as long as dfuzzer can discover issues like systemd/systemd#22555 it should probably be used by projects providing dbus interfaces on a regular basis. Thanks for dfuzzer!

I am okay with giving you merge rights to this repository if you plan to continue working on more improvements/fixes

To make it easier to integrate dfuzzer into the systemd CI it was forked and improved a bit in https://github.com/systemd/systemd-dfuzzer. I think it should be possible to bring those patches back but given that some of them are kind of systemd (upstream)-specific in the sense that for example dfuzzer no longer shows package versions there I'm not sure what to do in order to keep it less focused on upstream projects. I'd wait for @mrc0mmand to chime in.

@thrix
Copy link
Collaborator

thrix commented Apr 7, 2022

@matusmarhefka btw we could create a generic tmt test for dfuzzer which users could use to easily fuzz tier dbus interfaces :) Adding them to RHEL, Fedora and CentOS Stream would be very simple. WDYT?

@evverx
Copy link
Member

evverx commented Apr 7, 2022

@thrix I'm not particularly familiar with tmt but looking at https://docs.fedoraproject.org/en-US/ci/tmt/ I'm not sure it would help to test systemd. The problem is that it's kind of hard to even boot VMs when systemd is compiled with ASan/UBSan so tmt would somehow have to create a script running as PID1 and preparing the runtime environment and then replacing itself with systemd built with ASan/UBsan. Another issue is that it isn't always clear where to look for ASan/UBsan backtraces. It was discussed in systemd/systemd#22547 (comment) and was solved by blocking a couple of dbus methods messing up the journal.

@evverx
Copy link
Member

evverx commented Apr 7, 2022

Just to clarify, I'm not saying that dfuzzer shouldn't be combined with tmt. It should probably make it easier for some projects to integrate dfuzzer into their CI. But in terms of testing systemd it would be great if it was possible to pass a seed corpus to catch issues like systemd/systemd#22477 and additionally fuzz advanced signatures. In other words the issues the systemd project ran into have almost nothing to do with how to specify and run the tests.

@thrix
Copy link
Collaborator

thrix commented Apr 8, 2022

@evverx I see, I was more targetting other components which have DBus interfaces, I understand systemd is special here and needs special handling, so might not be suitable ...

@evverx
Copy link
Member

evverx commented Apr 8, 2022

@thrix I agree in most cases it should be possible to automate that with tmt. It could automagically install dfuzzer, optionally wrap programs in valgrind (or build them with ASan/UBsan) and unleash dfuzzer on them. To make dfuzzer compatible with programs compiled with ASan something like systemd/systemd#22547 (comment) (where its memory leak detector was turned off) should be implemented though.

@mrc0mmand
Copy link
Member Author

I am glad that people are still interested and use this project

@matusmarhefka I think as long as dfuzzer can discover issues like systemd/systemd#22555 it should probably be used by projects providing dbus interfaces on a regular basis. Thanks for dfuzzer!

I am okay with giving you merge rights to this repository if you plan to continue working on more improvements/fixes

To make it easier to integrate dfuzzer into the systemd CI it was forked and improved a bit in https://github.com/systemd/systemd-dfuzzer. I think it should be possible to bring those patches back but given that some of them are kind of systemd (upstream)-specific in the sense that for example dfuzzer no longer shows package versions there I'm not sure what to do in order to keep it less focused on upstream projects. I'd wait for @mrc0mmand to chime in.

It would be definitely great to move the development back to this repository, since the systemd fork was meant to be just temporary. Once I get a bit of a spare time, I could cherry-pick the newly introduced patches and open the respective PRs.

@matusmarhefka
Copy link
Collaborator

@matusmarhefka btw we could create a generic tmt test for dfuzzer which users could use to easily fuzz tier dbus interfaces :) Adding them to RHEL, Fedora and CentOS Stream would be very simple. WDYT?

@thrix Sounds good, so your idea is to provide a generic tmt test in this repo which could be parametrized with bus name? Btw I was also thinking about introducing Packit and Testing Farm support in this repository.

@matusmarhefka
Copy link
Collaborator

I am glad that people are still interested and use this project

@matusmarhefka I think as long as dfuzzer can discover issues like systemd/systemd#22555 it should probably be used by projects providing dbus interfaces on a regular basis. Thanks for dfuzzer!

I am okay with giving you merge rights to this repository if you plan to continue working on more improvements/fixes

To make it easier to integrate dfuzzer into the systemd CI it was forked and improved a bit in https://github.com/systemd/systemd-dfuzzer. I think it should be possible to bring those patches back but given that some of them are kind of systemd (upstream)-specific in the sense that for example dfuzzer no longer shows package versions there I'm not sure what to do in order to keep it less focused on upstream projects. I'd wait for @mrc0mmand to chime in.

It would be definitely great to move the development back to this repository, since the systemd fork was meant to be just temporary. Once I get a bit of a spare time, I could cherry-pick the newly introduced patches and open the respective PRs.

@mrc0mmand That would be great. I can give you guys commit rights so you can review and merge new PRs. Can I add you @mrc0mmand and also @evverx ?

@matusmarhefka
Copy link
Collaborator

@mrc0mmand @evverx I've added you as collaborators, so you should now have commit rights :)

@evverx
Copy link
Member

evverx commented Apr 11, 2022

@matusmarhefka thanks!

Btw I was also thinking about introducing Packit and Testing Farm support in this repository.

FWIW to make sure dfuzzer can be built and run (with and without ASan/UBSan) @mrc0mmand integrated GHActions into the "systemd-dfuzzer" repository: https://github.com/systemd/systemd-dfuzzer/blob/main/.github/workflows/build.yml. Unlike Packit it doesn't cover various architectures the package can be built on though.

Also LGTM was integrated there: https://lgtm.com/projects/g/systemd/systemd-dfuzzer/alerts/?mode=tree. I added the dffuzer repository to LGTM as well: https://lgtm.com/projects/g/matusmarhefka/dfuzzer?mode=tree but since I'm not the owner I can't install it to the repository to make it analyze PRs. Could you install LGTM to the dfuzzer repository: https://lgtm.com/projects/g/matusmarhefka/dfuzzer/ci?

@matusmarhefka
Copy link
Collaborator

Done.

@evverx
Copy link
Member

evverx commented Apr 11, 2022

Thanks! It seems to be working.

I've just opened https://github.com/matusmarhefka/dfuzzer/pull/14 (where the C code is analyzed is well). Without that patch only the python code is covered.

@evverx
Copy link
Member

evverx commented Apr 11, 2022

FWIW @mrc0mmand fixed most issues LGTM found so I'd just ignore the alerts until patches like systemd/systemd-dfuzzer#6 are backported.

@mrc0mmand
Copy link
Member Author

mrc0mmand commented Apr 13, 2022

Thanks, again, for all the work and sorry for the delay, a lot of stuff has piled up. I've started going through the patches and opened #15 in the first batch. To continue further, I need this PR to get merged to avoid conflicts, so once #15 is approved & merged, I'll rebase this one to trigger CIs to move it forward.

Today's `kernel.pid_max=` is usually set to 2^22 (4194304) instead of
the original 2^16 (65535), causing buffer overflow when trying to write
`/proc/<PID>/cmdline` into the stack-allocated buffer. Let's fix this by
using the maximum "string length" of int (calculated using a macro
borrowed from systemd, which might be useful in the future).

Fixes: dbus-fuzzer#11
@mrc0mmand mrc0mmand merged commit 134260f into dbus-fuzzer:master Apr 15, 2022
@mrc0mmand mrc0mmand deleted the fix-buffer-overflow branch April 15, 2022 13:17
@evverx
Copy link
Member

evverx commented Apr 17, 2022

@thrix Sounds good, so your idea is to provide a generic tmt test in this repo which could be parametrized with bus name?

@matusmarhefka I opened https://github.com/matusmarhefka/dfuzzer/issues/20 so that this thread won't get lost.

Btw I was also thinking about introducing Packit and Testing Farm support in this repository.

@matusmarhefka now that I started fiddling with LDFLAGS I'm a bit concerned that I can accidentally break the package so I think it would be great to integrate Packit here. Could you install https://github.com/marketplace/packit-as-a-service? Once it's installed I'll add .packit.yaml to make sure the package is buildable. I tested it in evverx#1 and it seems to be working.

@thrix
Copy link
Collaborator

thrix commented Apr 19, 2022

@matusmarhefka btw we could create a generic tmt test for dfuzzer which users could use to easily fuzz tier dbus interfaces :) Adding them to RHEL, Fedora and CentOS Stream would be very simple. WDYT?

@thrix Sounds good, so your idea is to provide a generic tmt test in this repo which could be parametrized with bus name? Btw I was also thinking about introducing Packit and Testing Farm support in this repository.

Yes, and that test could be provided by this repo, a user could then run it via Testing Farm very easily. And it could also serve as a sanity test :) I will open an issue and provide a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

*** buffer overflow detected ***: terminated in df_print_process_info at at dfuzzer.c:937
5 participants