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

Infer interface parameter types and check for interface-template mis-declarations #206

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented May 10, 2021

  • Infer parameter types of interfaces and templates

    Infer the type of parameters of templates and interfaces.
    This can help find mis-declarations, see next commit.

    In the future this can be used for further checks, e.g. find
    discrepancies between the kind of a parameter and its documentation.

  • Add checks for template/interface mis-declarations

    Interfaces in the refpolicy should not:

    • declare anything (no side effects)
    • use prefix parameters

    Add one check to find interfaces that should be declared as a template
    and one check to find templates that can be declared as an interface.

    Refpolicy findings:

    qemu.if:            112: (S): Template qemu_role might be declared as an interface (S-012)
    wm.if:              142: (S): Interface wm_dbus_chat should be a template, due to parameter 0 (S-011)
    wm.if:              250: (S): Interface wm_write_pipes should be a template, due to parameter 0 (S-011)
    gnome.if:           673: (S): Interface gnome_dbus_chat_gkeyringd should be a template, due to parameter 0 (S-011)
    gnome.if:           741: (S): Interface gnome_stream_connect_gkeyringd should be a template, due to parameter 0 (S-011)
    userdomain.if:     1431: (S): Template userdom_security_admin_template might be declared as an interface (S-012)
    kismet.if:           18: (S): Template kismet_role might be declared as an interface (S-012)
    dbus.if:            193: (S): Interface dbus_connect_spec_session_bus should be a template, due to parameter 0 (S-011)
    dbus.if:            245: (S): Interface dbus_spec_session_bus_client should be a template, due to parameter 0 (S-011)
    dbus.if:            298: (S): Interface dbus_send_spec_session_bus should be a template, due to parameter 0 (S-011)
    dbus.if:            436: (S): Interface dbus_spec_session_domain should be a template, due to parameter 0 (S-011)
    rlogin.if:           32: (S): Template rlogin_read_home_content might be declared as an interface (S-012)
    git.if:              18: (S): Template git_role might be declared as an interface (S-012)
    Found the following issue counts:
    S-011: 8
    S-012: 5
    

    Closes: Suggest to change template to interface if appropriate #205

Infer the type of parameters of templates and interfaces.
This can help find mis-declarations, see next commit.

In the future this can be used for further checks, e.g. find
discrepancies between the kind of a parameter and its documentation.
cgzones added a commit to cgzones/refpolicy that referenced this pull request May 13, 2021
Following the guideline of interfaces not allowed to declare anything
and not use prefix parameters, declare interfaces doing so as templates.

Also declare templates not using those features and not calling
templates themselves as interfaces.

These changes originate from the discussion in
SELinuxProject/selint#205 and are found by
new proposed SELint checks at
SELinuxProject/selint#206.

Signed-off-by: Christian Göttsche <[email protected]>
Interfaces in the refpolicy should not:
  - declare anything (no side effects)
  - use prefix parameters

Add one check to find interfaces that should be declared as a template
and one check to find templates that can be declared as an interface.

Refpolicy findings:

qemu.if:            112: (S): Template qemu_role might be declared as an interface (S-012)
wm.if:              142: (S): Interface wm_dbus_chat should be a template, due to parameter 0 (S-011)
wm.if:              250: (S): Interface wm_write_pipes should be a template, due to parameter 0 (S-011)
gnome.if:           673: (S): Interface gnome_dbus_chat_gkeyringd should be a template, due to parameter 0 (S-011)
gnome.if:           741: (S): Interface gnome_stream_connect_gkeyringd should be a template, due to parameter 0 (S-011)
userdomain.if:     1431: (S): Template userdom_security_admin_template might be declared as an interface (S-012)
kismet.if:           18: (S): Template kismet_role might be declared as an interface (S-012)
dbus.if:            193: (S): Interface dbus_connect_spec_session_bus should be a template, due to parameter 0 (S-011)
dbus.if:            245: (S): Interface dbus_spec_session_bus_client should be a template, due to parameter 0 (S-011)
dbus.if:            298: (S): Interface dbus_send_spec_session_bus should be a template, due to parameter 0 (S-011)
dbus.if:            436: (S): Interface dbus_spec_session_domain should be a template, due to parameter 0 (S-011)
rlogin.if:           32: (S): Template rlogin_read_home_content might be declared as an interface (S-012)
git.if:              18: (S): Template git_role might be declared as an interface (S-012)
Found the following issue counts:
S-011: 8
S-012: 5

Closes: SELinuxProject#205
#include "maps.h"
#include "util.h"

/* Uses directly in the testsuite */
Copy link
Member

Choose a reason for hiding this comment

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

typo, uses -> used

}
}

static void infer_func(const char *name, enum name_flavor flavor, unsigned short id, void *visitor_data)
Copy link
Member

Choose a reason for hiding this comment

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

Can this (and infer_interface) get a comment explaining their usage? It's not clear to me from the names, and it's slow progress trying to infer from the implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, reading more, I think I have a rough idea of what these are both for based on how they're used. I still would like to have something with the functions to help orient anyone reading the code.

bool is_inferred = true;
for (int i = 0; i < TRAIT_MAX_PARAMETERS; ++i) {
if (if_trait->parameters[i] == PARAM_UNKNOWN) {
is_inferred = false;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a few lines of spaces got in this file (here and line 213 below)

name = name + 1;
}

if (dollar == name && *param_end == '\0') {
Copy link
Member

Choose a reason for hiding this comment

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

I may very well be missing something here, but I'm concerned about the situation where a parameter is incorrectly used in a way that would infer as two different parameters. Something like:

allow $1 foo_t:file read;
role $1 types foo_t;

This would be a compile error, but I think the logic here would end up setting the flavor of $1 to PARAM_ROLE, since the second line just overwrites the first? It seems like it would be valuable info to have to know if a parameter was being used inconsistently.

Then again, I could definitely be missing something, so feel free to let me know if this scenario is already handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, a subsequent usage of the same no yet final inferred parameter will completely override the current type.
Probably the inferred state shouldn't be a type of an enum but a bit-mask where its possible to store the information a parameter is used as a type-or-attribute, a role and a template string-part. So analysis checks can then create reports like "Parameter X has conflicting usages", "Interface foo uses templated parameter X (declare as template)".

0xC0ncord pushed a commit to 0xC0ncord/hardened-refpolicy that referenced this pull request Jul 20, 2021
Following the guideline of interfaces not allowed to declare anything
and not use prefix parameters, declare interfaces doing so as templates.

Also declare templates not using those features and not calling
templates themselves as interfaces.

These changes originate from the discussion in
SELinuxProject/selint#205 and are found by
new proposed SELint checks at
SELinuxProject/selint#206.

Signed-off-by: Christian Göttsche <[email protected]>
0xC0ncord pushed a commit to 0xC0ncord/hardened-refpolicy that referenced this pull request Jul 21, 2021
Following the guideline of interfaces not allowed to declare anything
and not use prefix parameters, declare interfaces doing so as templates.

Also declare templates not using those features and not calling
templates themselves as interfaces.

These changes originate from the discussion in
SELinuxProject/selint#205 and are found by
new proposed SELint checks at
SELinuxProject/selint#206.

Signed-off-by: Christian Göttsche <[email protected]>
perfinion pushed a commit to perfinion/hardened-refpolicy that referenced this pull request Sep 5, 2021
Following the guideline of interfaces not allowed to declare anything
and not use prefix parameters, declare interfaces doing so as templates.

Also declare templates not using those features and not calling
templates themselves as interfaces.

These changes originate from the discussion in
SELinuxProject/selint#205 and are found by
new proposed SELint checks at
SELinuxProject/selint#206.

Signed-off-by: Christian Göttsche <[email protected]>
Signed-off-by: Jason Zaman <[email protected]>
@cgzones cgzones marked this pull request as draft November 30, 2021 16:42
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.

Suggest to change template to interface if appropriate
2 participants