#517 Push operations in DrawImage can lead to negative strncpy when looking for pop

[Jeriko One]

Built from source
GraphicsMagick 1.4 snapshot-20171007 Q8

Files:
nested_clippaths.svg - SVG file that will hit the clip-path. This is the only one that I found that could be hit using a format other than MVG. This could be a functonality problem as well if nested clippaths are meant to be supported.

pattern.mvg - MVG file that hits the pattern path
gradient.mvg - MVG file that hits the gradient path

$ gm convert nested_clippaths.svg /tmp/test.jpg
gm convert: abort due to signal 11 (SIGSEGV) "Segmentation Fault"...
Aborted

The same pattern around "push" happens in several places when looking for the corresponding pop operation. Instead of opening a ticket for each fo them I put them in this one. If it'd be better for me to split this let me know.

In DrawImage the "push" keyword has several actions depending on the following token.

clip-path, pattern, and gradient tokens can lead to strncpy of negative lengths, when a NULL byte is encountered when looking for their "pop" counterparts.

for the "clip-path" path

2353                 for (p=q; *q != '\0'; )
2354                 {
2355                   MagickGetToken(q,&q,token,token_max_length);
2356                   if (LocaleCompare(token,"pop") != 0)
2357                     continue;
2358                   MagickGetToken(q,(char **) NULL,token,token_max_length);
2359                   if (LocaleCompare(token,"clip-path") != 0)
2360                     continue;
2361                   break;
2362                 }
2363                 (void) strncpy(token,p,q-p-4);

It's clear that if p starts off as '\0' then q = p, and the strncpy will have a length of -4. The amount of characters between p and q has to be at least 4 or you'll end up with a negative strncpy.

the same problem exist for "gradient"

2426                 for (p=q; *q != '\0'; )
2427                 {
2428                   MagickGetToken(q,&q,token,token_max_length);
2429                   if (LocaleCompare(token,"pop") != 0)
2430                     continue;
2431                   MagickGetToken(q,(char **) NULL,token,token_max_length);
2432                   if (LocaleCompare(token,"gradient") != 0)
2433                     continue;
2434                   break;
2435                 }
2436                 (void) strncpy(token,p,q-p-4);

and "pattern"

2557                 for (p=q; *q != '\0'; )
2558                 {
2559                   MagickGetToken(q,&q,token,token_max_length);
2560                   if (LocaleCompare(token,"pop") != 0)
2561                     continue;
2562                   MagickGetToken(q,(char **) NULL,token,token_max_length);
2563                   if (LocaleCompare(token,"pattern") != 0)
2564                     continue;
2565                   break;
2566                 }
2567                 (void) strncpy(token,p,q-p-4);

ASan output

=================================================================
==7426==ERROR: AddressSanitizer: negative-size-param: (size=-4)
    #0 0x7ffff6ee27ba in strncpy (/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/libasan.so.2+0x767ba)
    #1 0x523e2c in DrawImage magick/render.c:2363
    #2 0x51e6ff in DrawClipPath magick/render.c:1433
    #3 0x5211e2 in DrawImage magick/render.c:1956
    #4 0x73b8b0 in ReadMVGImage coders/mvg.c:195
    #5 0x4782bb in ReadImage magick/constitute.c:1607
    #6 0x7c1e10 in ReadSVGImage coders/svg.c:3021
    #7 0x4782bb in ReadImage magick/constitute.c:1607
    #8 0x420389 in ConvertImageCommand magick/command.c:4348
    #9 0x4358f1 in MagickCommand magick/command.c:8869
    #10 0x45fe3a in GMCommandSingle magick/command.c:17396
    #11 0x460086 in GMCommand magick/command.c:17449
    #12 0x40ba65 in main utilities/gm.c:61
    #13 0x7ffff447666f in __libc_start_main (/lib64/libc.so.6+0x2066f)
    #14 0x40b978 in _start (/home/j/graphicsmagick/build/bin/gm+0x40b978)

0x611000007340 is located 0 bytes inside of 256-byte region [0x611000007340,0x611000007440)
allocated by thread T0 here:
    #0 0x7ffff6f04572 in malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/libasan.so.2+0x98572)
    #1 0x4dbc0d in MagickMalloc magick/memory.c:156
    #2 0x5750af in AllocateString magick/utility.c:220
    #3 0x5208db in DrawImage magick/render.c:1837
    #4 0x51e6ff in DrawClipPath magick/render.c:1433
    #5 0x5211e2 in DrawImage magick/render.c:1956
    #6 0x73b8b0 in ReadMVGImage coders/mvg.c:195
    #7 0x4782bb in ReadImage magick/constitute.c:1607
    #8 0x7c1e10 in ReadSVGImage coders/svg.c:3021
    #9 0x4782bb in ReadImage magick/constitute.c:1607
    #10 0x420389 in ConvertImageCommand magick/command.c:4348
    #11 0x4358f1 in MagickCommand magick/command.c:8869
    #12 0x45fe3a in GMCommandSingle magick/command.c:17396
    #13 0x460086 in GMCommand magick/command.c:17449
    #14 0x40ba65 in main utilities/gm.c:61
    #15 0x7ffff447666f in __libc_start_main (/lib64/libc.so.6+0x2066f)

SUMMARY: AddressSanitizer: negative-size-param ??:0 strncpy
==7426==ABORTING

[Bob Friesenhahn]

These issues are fixed by Mercurial changeset 15243:785758bbbfcc. Thank you for notifying us of this issue, with such a well written analysis, and with useful test-cases.