Thread: getting confused parsing ACLITEMS...

getting confused parsing ACLITEMS...

From
"Christopher Kings-Lynne"
Date:
This is the situation, I create a user called "

test=# create user """";
CREATE USER
test=# drop user """";
DROP USER
test=# create user """";
CREATE USER
test=# create table temp(a int4);
CREATE TABLE
test=# grant select on temp to """";
GRANT
test=# \dp temp               Access privileges for database "test"Schema | Table |                  Access privileges
--------+-------+-----------------------------------------------------public | temp  |
{chriskl=a*r*w*d*R*x*t*/chriskl,"\"\"\"=r/chriskl"}
(1 row)

OK, so the second aclitem is:
"\"\"\"=r/chriskl"

So how on earth does that quoting work????

If I remove the first and last quote (assuming the whole thing is quoted),
unescape it, then I get this:

"""=r/chriskl

Which I cannot parse, as """ doens't mean anything!  I think that the second
aclitem should appear like this;
"\"\\\"\"=r/chriskl"

Which will (after removing string quotes and unescaping), reduce to this:

"\""=r/chriskl

I notice that it doesn't confuse postgres itself though:

test=# revoke select on temp from """";
REVOKE
test=# \dp temp      Access privileges for database "test"Schema | Table |        Access privileges
--------+-------+----------------------------------public | temp  | {chriskl=a*r*w*d*R*x*t*/chriskl}
(1 row)

So is there a bug here?

Chris



Re: getting confused parsing ACLITEMS...

From
"Christopher Kings-Lynne"
Date:
The situation seems to be a bug that this patch would address.  It seems to
me that when a username is considered unsafe due to containing double
quotes, the double quotes should be escaped (and the backslashes)!

Does this look alright?

Chris

Index: src/backend/utils/adt/acl.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/adt/acl.c,v
retrieving revision 1.94
diff -c -r1.94 acl.c
*** src/backend/utils/adt/acl.c 4 Aug 2003 02:40:04 -0000       1.94
--- src/backend/utils/adt/acl.c 8 Aug 2003 09:03:19 -0000
***************
*** 124,131 ****       }       if (!safe)               *p++ = '"';
!       for (src = s; *src; src++)               *p++ = *src;       if (!safe)               *p++ = '"';       *p =
'\0';
--- 124,134 ----       }       if (!safe)               *p++ = '"';
!       for (src = s; *src; src++) {
!               if (!safe && (*src == '"' || *src == '\\'))
!                       *p++ = '\\';               *p++ = *src;
+       }       if (!safe)               *p++ = '"';       *p = '\0';





Re: getting confused parsing ACLITEMS...

From
Tom Lane
Date:
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> The situation seems to be a bug that this patch would address.  It seems to
> me that when a username is considered unsafe due to containing double
> quotes, the double quotes should be escaped (and the backslashes)!

> Does this look alright?

I would personally say that the convention ought to match what is done
in quoted identifiers, namely: " becomes "", backslash is not special
and therefore doesn't need anything.

More to the point, this is highly incomplete... you did not teach the
adjacent getid routine about this, and there is code in (at least)
pg_dump.c that knows the quoting conventions used here.
        regards, tom lane


Re: getting confused parsing ACLITEMS...

From
Tom Lane
Date:
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> More to the point, this is highly incomplete... you did not teach the
>> adjacent getid routine about this, and there is code in (at least)
>> pg_dump.c that knows the quoting conventions used here.

> Hang on - those routines can parse the acls just fine?  How?  How do they
> handle usernames with equals signs in them (my major prob).  How can it
> work at all?

IIRC, the present code assumes that usernames won't contain embedded
doublequotes.  I did recently fix pg_dump to work in cases that it
wouldn't have handled before, including embedded equals.  (BTW, my
mistake above: the pg_dump code is not in pg_dump.c, but in dumputils.c.
Look at copyAclUserName in particular.)
        regards, tom lane


Re: getting confused parsing ACLITEMS...

From
Tom Lane
Date:
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> So if you agree that there is a quoting problem,and you don't mind
> breaking backwards compatibility for it, I'll do a complete patch...

I don't see any backwards-compatibility issue, because usernames
containing double quotes just plain don't work in past releases;
we've never before bothered to have a complete quoting solution
in ACLs.
        regards, tom lane


Re: getting confused parsing ACLITEMS...

From
Christopher Kings-Lynne
Date:
> More to the point, this is highly incomplete... you did not teach the
> adjacent getid routine about this, and there is code in (at least)
> pg_dump.c that knows the quoting conventions used here.

Hang on - those routines can parse the acls just fine?  How?  How do they
handle usernames with equals signs in them (my major prob).  How can it
work at all?

Chris




Re: getting confused parsing ACLITEMS...

From
Christopher Kings-Lynne
Date:
OK,

So if you agree that there is a quoting problem,and you don't mind
breaking backwards compatibility for it, I'll do a complete patch...

Chris

On Fri, 8 Aug 2003, Tom Lane wrote:

> "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> > The situation seems to be a bug that this patch would address.  It seems to
> > me that when a username is considered unsafe due to containing double
> > quotes, the double quotes should be escaped (and the backslashes)!
>
> > Does this look alright?
>
> I would personally say that the convention ought to match what is done
> in quoted identifiers, namely: " becomes "", backslash is not special
> and therefore doesn't need anything.
>
> More to the point, this is highly incomplete... you did not teach the
> adjacent getid routine about this, and there is code in (at least)
> pg_dump.c that knows the quoting conventions used here.
>
>             regards, tom lane
>



Re: getting confused parsing ACLITEMS...

From
Andreas Pflug
Date:
Tom Lane wrote:

>Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>  
>
>>So if you agree that there is a quoting problem,and you don't mind
>>breaking backwards compatibility for it, I'll do a complete patch...
>>    
>>
>
>I don't see any backwards-compatibility issue, because usernames
>containing double quotes just plain don't work in past releases;
>we've never before bothered to have a complete quoting solution
>in ACLs.
>  
>

Is it useful to allow these special chars at all? Seems this creates a 
lot of work, and most admins will probably stick to "normal" user names 
anyway.

Regards,
Andreas



Re: getting confused parsing ACLITEMS...

From
Tom Lane
Date:
Andreas Pflug <pgadmin@pse-consulting.de> writes:
> Is it useful to allow these special chars at all? Seems this creates a 
> lot of work, and most admins will probably stick to "normal" user names 
> anyway.

Well, the reason it's been left unfixed for so long is exactly that it
didn't seem pressing.  But if Chris wants to do the work, I won't stand
in his way ...
        regards, tom lane


Re: getting confused parsing ACLITEMS...

From
Andrew Dunstan
Date:
Of course, now I've just gone to some trouble to accomodate funky 
characters in user and dbnames in logging I'd have to kill him ...

:-)

Seriously, I think there's a good case for banning a few characters in 
at least some names - like []<>'"~#*|\ , say

andrew

Tom Lane wrote:

>Andreas Pflug <pgadmin@pse-consulting.de> writes:
>  
>
>>Is it useful to allow these special chars at all? Seems this creates a 
>>lot of work, and most admins will probably stick to "normal" user names 
>>anyway.
>>    
>>
>
>Well, the reason it's been left unfixed for so long is exactly that it
>didn't seem pressing.  But if Chris wants to do the work, I won't stand
>in his way ...
>
>
>  
>



Re: getting confused parsing ACLITEMS...

From
Christopher Kings-Lynne
Date:
> >I don't see any backwards-compatibility issue, because usernames
> >containing double quotes just plain don't work in past releases;
> >we've never before bothered to have a complete quoting solution
> >in ACLs.
> >
> >
>
> Is it useful to allow these special chars at all? Seems this creates a
> lot of work, and most admins will probably stick to "normal" user names
> anyway.

It's the principle of the thing :)  Also, I don't know how non-english
speakers treat their double quote characers...

Chris




Re: getting confused parsing ACLITEMS...

From
Christopher Kings-Lynne
Date:
> Seriously, I think there's a good case for banning a few characters in
> at least some names - like []<>'"~#*|\ , say

Why?  They're allowed in all other identifiers.  And what if someone
already has a database full of usernames with those chars?  They wouldn't
be able to load their dump properly...

Chris

> andrew
>
> Tom Lane wrote:
>
> >Andreas Pflug <pgadmin@pse-consulting.de> writes:
> >
> >
> >>Is it useful to allow these special chars at all? Seems this creates a
> >>lot of work, and most admins will probably stick to "normal" user names
> >>anyway.
> >>
> >>
> >
> >Well, the reason it's been left unfixed for so long is exactly that it
> >didn't seem pressing.  But if Chris wants to do the work, I won't stand
> >in his way ...
> >
> >
> >
> >
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
>



Re: getting confused parsing ACLITEMS...

From
Andrew Dunstan
Date:
I know. It just makes a few things a pain if you can't say "I know this 
character can't be part of that".

Nevermind. Just wishful thinking. I'll shut up now.

andrew

Christopher Kings-Lynne wrote:

>>Seriously, I think there's a good case for banning a few characters in
>>at least some names - like []<>'"~#*|\ , say
>>    
>>
>
>Why?  They're allowed in all other identifiers.  And what if someone
>already has a database full of usernames with those chars?  They wouldn't
>be able to load their dump properly...
>
>Chris
>  
>



Re: getting confused parsing ACLITEMS...

From
Bruce Momjian
Date:
I believe Tom just applied this.  Thanks.

---------------------------------------------------------------------------

Christopher Kings-Lynne wrote:
> The situation seems to be a bug that this patch would address.  It seems to
> me that when a username is considered unsafe due to containing double
> quotes, the double quotes should be escaped (and the backslashes)!
> 
> Does this look alright?
> 
> Chris
> 
> Index: src/backend/utils/adt/acl.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/adt/acl.c,v
> retrieving revision 1.94
> diff -c -r1.94 acl.c
> *** src/backend/utils/adt/acl.c 4 Aug 2003 02:40:04 -0000       1.94
> --- src/backend/utils/adt/acl.c 8 Aug 2003 09:03:19 -0000
> ***************
> *** 124,131 ****
>         }
>         if (!safe)
>                 *p++ = '"';
> !       for (src = s; *src; src++)
>                 *p++ = *src;
>         if (!safe)
>                 *p++ = '"';
>         *p = '\0';
> --- 124,134 ----
>         }
>         if (!safe)
>                 *p++ = '"';
> !       for (src = s; *src; src++) {
> !               if (!safe && (*src == '"' || *src == '\\'))
> !                       *p++ = '\\';
>                 *p++ = *src;
> +       }
>         if (!safe)
>                 *p++ = '"';
>         *p = '\0';
> 
> 
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
>                http://archives.postgresql.org
> 

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073