https://bugs.documentfoundation.org/show_bug.cgi?id=173266
Bug ID: 173266
Summary: FILEOPEN PPTX: Impress crashes when importing a
comments part with a comment position element (p:pos)
that has no preceding comment (p:cm
Product: LibreOffice
Version: 27.2.0.0 alpha0+ master
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: normal
Priority: medium
Component: Base
Assignee: [email protected]
Reporter: [email protected]
Description:
Dear LibreOffice-Team,
A PPTX whose comments part contains a `<p:pos>` element that no `<p:cm>`
precedes crashes the Impress import.
`SlideFragmentHandler` calls `back()` on an empty vector, which the libc++
hardening in the OSS-Fuzz build turns into a trap and which is undefined in a
build without hardening.
This bug was found with a new fuzzing approach via the `libreoffice` oss-fuzz
target `pptxfuzzer` using the `core` master branch at commit
`50ae505eb3ef23a3ff4a52a62d41a6f0fa6683ab`, built by OSS-Fuzz with ASan.
`oox/source/ppt/slidefragmenthandler.cxx` handles the `pos` element without
checking that a comment exists to attach it to:
```c++
231 case PPT_TOKEN( pos ):
232 mpSlidePersistPtr->getCommentsList().cmLst.back().setPoint(
233 rAttribs.getStringDefaulted(XML_x),
234 rAttribs.getStringDefaulted(XML_y));
235 break;
```
`cmLst` only ever grows in the `cm` case at line 218, the one `emplace_back` on
it in the whole tree, and that case does guard the same call:
```c++
218 case PPT_TOKEN( cm ):
219 if (!mpSlidePersistPtr->getCommentsList().cmLst.empty() &&
!getCharVector().empty())
220 {
221 // set comment text for earlier comment
222 mpSlidePersistPtr->getCommentsList().cmLst.back().setText(
getCharVector().back() );
223 }
224 // insert a new comment in vector commentsList
225 mpSlidePersistPtr->getCommentsList().cmLst.emplace_back();
```
So a `pos` that arrives before any `cm` was recognised finds `cmLst` empty.
```
==40==ERROR: AddressSanitizer: ILL on unknown address 0x55555f9f41b2
SCARINESS: 10 (signal)
#0 in __clang_trap_msg$libc++$ ... libc++ Hardening assertion !empty()
failed: back() called on an empty vector
#2 in oox::ppt::SlideFragmentHandler::onCreateContext(int,
oox::AttributeList const&)
#3 in /src/libreoffice/oox/source/ppt/slidefragmenthandler.cxx:232:52
#5 in oox::core::ContextHandler2Helper::implCreateChildContext(...)
oox/source/core/contexthandler2.cxx:99:34
#6 in createFastChildContext oox/source/core/fragmenthandler2.cxx:67:12
#8 in (anonymous namespace)::Entity::startElement(...)
sax/source/fastparser/fastparser.cxx:468:44
```
The caller guarantees the vector starts out empty,
`oox/source/ppt/presentationfragmenthandler.cxx`:
```c++
690 pCommentsPersistPtr->getCommentsList().cmLst.clear();
691 getFilter().importFragment( xCommentsFragmentHandler );
692
693 if
(!pCommentsPersistPtr->getCommentsList().cmLst.empty())
694 {
695 //set comment chars for last comment on slide
696 SlideFragmentHandler* comment_handler =
697
dynamic_cast<SlideFragmentHandler*>(xCommentsFragmentHandler.get());
698 assert(comment_handler);
699 // some comments have no text -> set empty string
as text to avoid
700 // crash (back() on empty vector is undefined) and
losing other
701 // comment data that might be there (author,
position, timestamp etc.)
702
pCommentsPersistPtr->getCommentsList().cmLst.back().setText(
```
The same hazard on the same vector was already found and fixed here, one call
site over, and the comment at line 700 names the undefined behaviour.
The `pos` case is the one `back()` on `cmLst` that was left unguarded.
The attached `minimal_pos_without_cm.pptx` is a hand-built presentation whose
`ppt/comments/comment1.xml` is exactly this:
```xml
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<p:cmLst
xmlns:p="http://schemas.openxmlformats.org/presentationml/2006/main";><p:pos
x="0" y="0"/></p:cmLst>
```
`control_pos_with_cm.pptx` is the same presentation with that `<p:pos>` wrapped
in a `<p:cm>` that also carries a `<p:text>`.
The two differ in `ppt/comments/comment1.xml` and in nothing else, and every
part of both is well-formed XML.
In the original fuzzer testcase
`crash-1bda1266325d41f5430d2680a43366f8062ce220` the comment element is present
but its name is damaged, `<p:ca umthorId="0" ...>` instead of `<p:cm
authorId="0" ...>`, so `PPT_TOKEN( cm )` never matches while the nested
`<p:pos>` still does.
Any spelling the token table does not know has the same effect, which is why
the missing guard and not the specific mutation is the defect.
That part is also not well-formed, since the element closes as `</p:cm>`, but
the crash happens on the `<p:pos>` start tag before the parser reaches the
mismatch.
`unzip -t` reports a bad CRC-32 on it and on ten of the other thirty-four
parts, because the fuzzer rewrote them in place without fixing the checksums,
so the hand-built presentations above are the artifacts to reproduce from.
The same fuzz target produced a second, unrelated defect in the table style
code, reported separately as bug 165.
## Fix
We are not shipping a patch, but the suggested fix is to give `pos` the
emptiness check that `cm` already has:
```diff
case PPT_TOKEN( pos ):
- mpSlidePersistPtr->getCommentsList().cmLst.back().setPoint(
- rAttribs.getStringDefaulted(XML_x),
- rAttribs.getStringDefaulted(XML_y));
+ if (!mpSlidePersistPtr->getCommentsList().cmLst.empty())
+ {
+ mpSlidePersistPtr->getCommentsList().cmLst.back().setPoint(
+ rAttribs.getStringDefaulted(XML_x),
+ rAttribs.getStringDefaulted(XML_y));
+ }
break;
```
Dropping the position of a comment that does not exist is the only sensible
reading, and it matches what the `cm` case does with the text of a comment that
does not exist.
## Reproduce
The original fuzzer testcase, its CASR report and the two hand-built
presentations are attached.
```bash
export DOCKER_DEFAULT_PLATFORM=linux/amd64 # if on mac
git clone https://github.com/google/oss-fuzz.git
cd oss-fuzz
python3 infra/helper.py build_image libreoffice
python3 infra/helper.py build_fuzzers --sanitizer address libreoffice
python3 infra/helper.py reproduce libreoffice pptxfuzzer
crash-1bda1266325d41f5430d2680a43366f8062ce220
python3 infra/helper.py reproduce libreoffice pptxfuzzer
minimal_pos_without_cm.pptx
python3 infra/helper.py reproduce libreoffice pptxfuzzer
control_pos_with_cm.pptx
```
We tested against OSS-Fuzz at commit
`a4df12d70b5420567d893b3f53e5818a74df5db7`, default `linux/amd64` containers,
and `core` master at commit `50ae505eb3ef23a3ff4a52a62d41a6f0fa6683ab`.
Found by the CISPA Fandango-Team while triaging OSS-Fuzz findings for
libreoffice.
Steps to Reproduce:
<-- Download and unzip the Reproduce.zip -->
export DOCKER_DEFAULT_PLATFORM=linux/amd64 # if on mac
git clone https://github.com/google/oss-fuzz.git
cd oss-fuzz
python3 infra/helper.py build_image libreoffice
python3 infra/helper.py build_fuzzers --sanitizer address libreoffice
python3 infra/helper.py reproduce libreoffice pptxfuzzer
crash-1bda1266325d41f5430d2680a43366f8062ce220
python3 infra/helper.py reproduce libreoffice pptxfuzzer
minimal_pos_without_cm.pptx
python3 infra/helper.py reproduce libreoffice pptxfuzzer
control_pos_with_cm.pptx
Actual Results:
==40==ERROR: AddressSanitizer: ILL on unknown address 0x55555f9f41b2
SCARINESS: 10 (signal)
#0 in __clang_trap_msg$libc++$ ... libc++ Hardening assertion !empty()
failed: back() called on an empty vector
#2 in oox::ppt::SlideFragmentHandler::onCreateContext(int,
oox::AttributeList const&)
#3 in /src/libreoffice/oox/source/ppt/slidefragmenthandler.cxx:232:52
#5 in oox::core::ContextHandler2Helper::implCreateChildContext(...)
oox/source/core/contexthandler2.cxx:99:34
#6 in createFastChildContext oox/source/core/fragmenthandler2.cxx:67:12
#8 in (anonymous namespace)::Entity::startElement(...)
sax/source/fastparser/fastparser.cxx:468:44
Expected Results:
No crash
Reproducible: Always
User Profile Reset: No
Additional Info:
This bug was found with a new fuzzing approach via the `libreoffice` oss-fuzz
target `pptxfuzzer` using the `core` master branch at commit
`50ae505eb3ef23a3ff4a52a62d41a6f0fa6683ab`, built by OSS-Fuzz with ASan.
--
You are receiving this mail because:
You are the assignee for the bug.
