[fix] permissions on install #4695

line-o · 2023-01-21T14:26:02Z

Description:

Xquery modules and collections have to be set as executable on installation if the mode set in conf.xml or in the permission element in repo.xml does not set it. With this PR applied this will be limited to the owner if the group or others cannot read or write the resource or collection.

Reference:

refs #4686
Completes a TODO in the code.

Type of tests:

Junit-Tests

line-o · 2023-01-21T14:37:27Z

tests failures related to #4694

reinhapa

Could the formatting and the actual fix be splitt into two separate PRs? Also there are two POM files where the version is still not updated (I created a separate PR #4696 for this issue)

line-o · 2023-01-22T12:11:06Z

@reinhapa that's why I reference #4694

line-o · 2023-01-22T12:13:22Z

I already separated the formatting fix into a separate commit. This is the agreed way, I believe. If you want to just examine the changes please look at the commits 6f52c59, e83a2b7 and 836d238

exist-core/src/main/java/org/exist/security/AbstractUnixStylePermission.java

dizzzz

I have no idea :-=) 😵‍💫

reinhapa

Not totally sure if this is the needed solution, but the test coverage will improve here.

This method will add the executable flag to the given mode for - the owner - the group, if the group has read or write access - others, if they have read or write access

Reformatted code (with intelliJ defaults), removed unused variables.

Deployment.setPermissions now calls UnixStylePermission.safeSetExecutable instead of setting them always to world executable.

adamretter · 2023-01-23T21:33:14Z

We really should not do this. This sets things executable that it is the responsibility of the user or package maintainer to do. We cannot and should not assume that things should just be set executable for convenience.

line-o · 2023-01-23T21:43:16Z

@adamretter What do you mean by "we should not do this"? Keep it as it is?
8 years ago you added this TODO - This PR will add the executable bit only to groups and users if they have read permissions. What is a collection worth if it is created with read but without executable rights?

adamretter · 2023-01-23T21:46:06Z

Keep it as it is

No my preference would be to remove the security problem that was added where it should never have been added, i.e. this comment of mine:

mode = mode | 0111;     //TODO(AR) Whoever did this - this is a really bad idea. You are circumventing the security of the system

sonarcloud · 2023-01-23T21:47:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

line-o · 2023-01-23T21:49:35Z

Sorry, I could not find a solution there. I provide one which might not be your preferred solution but removing this line will break more than it solves.

reinhapa · 2023-01-24T06:00:01Z

Keep it as it is

No my preference would be to remove the security problem that was added where it should never have been added, i.e. this comment of mine:
mode = mode | 0111;     //TODO(AR) Whoever did this - this is a really bad idea. You are circumventing the security of the system

@adamretter how would you remove this security problem in your opinion? Remove that line completely in the next major version and document as such?

line-o · 2023-01-24T09:26:46Z

That is not a viable solution @adamretter @reinhapa
Please provide a path forward that leaves installed applications usable.

reinhapa · 2023-01-24T09:32:15Z

That is not a viable solution @adamretter @reinhapa Please provide a path forward that leaves installed applications usable.

@line-o What exactly is not a viable solution? Do not touch any access settings? From the discussion in the last community talk I thought, that this code is only executed on installation time... So from that point of view, that change would only have an impact if you install new XARs that have improper security definitions. Or do I not get the point here?

line-o · 2023-01-24T09:41:34Z

There is no other way to set collections as executable on installation by default. The same is true for XQuery scripts.
There is a reason this line is there since eight years despite the scary comment.

reinhapa · 2023-01-24T10:05:41Z

There is no other way to set collections as executable on installation by default. The same is true for XQuery scripts.

So you mean there is no way to define the according access definition with a XAR file? Is this what you are trying to say? In that case I would propose to extend the XAR API in order to support the access settings and only using the existing fallback method if no explicit definition is available to support backwards compatibility then...

line-o · 2023-01-24T10:09:33Z

@reinhapa let me reiterate:

case 1: a XAR without any application specific permissions set in repo.xml
the default permissions (rw-r--r-- at the time of writing) are applied to all resources and collections extracted from the xar. Leaving this unchanged will not even grant the owner (admin/DBA by default) the ability to open any of its collections nor execute any of the xquery scripts.

case 2: a XAR with a permission element in repo.xml
The permissions element (I only have seen applications with a single one, including bundled applications) can set the permissions for a single owner and group and can have one mode which will be applied to all collections and resources.
Let's say this is even more restrictive and set to r--r-----. Previously this would have set the mode for collections and xquery scripts to r-xr-x--x and with this PR applied it is r-xr-x---. That is an improvement in security. Doing this does result in the application not being accessible for unauthenticated users, though. They will not even be able to access the controller.xq or a login page. But this is also the status quo as you need read rights in order to execute queries (this is something we should discuss separately).

Having both collections and xquery scripts executable for database users who can also read or write the same resource is reasonable. It must be possible in a straightforward way for the vast majority of users of our platform to end up with a working application. Any other solution must provide this path.

line-o · 2023-01-24T10:33:44Z

I would propose to extend the XAR API in order to support the access settings and only using the existing fallback method if no explicit definition is available to support backwards compatibility then...

I agree, having fine-grained control over permissions at installation time is the way forward. But this will take time to introduce as we have not even started yet discussing it and is clearly out of scope of this PR. This fallback should still then use safeSetExecutable. So it does not touch the change introduced in this PR. I am convinced this change is a step in the right direction.

If we keep this preferably declarative permissions setting logic within repo.xml this can be done without having to change the expath packaging standard. I can open a feature-request in the following days so that we do not forget about it again.

joewiz · 2023-03-06T19:11:27Z

During last week and this week's Community Call, we discussed eXist's current behavior and what this PR changes. We understand that this PR is very narrow in focus. It does not break existing packages. It only gives package creators a new ability to control the "executable" flag on collections and XQuery files via the repo.xml. If packages don't specify this (i.e., any existing package), permissions will fall back on the defaults specified in conf.xml. These defaults align with what's hardcoded for package installation now. So effectively this doesn't change the current defaults.

We concluded that the PR is acceptable.

The only thing remaining is that the existing documentation should be surveyed for necessary edits / expansion.

line-o requested a review from a team January 21, 2023 14:32

line-o added the security label Jan 21, 2023

reinhapa requested changes Jan 22, 2023

View reviewed changes

dizzzz reviewed Jan 23, 2023

View reviewed changes

exist-core/src/main/java/org/exist/security/AbstractUnixStylePermission.java Outdated Show resolved Hide resolved

line-o requested a review from reinhapa January 23, 2023 11:14

line-o force-pushed the fix/permissions-on-install branch from 836d238 to 523be46 Compare January 23, 2023 13:11

dizzzz reviewed Jan 23, 2023

View reviewed changes

reinhapa approved these changes Jan 23, 2023

View reviewed changes

line-o added 5 commits January 23, 2023 22:17

[refactor] add safeSetExecutable to AbstractUnixStylePermission

0099bef

This method will add the executable flag to the given mode for - the owner - the group, if the group has read or write access - others, if they have read or write access

[refactor] cleanup of AbstractUnixStylePermission

d383df8

Reformatted code (with intelliJ defaults), removed unused variables.

[fix] install: safely set xq and collections as executable

d41268f

Deployment.setPermissions now calls UnixStylePermission.safeSetExecutable instead of setting them always to world executable.

[test] unit test UnixStylePermission.safeSetExecutable

cc94a45

[refactor] address code smells

ea72991

line-o force-pushed the fix/permissions-on-install branch from 9efc399 to ea72991 Compare January 23, 2023 21:17

line-o mentioned this pull request Jan 24, 2023

[feature] introduce fine-grained control over collection and resource permissions in repo.xml #4701

Open

line-o added this to the eXist-7.0.0 milestone Jan 31, 2023

reinhapa requested a review from dizzzz March 7, 2023 21:15

joewiz added the needs documentation Signals issues or PRs that will require an update to the documentation repo label Mar 8, 2023

reinhapa merged commit 1eaa076 into eXist-db:develop Mar 11, 2023

line-o deleted the fix/permissions-on-install branch April 3, 2023 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] permissions on install #4695

[fix] permissions on install #4695

line-o commented Jan 21, 2023 •

edited

Loading

line-o commented Jan 21, 2023

reinhapa left a comment

line-o commented Jan 22, 2023

line-o commented Jan 22, 2023

dizzzz left a comment

reinhapa left a comment

adamretter commented Jan 23, 2023

line-o commented Jan 23, 2023

adamretter commented Jan 23, 2023

sonarcloud bot commented Jan 23, 2023

line-o commented Jan 23, 2023

reinhapa commented Jan 24, 2023

line-o commented Jan 24, 2023 •

edited

Loading

reinhapa commented Jan 24, 2023

line-o commented Jan 24, 2023

reinhapa commented Jan 24, 2023

line-o commented Jan 24, 2023 •

edited

Loading

line-o commented Jan 24, 2023 •

edited

Loading

joewiz commented Mar 6, 2023

[fix] permissions on install #4695

[fix] permissions on install #4695

Conversation

line-o commented Jan 21, 2023 • edited Loading

Description:

Reference:

Type of tests:

line-o commented Jan 21, 2023

reinhapa left a comment

Choose a reason for hiding this comment

line-o commented Jan 22, 2023

line-o commented Jan 22, 2023

dizzzz left a comment

Choose a reason for hiding this comment

reinhapa left a comment

Choose a reason for hiding this comment

adamretter commented Jan 23, 2023

line-o commented Jan 23, 2023

adamretter commented Jan 23, 2023

sonarcloud bot commented Jan 23, 2023

line-o commented Jan 23, 2023

reinhapa commented Jan 24, 2023

line-o commented Jan 24, 2023 • edited Loading

reinhapa commented Jan 24, 2023

line-o commented Jan 24, 2023

reinhapa commented Jan 24, 2023

line-o commented Jan 24, 2023 • edited Loading

line-o commented Jan 24, 2023 • edited Loading

joewiz commented Mar 6, 2023

line-o commented Jan 21, 2023 •

edited

Loading

line-o commented Jan 24, 2023 •

edited

Loading

line-o commented Jan 24, 2023 •

edited

Loading

line-o commented Jan 24, 2023 •

edited

Loading