Thread: Question: CREATE EXTENSION and create schema permission?
CreateExtension() possibly creates a new schema when the supplied extension was not relocatable and the target schema was given by control file of the extension. However, it allows users to create a new schema with his ownership, even if current user does not have permission to create a new schema. Oid extowner = GetUserId(); : else if (control->schema != NULL) { /* * The extension is notrelocatable and the author gave us a schema * for it. We create the schema here if it does not already exist. */ schemaName = control->schema; schemaOid = get_namespace_oid(schemaName, true); if (schemaOid == InvalidOid) { schemaOid = NamespaceCreate(schemaName, extowner); /* Advancecmd counter to make the namespace visible */ CommandCounterIncrement(); } } It seems to me that we should inject permission checks here like as CreateSchemaCommand() doing. /* * To create a schema, must have schema-create privilege on the current * database and must be able to becomethe target role (this does not * imply that the target role itself must have create-schema privilege). * Thelatter provision guards against "giveaway" attacks. Note that a * superuser will always have both of these privilegesa fortiori. */ aclresult = pg_database_aclcheck(MyDatabaseId, saved_uid, ACL_CREATE); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, ACL_KIND_DATABASE, get_database_name(MyDatabaseId)); I didn't follow the discussion about extension so much when it got merged. Please tell me, if it was a topic already discussed before. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Kohei KaiGai <kaigai@kaigai.gr.jp> writes: > However, it allows users to create a new schema with his ownership, > even if current user does not have permission to create a new schema. [...] > It seems to me that we should inject permission checks here like as > CreateSchemaCommand() doing. It seems to me the code has been written this way before we relaxed the superuser only check in CREATE EXTENSION. I'm not enough into security to convince myself there's harm to protect against here, but I would agree there's a sound logic into refusing to create the schema if the current role isn't granted that operation. Please note, though, that you're effectively forbidding the role to create the extension. As it's not relocatable, the role will not be able to install it into another schema. Which could be exactly what you wanted to achieve. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
2011/8/21 Dimitri Fontaine <dimitri@2ndquadrant.fr>: > Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >> However, it allows users to create a new schema with his ownership, >> even if current user does not have permission to create a new schema. > [...] >> It seems to me that we should inject permission checks here like as >> CreateSchemaCommand() doing. > > It seems to me the code has been written this way before we relaxed the > superuser only check in CREATE EXTENSION. I'm not enough into security > to convince myself there's harm to protect against here, but I would > agree there's a sound logic into refusing to create the schema if the > current role isn't granted that operation. > > Please note, though, that you're effectively forbidding the role to > create the extension. As it's not relocatable, the role will not be > able to install it into another schema. Which could be exactly what you > wanted to achieve. > The current implementation set the current user as owner of the new schema. The default permission check of schema allows owner to create several kinds of underlying objects. In the result, we may consider a scenario that a user without permissions to create new objects possibly get a schema created by CREATE EXTENSION that allows him to create new objects (such as table, function, ...). I don't think it is a desirable behavior. :-( Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Kohei KaiGai <kaigai@kaigai.gr.jp> writes: > The current implementation set the current user as owner of the new schema. > The default permission check of schema allows owner to create several kinds > of underlying objects. > > In the result, we may consider a scenario that a user without permissions to > create new objects possibly get a schema created by CREATE EXTENSION > that allows him to create new objects (such as table, function, ...). > > I don't think it is a desirable behavior. :-( Agreed, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
The attached patch adds permission check at the scenario that I explained bellow. Unlike CreateSchemaCommand(), we don't have check_is_member_of_role() here because the extowner is obviously same with the current user in this code path. I hope this patch being also back ported to v9.1 tree, not only v9.2 development. Thanks, 2011/8/21 Dimitri Fontaine <dimitri@2ndquadrant.fr>: > Kohei KaiGai <kaigai@kaigai.gr.jp> writes: >> The current implementation set the current user as owner of the new schema. >> The default permission check of schema allows owner to create several kinds >> of underlying objects. >> >> In the result, we may consider a scenario that a user without permissions to >> create new objects possibly get a schema created by CREATE EXTENSION >> that allows him to create new objects (such as table, function, ...). >> >> I don't think it is a desirable behavior. :-( > > Agreed, > -- > Dimitri Fontaine > http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
Kohei KaiGai <kaigai@kaigai.gr.jp> writes: > The attached patch adds permission check at the scenario that I > explained bellow. Instead of using this patch, I changed the code to call CreateSchemaCommand itself. The test that was still missing was the one to restrict the schema name to not start with "pg_". It seemed to me that if we were treating this as a basically nonprivileged schema creation operation, that rule ought to be enforced too, as well as any other restrictions that we might someday add to CREATE SCHEMA execution. regards, tom lane