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

GID, PID, and UID targeting #15

Merged
merged 7 commits into from
Jun 11, 2019
Merged

GID, PID, and UID targeting #15

merged 7 commits into from
Jun 11, 2019

Conversation

hmwildermuth
Copy link
Contributor

@hmwildermuth hmwildermuth commented Jun 7, 2019

Can target a single GID, single UID, and/or a single PID.
A partial solution to #9

@@ -3,3 +3,8 @@ unsigned int krf_personality = 0x001c;
unsigned int krf_probability = 1000;
unsigned int krf_targeted_uid = 1002;
unsigned int krf_log_faults = 0;
unsigned int krf_pid_target = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a solid first approach, but I think we can structure this better for future extensibility. Instead of continuing to add individual variables + an option mask, we could have a structure in config.h. A very rough sketch:

#define KRF_T_MODE_MAX 31
#define KRF_T_MODE_MAX_MASK (1 << KRF_T_MODE_MAX)

typedef enum {
  KRF_T_MODE_PERSONALITY = (1 << 0);
  KRF_T_MODE_PID = (1 << 1);
  // ...
} krf_target_mode_mask_t;

typedef struct {
  krf_target_mode_mask_t mode_mask;
  unsigned int target_data[KRF_T_MODE_MAX];
} krf_target_options_t;

extern krf_target_options_t krf_target_options;

And rewrite the KRF_TARGETED macro into an (inlinable?) function that tests krf_target_options instead.

Similarly for the procfs sources/outputs: we can just have a single file that reads >=2 unsigned ints: the first is the mask, followed by each mask's parameter in ascending-bit order. Setting the mask to 0 disables all targeting and flushes any parameters from target_data.

Copy link
Member

@woodruffw woodruffw Jun 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also make target configuration way simpler: instead of allowing the user to configure >1 fields of target_data in one shot, we could require them to write to the procfs file once for each mask + datum. That's what they currently have to do for configuring syscalls, and that's a reasonable approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some problem with:

unsigned int target_data[KRF_T_MODE_MAX];

14:15 <disconnect3d> << (1<<31)
14:15 <cxx> -2147483648
14:16 <disconnect3d> unsigned int t[1<<31];
14:16 <cxx> error: narrowing conversion of '-2147483648' from 'int' to 'long unsigned int'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KRF_T_MODE_MAX would just be 31, so it should just be unsigned int[31]. It's KRF_T_MODE_MAX_MASK that would be 1 << 31 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, my bad :D kk :)

@@ -28,7 +28,45 @@

#define KRF_DEFINE(sys) asmlinkage krf_sys_##sys

#define KRF_TARGETED() ((current->personality & krf_personality) != 0)
#define KRF_TARGETED() \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go ahead and make this an inline function, below the externs:

static __always_inline int KRF_TARGETED(void) {
  // ...
}

Might also be worth updating module/codegen/codegen so that we can make it krf_targeted instead of all caps.

src/module/krf.c Outdated
char buf[KRF_PROCFS_MAX_SIZE + 1] = {0};
size_t buflen = 0;
size_t offset = 0;
size_t current_mode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use unsigned int for current_mode here, just for consistency with the actual structure.

@woodruffw
Copy link
Member

Some small nits, but otherwise looks really good!

@woodruffw woodruffw merged commit 8d49c71 into trailofbits:master Jun 11, 2019
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.

3 participants