* [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name
@ 2012-12-14 13:40 Suresh Jayaraman
2012-12-17 15:15 ` J. Bruce Fields
2012-12-17 21:45 ` Steve Dickson
0 siblings, 2 replies; 6+ messages in thread
From: Suresh Jayaraman @ 2012-12-14 13:40 UTC (permalink / raw)
To: steved; +Cc: J. Bruce Fields, linux-nfs
The validateascii() check in imconv() maps NFSv4 domain names with non-ASCII
characters to 'nobody'. In setups where Active directory or LDAP is used this
causes names with UTF-8 characters to being mapped to 'nobody' because of this
check.
As Bruce Fields puts it:
"idmapd doesn't seem like the right place to enforce restrictions on names.
Once the system has allowed a name it's too late to be complaining about it
here."
Replace the validateascii() call in imconv() with a check for null-termination
just to be extra-careful and remove the validateascii() function itself
as the only user of that function is being removed by this patch.
Signed-off-by: Suresh Jayaraman <sjayaraman@suse.com>
Cc: J. Bruce Fields <bfields@fieldses.org>
---
utils/idmapd/idmapd.c | 28 +++++-----------------------
1 file changed, 5 insertions(+), 23 deletions(-)
diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index e80efb4..9d66225 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -145,7 +145,6 @@ static void svrreopen(int, short, void *);
static int nfsopen(struct idmap_client *);
static void nfscb(int, short, void *);
static void nfsdcb(int, short, void *);
-static int validateascii(char *, u_int32_t);
static int addfield(char **, ssize_t *, char *);
static int getfield(char **, char *, size_t);
@@ -642,6 +641,8 @@ out:
static void
imconv(struct idmap_client *ic, struct idmap_msg *im)
{
+ u_int32_t len;
+
switch (im->im_conv) {
case IDMAP_CONV_IDTONAME:
idtonameres(im);
@@ -652,10 +653,10 @@ imconv(struct idmap_client *ic, struct idmap_msg *im)
im->im_id, im->im_name);
break;
case IDMAP_CONV_NAMETOID:
- if (validateascii(im->im_name, sizeof(im->im_name)) == -1) {
- im->im_status |= IDMAP_STATUS_INVALIDMSG;
+ len = strnlen(im->im_name, IDMAP_NAMESZ - 1);
+ /* Check for NULL termination just to be careful */
+ if (im->im_name[len+1] != '\0')
return;
- }
nametoidres(im);
if (verbose > 1)
xlog_warn("%s %s: (%s) name \"%s\" -> id \"%d\"",
@@ -855,25 +856,6 @@ nametoidres(struct idmap_msg *im)
}
static int
-validateascii(char *string, u_int32_t len)
-{
- u_int32_t i;
-
- for (i = 0; i < len; i++) {
- if (string[i] == '\0')
- break;
-
- if (string[i] & 0x80)
- return (-1);
- }
-
- if ((i >= len) || string[i] != '\0')
- return (-1);
-
- return (i + 1);
-}
-
-static int
addfield(char **bpp, ssize_t *bsizp, char *fld)
{
char ch, *bp = *bpp;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name
2012-12-14 13:40 [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name Suresh Jayaraman
@ 2012-12-17 15:15 ` J. Bruce Fields
2012-12-17 21:45 ` Steve Dickson
1 sibling, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2012-12-17 15:15 UTC (permalink / raw)
To: Suresh Jayaraman; +Cc: steved, linux-nfs
On Fri, Dec 14, 2012 at 07:10:14PM +0530, Suresh Jayaraman wrote:
> The validateascii() check in imconv() maps NFSv4 domain names with non-ASCII
> characters to 'nobody'. In setups where Active directory or LDAP is used this
> causes names with UTF-8 characters to being mapped to 'nobody' because of this
> check.
>
> As Bruce Fields puts it:
>
> "idmapd doesn't seem like the right place to enforce restrictions on names.
> Once the system has allowed a name it's too late to be complaining about it
> here."
>
> Replace the validateascii() call in imconv() with a check for null-termination
> just to be extra-careful and remove the validateascii() function itself
> as the only user of that function is being removed by this patch.
Seems OK, thanks.--b.
>
>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.com>
> Cc: J. Bruce Fields <bfields@fieldses.org>
> ---
> utils/idmapd/idmapd.c | 28 +++++-----------------------
> 1 file changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index e80efb4..9d66225 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -145,7 +145,6 @@ static void svrreopen(int, short, void *);
> static int nfsopen(struct idmap_client *);
> static void nfscb(int, short, void *);
> static void nfsdcb(int, short, void *);
> -static int validateascii(char *, u_int32_t);
> static int addfield(char **, ssize_t *, char *);
> static int getfield(char **, char *, size_t);
>
> @@ -642,6 +641,8 @@ out:
> static void
> imconv(struct idmap_client *ic, struct idmap_msg *im)
> {
> + u_int32_t len;
> +
> switch (im->im_conv) {
> case IDMAP_CONV_IDTONAME:
> idtonameres(im);
> @@ -652,10 +653,10 @@ imconv(struct idmap_client *ic, struct idmap_msg *im)
> im->im_id, im->im_name);
> break;
> case IDMAP_CONV_NAMETOID:
> - if (validateascii(im->im_name, sizeof(im->im_name)) == -1) {
> - im->im_status |= IDMAP_STATUS_INVALIDMSG;
> + len = strnlen(im->im_name, IDMAP_NAMESZ - 1);
> + /* Check for NULL termination just to be careful */
> + if (im->im_name[len+1] != '\0')
> return;
> - }
> nametoidres(im);
> if (verbose > 1)
> xlog_warn("%s %s: (%s) name \"%s\" -> id \"%d\"",
> @@ -855,25 +856,6 @@ nametoidres(struct idmap_msg *im)
> }
>
> static int
> -validateascii(char *string, u_int32_t len)
> -{
> - u_int32_t i;
> -
> - for (i = 0; i < len; i++) {
> - if (string[i] == '\0')
> - break;
> -
> - if (string[i] & 0x80)
> - return (-1);
> - }
> -
> - if ((i >= len) || string[i] != '\0')
> - return (-1);
> -
> - return (i + 1);
> -}
> -
> -static int
> addfield(char **bpp, ssize_t *bsizp, char *fld)
> {
> char ch, *bp = *bpp;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name
2012-12-14 13:40 [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name Suresh Jayaraman
2012-12-17 15:15 ` J. Bruce Fields
@ 2012-12-17 21:45 ` Steve Dickson
1 sibling, 0 replies; 6+ messages in thread
From: Steve Dickson @ 2012-12-17 21:45 UTC (permalink / raw)
To: Suresh Jayaraman; +Cc: J. Bruce Fields, linux-nfs
On 14/12/12 08:40, Suresh Jayaraman wrote:
> The validateascii() check in imconv() maps NFSv4 domain names with non-ASCII
> characters to 'nobody'. In setups where Active directory or LDAP is used this
> causes names with UTF-8 characters to being mapped to 'nobody' because of this
> check.
>
> As Bruce Fields puts it:
>
> "idmapd doesn't seem like the right place to enforce restrictions on names.
> Once the system has allowed a name it's too late to be complaining about it
> here."
>
> Replace the validateascii() call in imconv() with a check for null-termination
> just to be extra-careful and remove the validateascii() function itself
> as the only user of that function is being removed by this patch.
>
>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.com>
> Cc: J. Bruce Fields <bfields@fieldses.org>
Committed...
steved.
> ---
> utils/idmapd/idmapd.c | 28 +++++-----------------------
> 1 file changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
> index e80efb4..9d66225 100644
> --- a/utils/idmapd/idmapd.c
> +++ b/utils/idmapd/idmapd.c
> @@ -145,7 +145,6 @@ static void svrreopen(int, short, void *);
> static int nfsopen(struct idmap_client *);
> static void nfscb(int, short, void *);
> static void nfsdcb(int, short, void *);
> -static int validateascii(char *, u_int32_t);
> static int addfield(char **, ssize_t *, char *);
> static int getfield(char **, char *, size_t);
>
> @@ -642,6 +641,8 @@ out:
> static void
> imconv(struct idmap_client *ic, struct idmap_msg *im)
> {
> + u_int32_t len;
> +
> switch (im->im_conv) {
> case IDMAP_CONV_IDTONAME:
> idtonameres(im);
> @@ -652,10 +653,10 @@ imconv(struct idmap_client *ic, struct idmap_msg *im)
> im->im_id, im->im_name);
> break;
> case IDMAP_CONV_NAMETOID:
> - if (validateascii(im->im_name, sizeof(im->im_name)) == -1) {
> - im->im_status |= IDMAP_STATUS_INVALIDMSG;
> + len = strnlen(im->im_name, IDMAP_NAMESZ - 1);
> + /* Check for NULL termination just to be careful */
> + if (im->im_name[len+1] != '\0')
> return;
> - }
> nametoidres(im);
> if (verbose > 1)
> xlog_warn("%s %s: (%s) name \"%s\" -> id \"%d\"",
> @@ -855,25 +856,6 @@ nametoidres(struct idmap_msg *im)
> }
>
> static int
> -validateascii(char *string, u_int32_t len)
> -{
> - u_int32_t i;
> -
> - for (i = 0; i < len; i++) {
> - if (string[i] == '\0')
> - break;
> -
> - if (string[i] & 0x80)
> - return (-1);
> - }
> -
> - if ((i >= len) || string[i] != '\0')
> - return (-1);
> -
> - return (i + 1);
> -}
> -
> -static int
> addfield(char **bpp, ssize_t *bsizp, char *fld)
> {
> char ch, *bp = *bpp;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name
@ 2012-12-13 16:29 Suresh Jayaraman
2012-12-13 16:50 ` J. Bruce Fields
0 siblings, 1 reply; 6+ messages in thread
From: Suresh Jayaraman @ 2012-12-13 16:29 UTC (permalink / raw)
To: steved, linux-nfs; +Cc: J. Bruce Fields
The validateascii() check in imconv() maps NFSv4 domain names with non-ASCII
characters to 'nobody'. In setups where Active directory or LDAP is used this
causes names with UTF-8 characters to be mapped to 'nobody' because of this
check.
As Bruce Fields puts it:
"idmapd doesn't seem like the right place to enforce restrictions on names.
Once the system has allowed a name it's too late to be complaining about it
here."
Remove the check from imconv() and remove the validateascii() function itself
as the only user of that function is being removed by this patch.
Signed-off-by: Suresh Jayaraman <sjayaraman@suse.com>
Cc: J. Bruce Fields <bfields@fieldses.org>
---
utils/idmapd/idmapd.c | 24 ------------------------
1 file changed, 24 deletions(-)
diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index e80efb4..fe47dac 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -145,7 +145,6 @@ static void svrreopen(int, short, void *);
static int nfsopen(struct idmap_client *);
static void nfscb(int, short, void *);
static void nfsdcb(int, short, void *);
-static int validateascii(char *, u_int32_t);
static int addfield(char **, ssize_t *, char *);
static int getfield(char **, char *, size_t);
@@ -652,10 +651,6 @@ imconv(struct idmap_client *ic, struct idmap_msg *im)
im->im_id, im->im_name);
break;
case IDMAP_CONV_NAMETOID:
- if (validateascii(im->im_name, sizeof(im->im_name)) == -1) {
- im->im_status |= IDMAP_STATUS_INVALIDMSG;
- return;
- }
nametoidres(im);
if (verbose > 1)
xlog_warn("%s %s: (%s) name \"%s\" -> id \"%d\"",
@@ -855,25 +850,6 @@ nametoidres(struct idmap_msg *im)
}
static int
-validateascii(char *string, u_int32_t len)
-{
- u_int32_t i;
-
- for (i = 0; i < len; i++) {
- if (string[i] == '\0')
- break;
-
- if (string[i] & 0x80)
- return (-1);
- }
-
- if ((i >= len) || string[i] != '\0')
- return (-1);
-
- return (i + 1);
-}
-
-static int
addfield(char **bpp, ssize_t *bsizp, char *fld)
{
char ch, *bp = *bpp;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name
2012-12-13 16:29 Suresh Jayaraman
@ 2012-12-13 16:50 ` J. Bruce Fields
2012-12-14 13:37 ` Suresh Jayaraman
0 siblings, 1 reply; 6+ messages in thread
From: J. Bruce Fields @ 2012-12-13 16:50 UTC (permalink / raw)
To: Suresh Jayaraman; +Cc: steved, linux-nfs
On Thu, Dec 13, 2012 at 09:59:08PM +0530, Suresh Jayaraman wrote:
>
> The validateascii() check in imconv() maps NFSv4 domain names with non-ASCII
> characters to 'nobody'. In setups where Active directory or LDAP is used this
> causes names with UTF-8 characters to be mapped to 'nobody' because of this
> check.
>
> As Bruce Fields puts it:
>
> "idmapd doesn't seem like the right place to enforce restrictions on names.
> Once the system has allowed a name it's too late to be complaining about it
> here."
>
> Remove the check from imconv() and remove the validateascii() function itself
> as the only user of that function is being removed by this patch.
Thanks, seem fine. The only other thing I notice is that
validateascii() also checks (in a slightly strange way) for null
termination of the string, and it's the only place in idmapd that does.
But I think it'd be a kernel bug to pass up a non-terminated string
here, so skipping that check is fine too.
Possibly worth a comment, or a check just for null-termination if you
want to be extra-careful.
--b.
> @@ -652,10 +651,6 @@ imconv(struct idmap_client *ic, struct idmap_msg *im)
> im->im_id, im->im_name);
> break;
> case IDMAP_CONV_NAMETOID:
> - if (validateascii(im->im_name, sizeof(im->im_name)) == -1) {
> - im->im_status |= IDMAP_STATUS_INVALIDMSG;
> - return;
> - }
> nametoidres(im);
> if (verbose > 1)
> xlog_warn("%s %s: (%s) name \"%s\" -> id \"%d\"",
> @@ -855,25 +850,6 @@ nametoidres(struct idmap_msg *im)
> }
>
> static int
> -validateascii(char *string, u_int32_t len)
> -{
> - u_int32_t i;
> -
> - for (i = 0; i < len; i++) {
> - if (string[i] == '\0')
> - break;
> -
> - if (string[i] & 0x80)
> - return (-1);
> - }
> -
> - if ((i >= len) || string[i] != '\0')
> - return (-1);
> -
> - return (i + 1);
> -}
> -
> -static int
> addfield(char **bpp, ssize_t *bsizp, char *fld)
> {
> char ch, *bp = *bpp;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name
2012-12-13 16:50 ` J. Bruce Fields
@ 2012-12-14 13:37 ` Suresh Jayaraman
0 siblings, 0 replies; 6+ messages in thread
From: Suresh Jayaraman @ 2012-12-14 13:37 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: steved, linux-nfs
On 12/13/2012 10:20 PM, J. Bruce Fields wrote:
> On Thu, Dec 13, 2012 at 09:59:08PM +0530, Suresh Jayaraman wrote:
>>
>> The validateascii() check in imconv() maps NFSv4 domain names with non-ASCII
>> characters to 'nobody'. In setups where Active directory or LDAP is used this
>> causes names with UTF-8 characters to be mapped to 'nobody' because of this
>> check.
>>
>> As Bruce Fields puts it:
>>
>> "idmapd doesn't seem like the right place to enforce restrictions on names.
>> Once the system has allowed a name it's too late to be complaining about it
>> here."
>>
>> Remove the check from imconv() and remove the validateascii() function itself
>> as the only user of that function is being removed by this patch.
>
> Thanks, seem fine. The only other thing I notice is that
> validateascii() also checks (in a slightly strange way) for null
> termination of the string, and it's the only place in idmapd that does.
>
> But I think it'd be a kernel bug to pass up a non-terminated string
> here, so skipping that check is fine too.
>
> Possibly worth a comment, or a check just for null-termination if you
> want to be extra-careful.
>
You are right. I think being extra-careful is Ok. I'll respin this patch
with a null-termination check.
Thanks
--
Suresh Jayaraman
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-17 21:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-14 13:40 [PATCH] idmapd: allow non-ASCII characters (UTF-8) in NFSv4 domain name Suresh Jayaraman
2012-12-17 15:15 ` J. Bruce Fields
2012-12-17 21:45 ` Steve Dickson
-- strict thread matches above, loose matches on Subject: below --
2012-12-13 16:29 Suresh Jayaraman
2012-12-13 16:50 ` J. Bruce Fields
2012-12-14 13:37 ` Suresh Jayaraman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).