Differential D1216

Disallow ptrace on greeter and kcheckpass process on FreeBSD
ClosedPublic
Actions

Authored by tcberner on Mar 24 2016, 6:03 PM.

Details

Reviewers
rakuco
graesslin
Commits
R133:42ad07d79785: Disallow ptrace on greeter and kcheckpass process on FreeBSD
Summary

Use FreeBSDs procctl to disable gdb&Co from attaching to kscreenlocker and kcheckpass.

What is not so nice is that when CMake runs we get the output:

-- The following features have been enabled:
 * procctl-trace , Required for disallow ptrace on greeter and kcheckpass process
[...]
-- The following features have been disabled:
 * prctl-dumpable , Required for disallow ptrace on greeter and kcheckpass process

This should probably be unified.

Also, as this will probably crop up all over the place, would it maybe be sensible to define some enableTracing() and disableTracing() functions so that this change has not have to be applied everywhere?

Test Plan

Testing done:

% gdb --pid <pid of kscreenlocker with --testing>
Result
    successfully attached

# gdb --pid <pid of kscreenlocker>
Result
    successfully attached

% gdb --pid <pid of kscreenlocker>
Result
    Attaching to process <pid of kscreenlocker>
    ptrace: Operation not permitted.

Diff Detail

Repository
R133 KScreenLocker
Lint
Lint Skipped
Unit
Unit Tests Skipped
tcberner updated this revision to Diff 2945.Mar 24 2016, 6:03 PM
tcberner retitled this revision from to Disallow ptrace on greeter and kcheckpass process on FreeBSD .
tcberner updated this object.
tcberner edited the test plan for this revision. (Show Details)
tcberner added reviewers: graesslin, rakuco.
tcberner set the repository for this revision to R133 KScreenLocker.
Restricted Application added a project: Plasma. · View Herald TranscriptMar 24 2016, 6:03 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rakuco added inline comments.Mar 28 2016, 3:01 PM
CMakeLists.txt
25–30

I think you need to merge these two add_feature_info() calls: as-is, either one or the other will always be shown in the "disabled features" list, and I guess you only want this to happen if neither call/header is found. Perhaps something like this:

check_include_file(...)
check_symbol_exists(...)
# ...
if (HAVE_PR_SET_DUMPABLE OR HAVE_PROC_TRACE_CTL)
  set(CAN_DISABLE_PTRACE TRUE)
endif ()
add_feature_info("prctl/procctl tracing control"
                 CAN_DISABLE_PTRACE
                 "Required for disallowing ptrace on greeter and kcheckpass processes.")
greeter/main.cpp
67

There's an extra space before the if here and in a few other places.

140–141

The indentation here looks wrong.

kcheckpass/kcheckpass.c
339–340

The indentation looks wrong here too.

graesslin requested changes to this revision.Mar 29 2016, 6:43 AM
graesslin edited edge metadata.

I agree with rakuco: the two variables need to be merged into one add_feature_info.

Otherwise: thanks a lot for implementing it!

This revision now requires changes to proceed.Mar 29 2016, 6:43 AM
tcberner updated this revision to Diff 3009.Mar 29 2016, 4:40 PM
tcberner edited edge metadata.

The add_feature code is now unified, and the identations should be fixed too.

tcberner marked 4 inline comments as done.Mar 29 2016, 4:42 PM

Mark stuff done.

rakuco edited edge metadata.Mar 29 2016, 4:47 PM

Looks great to me now; I'll leave it to Martin to accept the diff.

CMakeLists.txt
34

While here, you could adjust the wording a little, like I suggested in my comment ("disallow" -> "disallowing", "the greeter and kcheckpass processes").

graesslin accepted this revision.Mar 30 2016, 6:00 AM
graesslin edited edge metadata.

Wonderful, thanks a lot!

We introduced similar things in:

I think that's it. KWallet we didn't introduce it as it's pointless and I'm not aware of any other application asking for passwords.

This revision is now accepted and ready to land.Mar 30 2016, 6:00 AM
Closed by commit R133:42ad07d79785: Disallow ptrace on greeter and kcheckpass process on FreeBSD (authored by tcberner). · Explain WhyMar 30 2016, 7:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
MCMakeLists.txt (3 lines)
Mconfig-kscreenlocker.h.cmake (2 lines)
Mgreeter/main.cpp (12 lines)
Mkcheckpass/kcheckpass.c (9 lines)

Diff 2945

View Options

CMakeLists.txt

Loading...
View Options

config-kscreenlocker.h.cmake

Loading...
View Options

greeter/main.cpp

Loading...
View Options

kcheckpass/kcheckpass.c

Loading...