On 04/09/2012 03:55 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws
internal server error when name contains '<', '>' or other special
characters.
The problem is, of course, proper escaping; not only in DNs but also in
ACIs. Right now we don't really do either.
This patch is just a simple workaround: disallow anything except
known-good characters. It's just names, so no functionality is lost.
All tickets for April are now taken, so unless a new one comes my way,
I'll take a dive into the code and fix it properly. This could take some
time and would mean somewhat larger changes.
Is there a reason you didn't use pattern/pattern_errmsg instead?
You'd need to change the regex as patterns use re.match rather than
re.search.
rob
Right, that makes more sense.
It changes API.txt though. Do I need to bump VERSION in this case?
Also, is there a reason pattern_errmsg is included in API.txt?
--
Petr³
From ece4a28d2b6dc3c35e7fbc6fc3ef80a063312846 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Fri, 6 Apr 2012 04:56:46 -0400
Subject: [PATCH] Limit permission and selfservice names to alphanumerics, -,
_, space
The DN and ACI code doesn't always escape special characters properly.
Rather than trying to fix it, this patch takes the easy way out and
enforces that the names are safe.
https://fedorahosted.org/freeipa/ticket/2585
---
API.txt | 26 +++++++++++++-------------
ipalib/plugins/permission.py | 4 ++++
ipalib/plugins/selfservice.py | 4 ++++
tests/test_xmlrpc/test_permission_plugin.py | 11 +++++++++++
tests/test_xmlrpc/test_selfservice_plugin.py | 13 +++++++++++++
5 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/API.txt b/API.txt
index c1b2ba05c40576abf6328f834eadb11e2d030b49..a49376817daf06b504e8148783e50091a62b6363 100644
--- a/API.txt
+++ b/API.txt
@@ -2039,7 +2039,7 @@ output: Output('result', <type 'bool'>, None)
output: Output('value', <type 'unicode'>, None)
command: permission_add
args: 1,12,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, required=True)
option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=True)
option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False)
option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
@@ -2057,25 +2057,25 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA
output: Output('value', <type 'unicode'>, None)
command: permission_add_member
args: 1,4,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('version?', exclude='webui')
option: Str('privilege*', alwaysask=True, cli_name='privileges', csv=True)
output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('failed', <type 'dict'>, None)
output: Output('completed', <type 'int'>, None)
command: permission_del
args: 1,1,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=True, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('continue', autofill=True, cli_name='continue', default=False)
output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
output: Output('result', <type 'dict'>, None)
output: Output('value', <type 'unicode'>, None)
command: permission_find
args: 1,14,4
arg: Str('criteria?', noextrawhitespace=False)
-option: Str('cn', attribute=True, autofill=False, cli_name='name', multivalue=False, primary_key=True, query=True, required=False)
+option: Str('cn', attribute=True, autofill=False, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=False)
option: Str('permissions', attribute=True, autofill=False, cli_name='permissions', csv=True, multivalue=True, query=True, required=False)
option: Str('attrs', attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=True, required=False)
option: StrEnum('type', attribute=True, autofill=False, cli_name='type', multivalue=False, query=True, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
@@ -2095,7 +2095,7 @@ output: Output('count', <type 'int'>, None)
output: Output('truncated', <type 'bool'>, None)
command: permission_mod
args: 1,15,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Str('permissions', attribute=True, autofill=False, cli_name='permissions', csv=True, multivalue=True, required=False)
option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False)
option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
@@ -2110,23 +2110,23 @@ option: Flag('rights', autofill=True, default=False)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('version?', exclude='webui')
-option: Str('rename', cli_name='rename', multivalue=False, primary_key=True, required=False)
+option: Str('rename', cli_name='rename', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, required=False)
output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('value', <type 'unicode'>, None)
command: permission_remove_member
args: 1,4,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('version?', exclude='webui')
option: Str('privilege*', alwaysask=True, cli_name='privileges', csv=True)
output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('failed', <type 'dict'>, None)
output: Output('completed', <type 'int'>, None)
command: permission_show
args: 1,4,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('rights', autofill=True, default=False)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -2437,7 +2437,7 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA
output: Output('value', <type 'unicode'>, None)
command: selfservice_add
args: 1,5,3
-arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, required=True)
option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=False)
option: Str('attrs', attribute=True, cli_name='attrs', csv=True, multivalue=True, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
@@ -2448,14 +2448,14 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA
output: Output('value', <type 'unicode'>, None)
command: selfservice_del
args: 1,0,3
-arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
output: Output('result', <type 'bool'>, None)
output: Output('value', <type 'unicode'>, None)
command: selfservice_find
args: 1,7,4
arg: Str('criteria?')
-option: Str('aciname', attribute=True, autofill=False, cli_name='name', multivalue=False, primary_key=True, query=True, required=False)
+option: Str('aciname', attribute=True, autofill=False, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=False)
option: Str('permissions', attribute=True, autofill=False, cli_name='permissions', csv=True, multivalue=True, query=True, required=False)
option: Str('attrs', attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=True, required=False)
option: Flag('pkey_only?', autofill=True, default=False)
@@ -2468,7 +2468,7 @@ output: Output('count', <type 'int'>, None)
output: Output('truncated', <type 'bool'>, None)
command: selfservice_mod
args: 1,5,3
-arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Str('permissions', attribute=True, autofill=False, cli_name='permissions', csv=True, multivalue=True, required=False)
option: Str('attrs', attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, required=False)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
@@ -2479,7 +2479,7 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA
output: Output('value', <type 'unicode'>, None)
command: selfservice_show
args: 1,3,3
-arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('aciname', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('version?', exclude='webui')
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index ce2536d9921ede73d2c26468f5d99609552e1881..9b669d9f57e81e885bd080703ba6c405395f6608 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -18,6 +18,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import copy
+
from ipalib.plugins.baseldap import *
from ipalib import api, _, ngettext
from ipalib import Flag, Str, StrEnum
@@ -92,6 +93,7 @@
dn_ipaconfig = str(DN('cn=ipaconfig,cn=etc,%s' % api.env.basedn))
+
def check_attrs(attrs, type):
# Trying to delete attributes - no need for validation
if attrs is None:
@@ -154,6 +156,8 @@ class permission(LDAPObject):
cli_name='name',
label=_('Permission name'),
primary_key=True,
+ pattern='^[-_ a-zA-Z0-9]+$',
+ pattern_errmsg="May only contain letters, numbers, -, _, and space",
),
Str('permissions+',
cli_name='permissions',
diff --git a/ipalib/plugins/selfservice.py b/ipalib/plugins/selfservice.py
index 6f843d469be2e5c29fb7587fcef98069b839eec5..a60475b7cc7b109fdebdfecdbf71408d6041a0ba 100644
--- a/ipalib/plugins/selfservice.py
+++ b/ipalib/plugins/selfservice.py
@@ -18,6 +18,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import copy
+
from ipalib import api, _, ngettext
from ipalib import Flag, Str
from ipalib.request import context
@@ -60,6 +61,7 @@
),
)
+
class selfservice(Object):
"""
Selfservice object.
@@ -77,6 +79,8 @@ class selfservice(Object):
label=_('Self-service name'),
doc=_('Self-service name'),
primary_key=True,
+ pattern='^[-_ a-zA-Z0-9]+$',
+ pattern_errmsg="May only contain letters, numbers, -, _, and space",
),
Str('permissions*',
cli_name='permissions',
diff --git a/tests/test_xmlrpc/test_permission_plugin.py b/tests/test_xmlrpc/test_permission_plugin.py
index 29ae6d3713828ed0139cb18a967614fd0da9ddf5..d730399c16fe23720d96ea5c1309f3a5ca0bc79a 100644
--- a/tests/test_xmlrpc/test_permission_plugin.py
+++ b/tests/test_xmlrpc/test_permission_plugin.py
@@ -45,6 +45,8 @@
privilege1_dn = DN(('cn',privilege1),
api.env.container_privilege,api.env.basedn)
+invalid_permission1 = u'bad;perm'
+
class test_permission(Declarative):
@@ -719,5 +721,14 @@ class test_permission(Declarative):
),
),
+ dict(
+ desc='Try to create invalid %r' % invalid_permission1,
+ command=('permission_add', [invalid_permission1], dict(
+ type=u'user',
+ permissions=u'write',
+ )),
+ expected=errors.ValidationError(name='name',
+ error='May only contain letters, numbers, -, _, and space'),
+ ),
]
diff --git a/tests/test_xmlrpc/test_selfservice_plugin.py b/tests/test_xmlrpc/test_selfservice_plugin.py
index e1c61f85348e64feb4db616fe9ef7bbfe765f478..e60eb5d52373250d0f62e178e36f5fa602a0c311 100644
--- a/tests/test_xmlrpc/test_selfservice_plugin.py
+++ b/tests/test_xmlrpc/test_selfservice_plugin.py
@@ -26,6 +26,7 @@
from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid
selfservice1 = u'testself'
+invalid_selfservice1 = u'bad+name'
class test_selfservice(Declarative):
@@ -273,4 +274,16 @@ class test_selfservice(Declarative):
)
),
+ dict(
+ desc='Create invalid %r' % invalid_selfservice1,
+ command=(
+ 'selfservice_add', [invalid_selfservice1], dict(
+ attrs=[u'street', u'c', u'l', u'st', u'postalcode'],
+ permissions=u'write',
+ )
+ ),
+ expected=errors.ValidationError(name='name',
+ error='May only contain letters, numbers, -, _, and space'),
+ ),
+
]
--
1.7.7.6
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
