Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix PermissionDeniedException in NativeBroker.defragXMLresource #5311

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Fix PermissionDeniedException in NativeBroker.defragXMLresource #5311

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe48be4
[bugfix] do not set permissions on temporary document
line-o May 23, 2024
e6edd24
[test] address code review in UpdateInsertTriggersDefrag
line-o May 23, 2024
cf0ec0f
[bugfix] revert to previous constructor call for tempDoc in defragXML…
line-o May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions exist-core/src/main/java/org/exist/storage/NativeBroker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3171,9 +3171,9 @@ public Object start() {
return null;
}
}.run();
// create a copy of the old doc to copy the nodes into it
// Create a copy of the old document to copy the nodes into it.
// tempDoc serves as a storage container for the child nodes.
final DocumentImpl tempDoc = new DocumentImpl(null, doc.getDocId(), doc);
tempDoc.copyOf(this, doc, doc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think just removing this is the correct or secure solution. Can you write a bit more detail in the description of the PR about exactly what the problem was please, that might help me to suggest a better solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message body has some more detail.

With this patch applied, this can no longer happen as the call to DocumentImpl#copyOf is removed in defragXMLresource.
The temporary document is just a container to realign nodes on one BTree page and is destroyed within the method.

I hope this clarifies the motivation and justification.
I would like to point out that we arrived at this solution together with @wolfgangmm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely needs a more nuanced fix

Please only block PRs where you have either proof that it creates a problem or at least share the thoughts what the issue might be.

Copy link
Member

@adamretter adamretter May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please only block PRs where you have either proof that it creates a problem

Obviously!

or at least share the thoughts what the issue might be

I am concerned that the temporary document which looks persistent to me is not correctly initialised, also I am concerned that the security on it is now too relaxed, this could lead to a data leak/exploit.

Can you clearly describe the initial problem that you are trying to solve in the PR description please. For example, something like: There is an active user U with the security profile s(U), there is document D with the permissions p(D), when the action A is executed by the subject AS with security profile s(AS), it is rejected because s(AS) is incompatible with p(D).
Obviously you need to replace the symbols above with the actual details.

hope this clarifies the motivation and justification.

It just discusses the solution you arrived at, it does not clearly describe the problem you were trying to solve.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was debugging this code with Juri yesterday and as the original author of the defrag method, I confirm that the offending line tempDoc.copyOf(this, doc, doc); is not only unnecessary here, but can cause database corruption. The tempDoc is only used as a temporary container for copying nodes during defrag and is thrown away afterwards! Original permissions and ownership on the resource are not affected by the defragXMLResource operation in any way, because the tempDoc is never written. What is written is the original DocumentImpl, which still has the proper permissions and ownership.

It seems I originally added the call to tempDoc.copyOf 19 years ago and it was preserved during a series of refactorings later though the implementation of DocumentImpl had changed to the point that it now may actually cause a corruption when called in this case.

Please be aware that this issue is a ticking time bomb that will lead to data wipe out!!! Together with the even more fatal #5296 (because it's easier to run into) this is absolutely worth a hotfix, so I would give it a thumbs up, not excluding that more improvements could be done at a later point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wolfgangmm thanks for your reply. It is interesting to hear about these symptoms, and it does sound like something needs to be fixed. However, the actual original issue and its cause remain unclear. I would like further information please so that we may all understand it...

but can cause database corruption

Could you explain how that is possible please? As I understood it from @line-o's description there was a PermissionDeniedException which has not yet been explained. I am not sure how a PermissionDeniedException, which is a genuine way of forbidding a subject from doing something they are not allowed to do, could cause a database corruption here?

Original permissions and ownership on the resource are not affected by the defragXMLResource operation in any way, because the tempDoc is never written.

I did not expect that they would be.

What is written is the original DocumentImpl, which still has the proper permissions and ownership.

Okay.

What I (and presumably others) don't yet understand, and what has not yet been explained, is how can the current approach that places the same permissions onto tempDoc as the original resource that is to be defragmented, cause a PermissionDeniedException (and then also if I now understand you, a subsequent database corruption)?

Please be aware that this issue is a ticking time bomb that will lead to data wipe out!!!

I agree that it sounds like it needs to be fixed. At the moment I don't have enough information to see if the proposed fix correctly addresses the cause. I trust you and @line-o will provide the requested information about the underlying issue and cause...

Together with the even more fatal #5296 (because it's easier to run into)

Let's consider that separately. As at the moment #5296 is blocked by work that is still needed to address issues introduced in #5276 which itself was rushed and merged in violation of the agreed community process for reviewing and merging PRs.

this is absolutely worth a hotfix,

From what you have said so far, I agree. We just need more information to be able to complete the review of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@line-o added some information about how to reproduce the issue. It requires a certain combination of permissions. The corruption occurs because the PermissionDeniedException aborts the defrag process after some steps have already been applied. Therefore another way to prevent this might be to move the offending code further up to the start of the method. We did not follow this route as dropping the line is the cleaner way to solve the issue.

The PermissionDeniedException is eventually thrown in line 664 of DocumentImpl#copyOf:

permissions.setOwner(prev.get_1().getOwner());

why this happens I cannot explain entirely and it seems very weird that the owners would be different. It must be related to the setgid. But in itself it has nothing to do with the actual defrag routine and points to another bug in permission handling.

Copy link
Member

@adamretter adamretter May 24, 2024 •
edited by wolfgangmm
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that was my feeling from the start, which is why I wanted more information from yourselves. Based on your summation it seems to me that the this PR is a workaround and not a fix for the root issue.

Well, the root issue this PR tries to solve is the database corruption resulting from the defrag operation. The only correct resolution for this is to fix defrag, which we did. I strongly object against calling a fix a workaround.

Fixing the permission issue may help with other operations, but won't change this PR in any way.

Copy link
Member

@adamretter adamretter May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfgangmm I think you have accidentally deleted or overwritten my comments
Screenshot 2024-05-24 at 16 39 27

@wolfgangmm Are you able to restore my comments please? If not, I can try and re-create them...

final StreamListener listener = getIndexController().getStreamListener(doc, ReindexMode.STORE);
// copy the nodes
final NodeList nodes = doc.getChildNodes();
Expand All @@ -3198,15 +3198,17 @@ public Object start() {
return null;
}
}.run();
// assign new child nodes from the temporary document
doc.copyChildren(tempDoc);
doc.setSplitCount(0);
doc.setPageCount(tempDoc.getPageCount());
// store defragmented document
storeXMLResource(transaction, doc);
closeDocument();
if (LOG.isDebugEnabled()) {
LOG.debug("Defragmentation took {} ms.", (System.currentTimeMillis() - start));
}
} catch(final PermissionDeniedException | IOException e) {
} catch(final IOException e) {
LOG.error(e);
}
}
Expand Down
46 changes: 18 additions & 28 deletions exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,66 +21,56 @@
*/
package org.exist.xquery.update;

import org.exist.storage.DBBroker;
import org.exist.test.ExistXmldbEmbeddedServer;
import org.exist.test.TestConstants;
import org.junit.After;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
import org.xmldb.api.base.Collection;
import org.xmldb.api.base.ResourceSet;
import org.xmldb.api.base.XMLDBException;
import org.xmldb.api.modules.CollectionManagementService;
import org.xmldb.api.modules.XMLResource;
import org.xmldb.api.modules.XPathQueryService;
import org.xmldb.api.modules.XQueryService;

import static org.exist.storage.DBBroker.PROPERTY_XUPDATE_FRAGMENTATION_FACTOR;
import static org.exist.test.TestConstants.TEST_COLLECTION_URI;
import static org.exist.test.TestConstants.TEST_XML_URI;
import static org.exist.util.PropertiesBuilder.propertiesBuilder;
import static org.junit.Assert.assertEquals;

public class UpdateInsertTriggersDefrag {
@ClassRule
public static final ExistXmldbEmbeddedServer exist = new ExistXmldbEmbeddedServer(false, true, true, propertiesBuilder().put(DBBroker.PROPERTY_XUPDATE_FRAGMENTATION_FACTOR, -1).build());
final String path = TestConstants.TEST_COLLECTION_URI + "/" + TestConstants.TEST_XML_URI.toString();
final String xml = "<list><item>initial</item></list>";
Collection testCollection;
XQueryService queryService;
CollectionManagementService collectionService;

/**
* stores XML String and get Query Service
*
* @param documentName to be stored in the DB
* @param content to be stored in the DB
* @throws XMLDBException if an error occurs storing the document
*/
private void storeXML(final String documentName, final String content) throws XMLDBException {
final XMLResource doc = testCollection.createResource(documentName, XMLResource.class);
doc.setContent(content);
testCollection.storeResource(doc);
}
public static final ExistXmldbEmbeddedServer exist = new ExistXmldbEmbeddedServer(false, true, true,
propertiesBuilder().put(PROPERTY_XUPDATE_FRAGMENTATION_FACTOR, -1).build());
private final String path = TEST_COLLECTION_URI + "/" + TEST_XML_URI.toString();
private Collection testCollection;
private CollectionManagementService collectionService;

@Before
public void setUp() throws Exception {
collectionService = exist.getRoot().getService(CollectionManagementService.class);
testCollection = collectionService.createCollection(TestConstants.TEST_COLLECTION_URI.toString());
queryService = (XQueryService) testCollection.getService(XPathQueryService.class);
storeXML(TestConstants.TEST_XML_URI.toString(), xml);
testCollection = collectionService.createCollection(TEST_COLLECTION_URI.lastSegment().toString());
final XMLResource doc = testCollection.createResource(TEST_XML_URI.toString(), XMLResource.class);

doc.setContent("<list><item>initial</item></list>");
testCollection.storeResource(doc);
}

@After
public void tearDown() throws Exception {
collectionService.removeCollection(testCollection.getName());
testCollection.close();
}

@Test
public void triggerDefragAfterUpdate() throws Exception {
final XQueryService queryService = testCollection.getService(XQueryService.class);

final String update = "update insert <item>new node</item> into doc('" + path + "')//list";
final ResourceSet updateResult = queryService.queryResource(TestConstants.TEST_XML_URI.toString(), update);
final ResourceSet updateResult = queryService.queryResource(path, update);
assertEquals("Update expression returns an empty sequence", 0, updateResult.getSize());

final ResourceSet itemResult = queryService.queryResource(TestConstants.TEST_XML_URI.toString(), "//item");
final ResourceSet itemResult = queryService.queryResource(path, "//item");
assertEquals("Both items are returned", 2, itemResult.getSize());
}

Expand Down
Loading