Thread: Re: [BUGS] BUG #1134: ALTER USER ... RENAME breaks md5 passwords
PostgreSQL Bugs List wrote:
>
> The following bug has been logged online:
>
> Bug reference: 1134
> Logged by: Fabien COELHO
>
> Email address: coelho@cri.ensmp.fr
>
> PostgreSQL version: 7.5 Dev
>
> Operating system: any
>
> Description: ALTER USER ... RENAME breaks md5 passwords
>
> Details:
>
> If you rename a user with a md5 password, the
> password is broken. md5 passwords are the default,
> so it means that renaming a user with a password
> does not work by default.
>
> This is because the username is used implicitly as salt. This was a bad idea
> (tm).
>
> Fixing this has implications on the client/server
> protocol for md5 authentication. If you're going
> to fix it some day, consider also adding more
> characters to the server nonce used in the protocol.
Yes, the problem is that we used the username for the salt, just like
FreeBSD does for its MD5 passwords. Of course, you can't rename unix
users, while PostgreSQL allows user renaming.
The attached patch clears the password field on rename:
test=> CREATE USER pass password 'aa';
CREATE USER
test=> ALTER USER pass RENAME TO pass2;
NOTICE: password cleared because OF USER RENAME
ALTER USER
test=> ALTER USER pass2 RENAME TO pass3;
ALTER USER
and adds documention explaining this behavior. I can't think of a
better solution.
--
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, Pennsylvania 19073
Index: doc/src/sgml/ref/alter_user.sgml
===================================================================
RCS file: /cvsroot/pgsql-server/doc/src/sgml/ref/alter_user.sgml,v
retrieving revision 1.32
diff -c -c -r1.32 alter_user.sgml
*** doc/src/sgml/ref/alter_user.sgml 29 Nov 2003 19:51:38 -0000 1.32
--- doc/src/sgml/ref/alter_user.sgml 27 Apr 2004 00:29:56 -0000
***************
*** 57,62 ****
--- 57,64 ----
The second variant changes the name of the user. Only a database
superuser can rename user accounts. The session user cannot be
renamed. (Connect as a different user if you need to do that.)
+ Because <literal>MD5</>-encrypted passwords use the username as
+ cryptographic salt, renaming a user clears their password.
</para>
<para>
Index: src/backend/commands/user.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/commands/user.c,v
retrieving revision 1.139
diff -c -c -r1.139 user.c
*** src/backend/commands/user.c 16 Mar 2004 05:05:57 -0000 1.139
--- src/backend/commands/user.c 27 Apr 2004 00:29:57 -0000
***************
*** 959,966 ****
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("user \"%s\" does not exist", stmt->user)));
! if (!(superuser()
! || ((Form_pg_shadow) GETSTRUCT(oldtuple))->usesysid == GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied")));
--- 959,966 ----
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("user \"%s\" does not exist", stmt->user)));
! if (!(superuser() ||
! ((Form_pg_shadow) GETSTRUCT(oldtuple))->usesysid == GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied")));
***************
*** 1157,1172 ****
void
RenameUser(const char *oldname, const char *newname)
{
! HeapTuple tup;
Relation rel;
!
/* ExclusiveLock because we need to update the password file */
rel = heap_openr(ShadowRelationName, ExclusiveLock);
! tup = SearchSysCacheCopy(SHADOWNAME,
CStringGetDatum(oldname),
0, 0, 0);
! if (!HeapTupleIsValid(tup))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("user \"%s\" does not exist", oldname)));
--- 1157,1177 ----
void
RenameUser(const char *oldname, const char *newname)
{
! HeapTuple oldtuple,
! newtuple;
Relation rel;
! Datum repl_val[Natts_pg_shadow];
! char repl_null[Natts_pg_shadow];
! char repl_repl[Natts_pg_shadow];
! int i;
!
/* ExclusiveLock because we need to update the password file */
rel = heap_openr(ShadowRelationName, ExclusiveLock);
! oldtuple = SearchSysCache(SHADOWNAME,
CStringGetDatum(oldname),
0, 0, 0);
! if (!HeapTupleIsValid(oldtuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("user \"%s\" does not exist", oldname)));
***************
*** 1177,1183 ****
* not be an actual problem besides a little confusion, so think about
* this and decide.
*/
! if (((Form_pg_shadow) GETSTRUCT(tup))->usesysid == GetSessionUserId())
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("session user may not be renamed")));
--- 1182,1188 ----
* not be an actual problem besides a little confusion, so think about
* this and decide.
*/
! if (((Form_pg_shadow) GETSTRUCT(oldtuple))->usesysid == GetSessionUserId())
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("session user may not be renamed")));
***************
*** 1196,1208 ****
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to rename users")));
! /* rename */
! namestrcpy(&(((Form_pg_shadow) GETSTRUCT(tup))->usename), newname);
! simple_heap_update(rel, &tup->t_self, tup);
! CatalogUpdateIndexes(rel, tup);
heap_close(rel, NoLock);
- heap_freetuple(tup);
user_file_update_needed = true;
}
--- 1201,1231 ----
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to rename users")));
! for (i = 0; i < Natts_pg_shadow; i++)
! repl_repl[i] = ' ';
!
! repl_repl[Anum_pg_shadow_usename - 1] = 'r';
! repl_val[Anum_pg_shadow_usename - 1] = DirectFunctionCall1(namein,
! CStringGetDatum(newname));
! repl_null[Anum_pg_shadow_usename - 1] = ' ';
+ if (!heap_attisnull(oldtuple, Anum_pg_shadow_passwd))
+ {
+ /* MD5 uses the username as salt, so just clear it on a rename */
+ repl_repl[Anum_pg_shadow_passwd - 1] = 'r';
+ repl_null[Anum_pg_shadow_passwd - 1] = 'n';
+
+ ereport(NOTICE,
+ (errmsg("password cleared because of user rename")));
+ }
+
+ newtuple = heap_modifytuple(oldtuple, rel, repl_val, repl_null, repl_repl);
+ simple_heap_update(rel, &oldtuple->t_self, newtuple);
+
+ CatalogUpdateIndexes(rel, newtuple);
+
+ ReleaseSysCache(oldtuple);
heap_close(rel, NoLock);
user_file_update_needed = true;
}
Dear Bruce,
> Yes, the problem is that we used the username for the salt, just like
> FreeBSD does for its MD5 passwords.
Not that I know of on FreeBSD?
shell> uname -a
FreeBSD palo-alto2.ensmp.fr 4.9-STABLE FreeBSD 4.9-STABLE #5: Mon Mar 1 21:31:30 CET 2004
root@palo-alto2.ensmp.fr:/usr/src/sys/compile/IAR2Mi386
shell> grep coelho /var/yp/master.passwd
coelho:$1$00EacB0I$4kQ/HmqFFQANZP/mxj8ZX0:210:20::0:0:COELHO, Fabien:/users/cri/coelho:/usr/local/bin/bash
^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^
salt some base 64 encoding of 1002 paranoid md5 computations.
Even of the salt is based on the login, the point is that it is stored
separatly, so the system does not rely on the login string to check the
password.
The only other scheme which requires the user password somehow is the HTTP
digest authentification, and AFAIK no one in the world uses it;-)
> The attached patch clears the password field on rename:
By 'clearing' and after a look at the patch, I understand that the access
will be denied after the rename, which is the current behavior anyway;-)
> and adds documention explaining this behavior. I can't think of a
> better solution.
Yes, I'm afraid there is no 'light' fix, other than acknowledging the
fact... Not a big issue.
Thanks,
--
Fabien Coelho - coelho@cri.ensmp.fr
Fabien COELHO wrote: > > Dear Bruce, > > > Yes, the problem is that we used the username for the salt, just like > > FreeBSD does for its MD5 passwords. > > Not that I know of on FreeBSD? > > shell> uname -a > FreeBSD palo-alto2.ensmp.fr 4.9-STABLE FreeBSD 4.9-STABLE #5: Mon Mar 1 21:31:30 CET 2004 root@palo-alto2.ensmp.fr:/usr/src/sys/compile/IAR2Mi386 > > shell> grep coelho /var/yp/master.passwd > coelho:$1$00EacB0I$4kQ/HmqFFQANZP/mxj8ZX0:210:20::0:0:COELHO, Fabien:/users/cri/coelho:/usr/local/bin/bash > ^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ > salt some base 64 encoding of 1002 paranoid md5 computations. > > Even of the salt is based on the login, the point is that it is stored > separatly, so the system does not rely on the login string to check the > password. Oh, I thought FreeBSD used the username. Not sure were we got that idea. I know we needed a different salt only so users with the same password would not have the same MD5 value. -- 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, Pennsylvania 19073
On Tue, Apr 27, 2004 at 09:37:50AM +0200, Fabien COELHO wrote: > Even of the salt is based on the login, the point is that it is stored > separatly, so the system does not rely on the login string to check the > password. > > The only other scheme which requires the user password somehow is the HTTP > digest authentification, and AFAIK no one in the world uses it;-) I think (some of the) SASL authentication mechanisms also use a digest of the user and password, if that's what you meant. But the username and password have to be stored separately on the server anyway, just like HTTP digest -- they are means of hiding it on the wire, not on disk. -- Alvaro Herrera (<alvherre[a]dcc.uchile.cl>) "El miedo atento y previsor es la madre de la seguridad" (E. Burke)
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The attached patch clears the password field on rename:
I think you should clear the password field *only* if it's
MD5-encrypted.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The attached patch clears the password field on rename: > > I think you should clear the password field *only* if it's > MD5-encrypted. I thought about that but it seems strange to conditionally do the clearing, but if you think we should, I can do it. -- 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, Pennsylvania 19073
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The attached patch clears the password field on rename: > > I think you should clear the password field *only* if it's > MD5-encrypted. Patch attached and applied. -- 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, Pennsylvania 19073 Index: doc/src/sgml/ref/alter_user.sgml =================================================================== RCS file: /cvsroot/pgsql-server/doc/src/sgml/ref/alter_user.sgml,v retrieving revision 1.32 diff -c -c -r1.32 alter_user.sgml *** doc/src/sgml/ref/alter_user.sgml 29 Nov 2003 19:51:38 -0000 1.32 --- doc/src/sgml/ref/alter_user.sgml 5 May 2004 03:06:44 -0000 *************** *** 57,62 **** --- 57,65 ---- The second variant changes the name of the user. Only a database superuser can rename user accounts. The session user cannot be renamed. (Connect as a different user if you need to do that.) + Because <literal>MD5</>-encrypted passwords use the username as + cryptographic salt, renaming a user clears their <literal>MD5</> + password. </para> <para> Index: src/backend/commands/user.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/commands/user.c,v retrieving revision 1.139 diff -c -c -r1.139 user.c *** src/backend/commands/user.c 16 Mar 2004 05:05:57 -0000 1.139 --- src/backend/commands/user.c 5 May 2004 03:06:46 -0000 *************** *** 959,966 **** (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("user \"%s\" does not exist", stmt->user))); ! if (!(superuser() ! || ((Form_pg_shadow) GETSTRUCT(oldtuple))->usesysid == GetUserId())) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied"))); --- 959,966 ---- (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("user \"%s\" does not exist", stmt->user))); ! if (!(superuser() || ! ((Form_pg_shadow) GETSTRUCT(oldtuple))->usesysid == GetUserId())) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied"))); *************** *** 1157,1172 **** void RenameUser(const char *oldname, const char *newname) { ! HeapTuple tup; Relation rel; ! /* ExclusiveLock because we need to update the password file */ rel = heap_openr(ShadowRelationName, ExclusiveLock); ! tup = SearchSysCacheCopy(SHADOWNAME, CStringGetDatum(oldname), 0, 0, 0); ! if (!HeapTupleIsValid(tup)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("user \"%s\" does not exist", oldname))); --- 1157,1181 ---- void RenameUser(const char *oldname, const char *newname) { ! HeapTuple oldtuple, ! newtuple; ! TupleDesc dsc; Relation rel; ! Datum datum; ! bool isnull; ! Datum repl_val[Natts_pg_shadow]; ! char repl_null[Natts_pg_shadow]; ! char repl_repl[Natts_pg_shadow]; ! int i; ! /* ExclusiveLock because we need to update the password file */ rel = heap_openr(ShadowRelationName, ExclusiveLock); + dsc = RelationGetDescr(rel); ! oldtuple = SearchSysCache(SHADOWNAME, CStringGetDatum(oldname), 0, 0, 0); ! if (!HeapTupleIsValid(oldtuple)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("user \"%s\" does not exist", oldname))); *************** *** 1177,1183 **** * not be an actual problem besides a little confusion, so think about * this and decide. */ ! if (((Form_pg_shadow) GETSTRUCT(tup))->usesysid == GetSessionUserId()) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("session user may not be renamed"))); --- 1186,1192 ---- * not be an actual problem besides a little confusion, so think about * this and decide. */ ! if (((Form_pg_shadow) GETSTRUCT(oldtuple))->usesysid == GetSessionUserId()) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("session user may not be renamed"))); *************** *** 1196,1208 **** (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("must be superuser to rename users"))); ! /* rename */ ! namestrcpy(&(((Form_pg_shadow) GETSTRUCT(tup))->usename), newname); ! simple_heap_update(rel, &tup->t_self, tup); ! CatalogUpdateIndexes(rel, tup); heap_close(rel, NoLock); - heap_freetuple(tup); user_file_update_needed = true; } --- 1205,1237 ---- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("must be superuser to rename users"))); ! for (i = 0; i < Natts_pg_shadow; i++) ! repl_repl[i] = ' '; ! ! repl_repl[Anum_pg_shadow_usename - 1] = 'r'; ! repl_val[Anum_pg_shadow_usename - 1] = DirectFunctionCall1(namein, ! CStringGetDatum(newname)); ! repl_null[Anum_pg_shadow_usename - 1] = ' '; ! ! datum = heap_getattr(oldtuple, Anum_pg_shadow_passwd, dsc, &isnull); ! ! if (!isnull && isMD5(DatumGetCString(DirectFunctionCall1(textout, datum)))) ! { ! /* MD5 uses the username as salt, so just clear it on a rename */ ! repl_repl[Anum_pg_shadow_passwd - 1] = 'r'; ! repl_null[Anum_pg_shadow_passwd - 1] = 'n'; ! ! ereport(NOTICE, ! (errmsg("MD5 password cleared because of user rename"))); ! } ! ! newtuple = heap_modifytuple(oldtuple, rel, repl_val, repl_null, repl_repl); ! simple_heap_update(rel, &oldtuple->t_self, newtuple); ! ! CatalogUpdateIndexes(rel, newtuple); + ReleaseSysCache(oldtuple); heap_close(rel, NoLock); user_file_update_needed = true; }
Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > The attached patch clears the password field on rename:
> >
> > I think you should clear the password field *only* if it's
> > MD5-encrypted.
>
> Patch attached and applied.
Oh, I forgot to display the new behavior:
test=> CREATE USER test;
CREATE USER
test=> ALTER USER test RENAME TO test2;
ALTER USER
test=> ALTER USER test2 UNENCRYPTED PASSWORD 'x';
ALTER USER
test=> ALTER USER test2 RENAME TO test4;
ALTER USER
test=> ALTER USER test4 PASSWORD 'x';
ALTER USER
test=> ALTER USER test4 RENAME TO test8;
NOTICE: MD5 password cleared because of user rename
ALTER USER
test=> SELECT * FROM pg_shadow WHERE usename = 'test8';
usename | usesysid | usecreatedb | usesuper | usecatupd | passwd |
valuntil | useconfig
---------+----------+-------------+----------+-----------+--------+----------+-----------
test8 | 100 | f | f | f | |
|
(1 row)
--
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, Pennsylvania 19073