Bug 663445

Summary: Hardcoded SELinux constants in pam_selinux
Product: [Fedora] Fedora Reporter: Tomas Mraz <tmraz>
Component: pamAssignee: Tomas Mraz <tmraz>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: dwalsh, eparis, mgrepl, mmaslano, pertusus, tmraz
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: pam-1.1.4-1.fc15 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 663193 Environment:
Last Closed: 2011-11-24 14:16:33 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Tomas Mraz 2010-12-15 20:08:47 UTC
Similarly to cronie pam_selinux contains hardcoded parameter values for security_compute_av() call.

--- Additional comment from tmraz on 2010-12-15 11:11:36 EST ---

OK if the values are fixed for each boot it makes sense to cache them even if the lookup is cheap, i'll do that.

I'm curious whether other code doesn't contain the hardcoded values as well - f.e. pam_selinux or openssh.

--- Additional comment from dwalsh on 2010-12-15 12:53:01 EST ---

Theoretically, although, this is only going to happen when an app is a policy enforcer.  openssh might do something around checking whether the range is "contained" within the system range.

-----

We have the following code in pam_selinux:

static int mls_range_allowed(pam_handle_t *pamh, security_context_t src, security_context_t dst, int debug)
{
  struct av_decision avd;
  int retval;
  unsigned int bit = CONTEXT__CONTAINS;
  context_t src_context = context_new (src);
  context_t dst_context = context_new (dst);
  context_range_set(dst_context, context_range_get(src_context));
  if (debug)
    pam_syslog(pamh, LOG_NOTICE, "Checking if %s mls range valid for  %s", dst, context_str(dst_context));

  retval = security_compute_av(context_str(dst_context), dst, SECCLASS_CONTEXT, bit, &avd);
  context_free(src_context);
  context_free(dst_context);
  if (retval || ((bit & avd.allowed) != bit))
    return 0;
  return 1;
}

How it should be fixed? I suppose the SECCLASS_CONTEXT and CONTEXT__CONTAINS should be un-hardcoded.

Comment 1 Daniel Walsh 2010-12-15 20:22:02 UTC
	security_class_t tclass = string_to_security_class("context");
	if (!tclass) return 0;
	access_vector_t bit = string_to_av_perm(tclass, "contains");
	if (!bit) return 0;

...

  retval = security_compute_av(context_str(dst_context), dst, tclass,
bit, &avd);

Comment 2 Eric Paris 2010-12-16 15:17:20 UTC
I told some people some lies earlier and need to make sure they are corrected.  these values can change on selinux policy update.  So you need to check the string_to_* functions before every call to security_compute_av.  The results should not be cached.