Bug 58597

Summary: XWPFDocument causes SecurityException under securitymanager
Product: POI Reporter: rmuir
Component: XWPFAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 3.13-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Attachments: patch
improved patch

Description rmuir 2015-11-07 23:06:01 UTC
When running under securitymanager, this calls setAccessible() to violate access rules, and will lead to an exception like this:

   > Throwable #1: org.apache.tika.exception.TikaException: Unexpected RuntimeException from org.apache.tika.parser.microsoft.ooxml.OOXMLParser@38ba0fca
   > 	at __randomizedtesting.SeedInfo.seed([30F5D02182B6C2A6:BD8A17A749023F4A]:0)
   > 	at org.apache.tika.parser.CompositeParser.parse(CompositeParser.java:282)
   > 	at org.apache.tika.parser.CompositeParser.parse(CompositeParser.java:280)
   > 	at org.apache.tika.parser.AutoDetectParser.parse(AutoDetectParser.java:120)
   > 	at org.apache.tika.Tika.parseToString(Tika.java:537)
   > 	at org.elasticsearch.mapper.attachments.AttachmentMapper$1.run(AttachmentMapper.java:624)
   > 	at org.elasticsearch.mapper.attachments.AttachmentMapper$1.run(AttachmentMapper.java:621)
   > 	at java.security.AccessController.doPrivileged(Native Method)
   > 	at org.elasticsearch.mapper.attachments.AttachmentMapper.parseWithTika(AttachmentMapper.java:621)
   > 	at org.elasticsearch.mapper.attachments.VariousDocTests.assertParseable(VariousDocTests.java:127)
   > 	at org.elasticsearch.mapper.attachments.VariousDocTests.testWordDocxDocument104(VariousDocTests.java:62)
   > 	at java.lang.Thread.run(Thread.java:745)
   > Caused by: org.apache.poi.POIXMLException: java.security.AccessControlException: access denied ("java.lang.reflect.ReflectPermission" "suppressAccessChecks")
   > 	at org.apache.poi.xwpf.usermodel.XWPFDocument.onDocumentRead(XWPFDocument.java:231)
   > 	at org.apache.poi.POIXMLDocument.load(POIXMLDocument.java:177)
   > 	at org.apache.poi.xwpf.usermodel.XWPFDocument.<init>(XWPFDocument.java:119)
   > 	at org.apache.poi.xwpf.extractor.XWPFWordExtractor.<init>(XWPFWordExtractor.java:58)
   > 	at org.apache.poi.extractor.ExtractorFactory.createExtractor(ExtractorFactory.java:204)
   > 	at org.apache.tika.parser.microsoft.ooxml.OOXMLExtractorFactory.parse(OOXMLExtractorFactory.java:86)
   > 	at org.apache.tika.parser.microsoft.ooxml.OOXMLParser.parse(OOXMLParser.java:87)
   > 	at org.apache.tika.parser.CompositeParser.parse(CompositeParser.java:280)
   > 	... 46 more
   > Caused by: java.security.AccessControlException: access denied ("java.lang.reflect.ReflectPermission" "suppressAccessChecks")
   > 	at java.security.AccessControlContext.checkPermission(AccessControlContext.java:457)
   > 	at java.security.AccessController.checkPermission(AccessController.java:884)
   > 	at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
   > 	at java.lang.reflect.AccessibleObject.setAccessible(AccessibleObject.java:128)
   > 	at org.apache.poi.xwpf.usermodel.XWPFDocument.onDocumentRead(XWPFDocument.java:228)
   > 	... 53 more


Can this be avoided somehow? Thanks!
Comment 1 Uwe Schindler (ASF) 2015-11-07 23:07:58 UTC
Hi Robert,
I found the problematics code part. In general, setAccessible should be added to POI's forbidden-apis, because libraries embedded in other projects should never ever violate Java Language constraints.
I will check for other AccessibleObject#setAccesible.
Comment 2 Uwe Schindler (ASF) 2015-11-07 23:13:58 UTC
I have no idea why we need reflection at all. We should just remove this. The target class is a POI-internal class, why not just make the problematic method public?
Comment 3 Uwe Schindler (ASF) 2015-11-07 23:57:16 UTC
I reviewed the POI code: There are tons of this stuff inside.
Some code parts also try to access stuff inside JVM's classes. This will break with Java 9, as setAccessible on classes from Java runtime will fail.

I have no idea how to fix this mess. Giving up for now. The code does not even use AccessController.doPrivileged(), so one could limit the additional security permissions to POI's JAR files.
Comment 4 Dominik Stadler 2015-11-08 10:06:52 UTC
FYI, I added some initial rules for reflection usage in r1713218, they are currently commented out to not break the build.
Comment 5 Uwe Schindler (ASF) 2015-11-08 11:35:48 UTC
Thanks Dominik. I was about to do the same.

At least we should wrap all reflection usage with AccessController.doPrivileged(), as this allows to enable this type of reflection only for the POI JAR file and not your whole project.
Comment 6 Uwe Schindler (ASF) 2015-11-09 09:11:56 UTC
I started to fix some of the setAccessible stuff. The first one was the mmap unmapper, which I copied from Lucene. See r1713350.
Comment 7 Uwe Schindler (ASF) 2015-11-09 11:38:18 UTC
Created attachment 33267 [details]
patch

First patch removing many useless setAccessible in POIs internal classes:

It adds getters marked with @Internal annotation. For the issue reported here, it is a bit more stupid, because you cannot make the method public, because it is protected by design. I added a public Accessor class with a static method that delegates. Maybe we shoud fix this in another way.

What do others think?

There are more stuff like this, I just started. There are some of those in tests, which is easy to fix in a similar way (using Accessor class inside tests, just placed in right package).
Comment 8 Uwe Schindler (ASF) 2015-11-10 23:20:29 UTC
I will commit the patch tomorrow and proceed with more fixes like this. This should also fix the issue of Robert.
Comment 9 Uwe Schindler (ASF) 2015-11-11 07:40:29 UTC
Created attachment 33270 [details]
improved patch

New patch that does not add a new class for the hack. I just added a static method in POIXMLDocumentPart to allow access to onDocumentRead, but clearly marked as @Internal.

On longer term the class structure and method acceibility should be reviewed. From looking at the code onDocument*() should all be public.

I'll commit this now.
Comment 10 Uwe Schindler (ASF) 2015-11-11 07:43:19 UTC
I committed this now. There are more setAccessible calls outside of test that are not yet wrapped. I will work on remving them in a similar way.

The root cause of this issue is now fixed, but the other problems must be solved too. Robert just did not see them lack of suitable test documents to trigger setAccessible calls.
Comment 11 Uwe Schindler (ASF) 2015-11-11 19:26:28 UTC
I removed all useless reflection in the remaining code:
- For test cases I added helper methods to do the actual reflection inside a doPrivileged. This also checks that you only reflect on POI classes (package check).
- Made some properties public with @Internal (where needed)
- Wrapped remaining stuff with doPrivileged()

I will open new issues for the following problems:
- Under Java 9, the ooxml-lite generator code will definitely fail. My idea is to use a separate classloader that tracks all loaded classes instead of reflecting into the ClassLoader
- The ZIP bomb detection will also fail in Java 9. The code should be refactored to wrap the zip file before opening a ZIPInputStream. This is not so easy, maybe somebody has an idea (I don't understand what's going on!!!)

I'll close this issue.