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

Certificate attributes are not sanitized prior to ldap search #5135

Closed
sssd-bot opened this issue May 2, 2020 · 3 comments
Closed

Certificate attributes are not sanitized prior to ldap search #5135

sssd-bot opened this issue May 2, 2020 · 3 comments
Assignees
Labels
Bugzilla Closed: Fixed Issue was closed as fixed.

Comments

@sssd-bot
Copy link

sssd-bot commented May 2, 2020

Cloned from Pagure issue: https://pagure.io/SSSD/sssd/issue/4180


I'm having an issue getting a maprule to work properly. It looks like sssd isn't sanitizing certificate attributes prior to issuing the ldap search. In this example, parentheses in the Subject DN are causing an ldap search error "Bad search filter." At the end of the example, I show the Bad search filter result from sssd and a working ldapsearch with backslash-escaped parentheses.

Additionally, altSecurityIdentities in LDAP stores a space between UID (OID...) and CN. AD is able to find and match the certificate to user when the Certificate uses a + to separate these values. I don't know if this is a standard transformation or if sssd could support a user-configurable transformation like !ad for certain character adjustments?

The smartcard is an US Government HSPD-12/PIV card.

certificate (sssctl cert-show):

Issuer: OU=Entrust Managed Services SSP CA,OU=Certification Authorities,O=Entrust,C=US
Subject: UID=1337+CN=JOHN GALT (Affiliate),OU=Department of Authentication,O=U.S. Government,C=US

SAN type: ntPrincipalName
  subject_nt_principal=1337@FEDIDCARD.GOV

LDAP:

altSecurityIdentities: X509:<I>C=US,O=Entrust,OU=Certification Authorities,OU=Entrust Managed Services SSP CA<S>C=US,O=U.S. Government,OU=Department of Authentication,OID.0.9.2342.19200300.100.1.1=1337 CN=JOHN GALT (Affiliate)

sssd.conf certmap:

[certmap/lol.gov/smartcard]
matchrule = <EKU>msScLogin<SAN:ntPrincipalName>^[0-9]+@FEDIDCARD\.GOV$
maprule = (altSecurityIdentities=X509:<I>{issuer_dn!ad}<S>{subject_dn!ad})
domains = lol.gov

sdap_attrs_add_ldap_attr:

Adding altSecurityIdentities [X509:<I>C=US,O=Entrust,OU=Certification\20Authorities,OU=Entrust\20Managed\20Services\20SSP\20CA<S>C=US,O=U.S.\20Government,OU=Department\20of\20Authentication,OID.0.9.2342.19200300.100.1.1=1337\20CN=JOHN\20GALT\20\28Affiliate\29] to attributes of [galt@lol.gov].

sss-certmap ldap search:

[sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(&(altSecurityIdentities=X509:<I>C=US,O=Entrust,OU=Certification Authorities,OU=Entrust Managed Services SSP CA<S>C=US,O=U.S. Government,OU=Department of Authentication,OID.0.9.2342.19200300.100.1.1=1337+CN=JOHN GALT (Affiliate))(objectclass=posixAccount)(uid=*)(&(uidNumber=*)(!(uidNumber=0))))(localAuthEnabled=TRUE))] [ou=linux,dc=lol,dc=gov].
[sdap_get_generic_ext_step] (0x0080): ldap_search_ext failed: Bad search filter

ldapsearch (working):

ldapsearch -x -b ou=linux,dc=lol,dc=gov '(&(&(altSecurityIdentities=X509:<I>C=US,O=Entrust,OU=Certification Authorities,OU=Entrust Managed Services SSP CA<S>C=US,O=U.S. Government,OU=Department of Authentication,OID.0.9.2342.19200300.100.1.1=1337 CN=JOHN GALT \(Affiliate\))(objectclass=posixAccount)(uid=*)(&(uidNumber=*)(!(uidNumber=0))))(localAuthEnabled=TRUE))'

Comments


Comment from sbose at 2020-04-29 10:43:30

Hi,

thank you for the report, this issue was reported at https://bugzilla.redhat.com/show_bug.cgi?id=1780404 (might be restricted) as well. I already had a patch in my tree but forgot to create a pull-request, it is #1036 now.

If you can tell me on which platform you are using SSSD I can try to prepare a test build with the fix.

bye,
Sumit


Comment from sbose at 2020-04-29 10:43:43

PR: #1036


Comment from sbose at 2020-04-29 10:43:57

Metadata Update from @sbose:

  • Custom field patch adjusted to on

Comment from sbose at 2020-04-29 10:48:05

Metadata Update from @sbose:


Comment from sbose at 2020-04-29 10:48:06

Issue linked to Bugzilla: Bug 1780404


Comment from sbose at 2020-04-29 10:48:40

Metadata Update from @sbose:

  • Issue assigned to sbose

Comment from darkcipher at 2020-04-29 13:54:44

Hi,
...
If you can tell me on which platform you are using SSSD I can try to prepare a test build with the fix.

Hello. I'm on Ubuntu 20.04.


Comment from darkcipher at 2020-04-29 14:05:23

It looks like your pull request will handle the sanitization. Any advise on the "+" issue? Microsoft AD is able to match. Could a s/+/ / be a part of the !ad filter?


Comment from sbose at 2020-04-29 17:04:22

Hi,

yes, in general this is possible but I'm currently searching for some documentation which explains this behavior.

It looks your certificate has a multi-valued RDN which is allowed for PIV cards as described in https://www.idmanagement.gov/wp-content/uploads/sites/1171/uploads/Common-Policy-Framework.pdf (search for 'multi-value').

According to https://tools.ietf.org/html/rfc4514 the components of a multi-valued RDN should be concatenated with the reserved character '+'.

What I'm currently missing is an explanation why AD does not use the '+' here but a space.

Btw, according to https://piv.idmanagement.gov/networkconfig/accounts/ the 'CN' part should come first followed by the 'OID' part which is different in your case. But according to https://permalink.lanl.gov/object/tr?what=info:lanl-repo/lareport/LA-UR-16-21404 section 4.1.1 both versions might be used.

bye,
Sumit


Comment from darkcipher at 2020-04-29 17:37:06

Yes, it's a multi-valued RDN (or Multi AVA) as defined in RFC4514 Section 2. Microsoft says they "support" RFC4514 Section 2 by defining MAX=1 MS-ADTS Ref. To me, this means they don't. I think they may just concatenate the multi-values with a space separator. The LDAP servers I'm using have the two values for altSecurityIdentities X509:... one with UID followed by CN, another with CN followed by UID, since LDAP doesn't guarantee order ... in LDAP's world, they are attributes, in MS-AD, they only support one, so concatenate.

altSecurityIdentities: X509:<I>C=US,O=Entrust,OU=Certification Authorities,OU=Entrust Managed Services SSP CA<S>C=US,O=U.S. Government,OU=Department of Authentication,OID.0.9.2342.19200300.100.1.1=1337 CN=JOHN GALT (Affiliate)
altSecurityIdentities: X509:<I>C=US,O=Entrust,OU=Certification Authorities,OU=Entrust Managed Services SSP CA<S>C=US,O=U.S. Government,OU=Department of Authentication,CN=JOHN GALT (Affiliate) OID.0.9.2342.19200300.100.1.1=1337
@jsf9k
Copy link

jsf9k commented Jul 16, 2020

I am running into this same issue, also having to do with parentheses in CNs when dealing with U.S. government smart cards. I'm going to have to configure FreeIPA to match on full certificates for now and revert back to using certmap data once this PR is merged.

jsf9k added a commit to cisagov/ansible-role-freeipa-server that referenced this issue Jul 17, 2020
There is currently a bug in sssd where it does not properly escape
parentheses in the certificate subject when performing an LDAP query.
See these links for more about this bug:
* SSSD/sssd#5135
* SSSD/sssd#1036

This bug inhibits us from matching on user certmap data in the case of
contractors, whose CNs contain the text "(affiliate)".  Hence we
require two certmap rules: one for certificates whose CNs contain
parentheses (e.g. contractor certificates) and therefore must match on
the full certificate, and one for certificates whose CNs do not
contain parentheses (e.g. fed certificates) and therefore can match on
user certmap data.  It is preferable to match on user certmap data,
since it should change less often than the full certificate.

Once the sssd pull request mentioned above is approved, merged, and
appears in a release, we should be able to use a single certmap rule
for all users that leverages the user certmap data.
@pbrezina
Copy link
Member

  • master
    • a2b9a84 - certmap: sanitize LDAP search filter

sumit-bose added a commit to sumit-bose/sssd that referenced this issue Jan 6, 2023
The sss_certmap_get_search_filter() will now sanitize the values read
from the certificates before adding them to a search filter. To be able
to get the plain values as well sss_certmap_expand_mapping_rule() is
added.

Resolves:
SSSD#5135

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
(cherry picked from commit a2b9a84)
alexey-tikhonov pushed a commit that referenced this issue Jan 6, 2023
The sss_certmap_get_search_filter() will now sanitize the values read
from the certificates before adding them to a search filter. To be able
to get the plain values as well sss_certmap_expand_mapping_rule() is
added.

Resolves:
#5135

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
(cherry picked from commit a2b9a84)

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com>
@alexey-tikhonov
Copy link
Member

Pushed PR: #6515

  • sssd-1-16
    • 918fb32 - certmap: sanitize LDAP search filter
    • a37a66b - certmap: add sss_certmap_display_cert_content()

@alexey-tikhonov alexey-tikhonov added the Closed: Fixed Issue was closed as fixed. label Jan 6, 2023
etrunko pushed a commit to etrunko/sssd that referenced this issue Nov 16, 2023
The sss_certmap_get_search_filter() will now sanitize the values read
from the certificates before adding them to a search filter. To be able
to get the plain values as well sss_certmap_expand_mapping_rule() is
added.

Resolves:
SSSD#5135

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
(cherry picked from commit a2b9a84)

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com>
etrunko pushed a commit to etrunko/sssd that referenced this issue Nov 16, 2023
The sss_certmap_get_search_filter() will now sanitize the values read
from the certificates before adding them to a search filter. To be able
to get the plain values as well sss_certmap_expand_mapping_rule() is
added.

Resolves:
SSSD#5135

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
(cherry picked from commit a2b9a84)

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugzilla Closed: Fixed Issue was closed as fixed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants