External DTD files are accessed without a privileged block when Derby parses XML values that reference such DTDs.

• Key: DERBY-2131
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: A B
• Reporter: A B
• Component/s: SQL
• Affects Version/s: 10.2.1.6, 10.2.2.0, 10.3.1.4
• Fix Version/s: 10.2.2.0, 10.3.1.4

Description

The Derby XMLPARSE operator ultimately makes a call to an external JAXP parser (ex. Xerces or Crimson) to parse an XML value. If the XML value that is being parsed references an external DTD, then the JAXP parser will need to read the DTD file to complete parsing. However, the current code in SqlXmlUtil.java does not use a privileged block when it calls out to the JAXP parser. As a result, when a user who is running with a security manager tries to insert a document that references an external DTD, the call to XMLPARSE will fail with a security exception--even if the JAXP parser has the required "read" permissions.

Comments

[A B]

Attaching a patch, d2131_v1.patch, that wraps the call to JAXP inside a priveleged block. I ran tests with some local (soon-to-be-posted) changes for DERBY-1758 to confirm that the patch solves the reported problem (i.e. that assignment of "read" permission to the JAXP parser allows successful execution of XMLPARSE). I also ran derbyall on Red Hat Linux using ibm142 with no failures. The "XMLSuite" JUnit suite also ran without error.

The patch doesn't include any tests; however, relevant test cases will be enabled as part of DERBY-1758 to verify the behavior.

I am very new to the notion of security managers and privileged blocks, so while this is a small patch, I would appreciate it if someone could review it to make sure that the changes make sense...

[A B]

Marking this issue as "blocking" DERBY-1758 since the latter cannot be resolved until the lang/XMLBindingTest is running under a security manager, and that cannot happen until this issue (DERBY-2131) is resolved.

[A B]

derbyall and suites.All both ran without errors and there were no objections to the posted patch, so I committed d2131_v1.patch with svn 481117.

[A B]

Reopening to set fix version.

[Daniel John Debrunner]

Finally got around to looking at the patch. It's generally correct, but you probably want to catch the PrivilegedExceptionAction and unwrap it. Then if there's an i/o error accessing the DTD it won't appear as a possible security exception to the user.

The other general rule with privileged blocks is to put as little code as possible into the priv block, I'm not sure what code requires the privilege block, but here's an example re-write.

       final InputSource is = new InputSource(new StringReader(xmlAsText));
        aList.add( java.security.AccessController.doPrivileged(
            new java.security.PrivilegedExceptionAction()
            {
                public Object run() throws Exception
                {
                    return dBuilder.parse(is);
                }
            }));

[A B]

Thanks for the suggestions, Dan. This is good to know. I'll work on a follow-up patch to make the changes you mention here and will post it tomorrow.

Re-opening the issue for now.

[A B]

Attaching a second patch, d2131_rewrite_v1.patch, that rewrites the privileged block (which has already been committed) to account for Dan's suggestions.

Regarding "you probably want to catch the PrivilegedExceptionAction and unwrap it. ". I did some quick looking at other places in the Derby code where PrivilegedExceptions are caught and it seems like unwrapping the exception is simply a matter of using the result of "PrivilegedException.getException()"--so in this case, we throw the result of "getException()" instead of throwing the PrivilegedException itself.

If this was too simple of an interpretation (ex. if the unwrapping needs to be more extensive or should be selective to i/o errors), please let me know and I can try again...

[Daniel John Debrunner]

DocumentBuilder.parse() is documented to throw these exceptions: SAXException, IOException but the run() method declares itself as throwing Exception. WOuld it not be better to have the specific errors?

Another version of the "rewrite" patch that declares the correct exceptions on the privileged "run" method. Thank you for pointing that out, Dan.

[A B]

Committed d2131_rewrite_v2.patch with svn 482303.

[A B]

Attaching a patch to port this fix back to 10.2. I ran the old "xmlSuite" on 10.2 jars with this patch applied and they ran without problem. I'm running derbyall now just to be safe and will commit the patch to 10.2 if all goes well.

d2131_10_2.patch was created as follows:

svn merge -r 481116:481117 https://svn.apache.org/repos/asf/db/derby/code/trunk

svn merge -r 482302:482303 https://svn.apache.org/repos/asf/db/derby/code/truk

svn diff > d2131_10_2.patch

[A B]

derbyall ran cleanly with 10.2 jars after applying this patch. And the XML tests in suites.xmlSuite all ran cleanly, as well. So I committed d2131_10_2.patch with svn 482837.

[A B]

No further issues/comments after the changes were committed, so marking this as closed.