fix(apng): Resume the push-mode chunk skip after a partial buffer - #906
fix(apng): Resume the push-mode chunk skip after a partial buffer#906Nexory wants to merge 2 commits into
Conversation
png_push_read_IDAT() skips a chunk that appears in the middle of APNG frame data by warning and consuming the chunk body, which needs the whole chunk to be buffered. The check for that sat inside the "chunk header not yet read" block, so when the buffer was short the function saved the buffer and returned with the chunk header flag already set and the body unconsumed. The next call skipped that block entirely, reached the idat_size == 0 tail and consumed the first four bytes of the chunk body as if they were its CRC, after which the rest of the body was parsed as a chunk stream. Attacker-chosen bytes inside an ignored chunk were therefore smuggled into the frame-data path, which is what faf0692 removed from png_push_read_chunk; png_push_read_IDAT kept it because its dispatch is nested inside the header-parse block instead of sitting after it. Move the skip out of that block so it is reached on every call while the chunk header is held, the way png_push_read_chunk does it. The condition selecting the skip is unchanged, so no chunk changes category and a reader that receives whole chunks at once behaves exactly as before; only the resumption after a partial buffer differs.
|
The browsers are the guys who demanded the push reader and this is their code. Does anyone else use it? I suggest upstreaming it to them. I'll take a look; this sounds credible. You may hear me laughing. |
jbowler
left a comment
jbowler
left a comment
There was a problem hiding this comment.
At lines 536,537 you add a variable "chunk_read_header" to fix a bug when PNG_READ_APNG_SUPPORTED.
This variable declaration must also be protected by PNG_READ_APNG_SUPPORTED
The chunk_header_read flag and the idat_size assignment pulled out of the header-parse block are only needed for the APNG resume path, so confine them to PNG_READ_APNG_SUPPORTED. Non-APNG builds set idat_size in the header block as before, leaving that path unchanged from pnggroup#906. Builds clean with -Wall -Wextra -Werror both with and without APNG.
|
Done. I've confined It's getting late here, so I'll check back on this tomorrow. |
|
Sorry, my earlier "unchanged from #906" wording was imprecise. Stated explicitly: Confirmed: with Verified two ways: preprocessing |
That sounds good. I haven't verified this but this is the way it should go unless someone wants to redo the full work that unified the two readers in libpng 1.7 and then change all the individual chunk readers to be state driven rather than a mess of nested C {}. ATM fdAT looks just like IDAT and IDAT is the only chunk in the base code which does an incremental read in the push reader. The original (Stepin?) APNG code tried to do the same for fdAT but I couldn't convince myself it was correct. The original push reader (Greg's implementation IRC) did seem to be set up to do incremental read of other chunks (e.g. IRC the text chunks) but the implementation seemed to me to be incomplete and, maybe, not even reachable. I don't think there is any reasonable way forward from the current code so it seems more realistic to fix the bugs one-by-one. For that reason I think this is a good approach, however I have not code reviewed the resultant code so someone familiar with the APNG stuff needs to do it. |
2 participants
What
png_push_read_IDAT()skips a chunk that turns up in the middle of APNG framedata by warning and consuming the chunk body, which requires the whole chunk to
be buffered. That check sits inside the "chunk header not yet read" block,
so on a short buffer the function saves the buffer and returns with
PNG_HAVE_CHUNK_HEADERalready set and the body unconsumed. The next call skipsthat block, reaches the
idat_size == 0tail and callspng_crc_finish(png_ptr, 0), consuming the first four bytes of the chunkbody as its CRC; the rest of the body is then parsed as a chunk stream.
This is the same defect
faf069246removed frompng_push_read_chunk, whosemessage notes that "an early return from insufficient buffer data cannot sit
between the sequence read and the body consumption".
png_push_read_chunkissafe because its APNG dispatch sits after the header-parse block, so a
re-entry re-evaluates it. In
png_push_read_IDATthe dispatch is nestedinside that block, so the re-entry never comes back to it.
Whether this is reached depends only on how the caller happens to split its
png_process_data()calls, which for a streaming decoder is network or pipearrival, not something the file author needs to control.
Fix
Move the skip out of the header-parse block, so it is reached on every call
while the chunk header is held. The condition selecting the skip is unchanged,
so no chunk changes category, and a reader that receives whole chunks at once
behaves exactly as before. Only the resumption after a partial buffer differs.
Verification
A harness builds a 2-frame APNG with libpng's own writer, splices an unknown
ancillary chunk between two
fdATchunks of the second frame, and feeds theresult at 21 different
png_process_data()granularities, comparing eachagainst the whole-file feed. A plain PNG runs through the same sweep first as a
control.
Diff of the harness output, before against after, is exactly this and nothing
else:
Every other line is identical, including the whole-file results
(
rows=256 frames=2 warn=1, warning "Ignoring unexpected chunk in APNGsequence") and the plain-PNG control (
diverged=0both before and after, so theharness itself is granularity-neutral). The "CRC error" warnings only appear
when body bytes are consumed as a CRC, which is the desynchronisation being
reported.
ctestpasses 37/37 with the change (includingpngvalid,pngstest,pngunknownandpngimage-full),pngpread.ccompiles clean under-Wall -Wextra, and it also compiles withPNG_READ_APNG_SUPPORTEDoff.I can attach the harness if that is useful. I did not add a test file because
the two neighbouring APNG push-mode fixes,
faf069246and9ec49c2d5, arecode-only, but I am happy to add one in whatever form you prefer.
Relation to #903
#903 adds a
PNG_CHUNK_CRITICAL()rejection to this same branch, but insertsit after the
push_length + 4 > buffer_sizeearly return, so on a short bufferthat check is skipped along with the rest of the branch. The two changes are
complementary rather than alternatives, and they will conflict textually. Happy
to rebase on top of #903 if it goes in first.